Files
rdbms-playground/docs/plans/20260525-adr-0035-sql-ddl-4f.md
T
claude@clouddev1 a2fc3c9e57 docs: 4f plan + session handoff 39
- docs/plans/20260525-adr-0035-sql-ddl-4f.md — ALTER TABLE ALTER COLUMN
  TYPE plan (/runda'd; forks resolved). Advanced lossy = ForceConversion
  (reuse do_change_column_type; existing transformed_lossy note); the
  internal-__rdbms_* guard folds into do_change_column_type both surfaces
  (user-confirmed); int->serial is ALLOWED (ADR-0018 §8), only non-int
  ->serial / blob / date<->datetime are static-refused.
- docs/handoff/20260525-handoff-39.md — 4d/4e shipped; 4f is next
  (plan ready).
2026-05-25 20:26:34 +00:00

14 KiB
Raw Permalink Blame History

Plan: ADR-0035 Phase 4, sub-phase 4f — ALTER TABLE … ALTER COLUMN TYPE

Add ALTER TABLE <T> ALTER COLUMN <col> TYPE <type> as a fourth AlterTableAction (AlterColumnType), runtime-decomposed to the existing do_change_column_type executor with the advanced-mode policy (ADR-0035 §7): lossy conversions are performed with a note, no force flag; static-refused (→serial, ↔blob) and incompatible conversions still refuse (ADR-0017, shared by both modes). This is the smallest 4e-style slice — almost entirely reuse.

The single new executor change is folding the internal-__rdbms_* guard into do_change_column_type (the 4e follow-up), so the simple change column surface is closed too.

1. Baseline (at handoff)

  • Tests: 1854 passing, 0 failing, 0 skipped, 1 ignored; clippy clean. Branch main, HEAD bbc2e34 (4e). 4f starts here. (4a4e are local-only commits since origin/main.)

2. Decisions (settled — ADR §7/§13 4f + user-confirmed 2026-05-25)

  1. Advanced lossy = ChangeColumnMode::ForceConversion — no new mode, no new note. ForceConversion already is the §7 advanced policy: static_refusal refuses →serial/↔blob; the dry-run refuses incompatible cells; lossy cells are transformed; the ClientSideNote { transformed, lossy } is surfaced. The existing client_side.transformed_lossy catalog note ("N row(s) transformed; M of those discarded information (lossy)…") is engine-neutral and reads correctly for advanced — it is the §7 "N values converted with loss" note. (Update the stale // fires under --force-conversion comment in en-US.yaml to mention advanced ALTER COLUMN TYPE too.)
  2. No SQL spelling for --force-conversion / --dont-convert (ADR §7/§12). Advanced always performs the conversion (relying on undo as the safety net — a payoff of shipping ADR-0006 first). The Postgres USING <expr> clause is not adopted (§12).
  3. Static-refused + incompatible reuse the executor (ADR-0017, shared by both modes). Verified static_refusal (type_change.rs) refuses: same-type (already X); non-int → serial (only int → serial is allowed); any ↔ blob; direct date ↔ datetime (route via text); and any pair not in the matrix. An incompatible per-cell conversion → the existing diagnostic. NOTE — ADR-0035 §7's "static-refused (→serial …)" summary is looser than the code: int → serial IS allowed (ADR-0018 §8 — auto-fills null cells with generated values, adds UNIQUE if non-PK), so advanced ALTER COLUMN c TYPE serial on an int column is a supported operation, not a refusal. The implementer must test int → serial (allowed) and text → serial / ↔ blob (refused) — do NOT "fix" int→serial to refuse.
  4. TYPE keyword onlyALTER COLUMN <col> TYPE <type> (ADR §4). The Postgres SET DATA TYPE synonym is not added (kept minimal; could be a later alias).
  5. Internal-__rdbms_* guard folded into do_change_column_type (user-confirmed 2026-05-25) — closes the simple change column exposure alongside the parse-time guard the SQL ALTER table slot already gives. do_add_constraint / do_add_relationship stay flagged for 4g.

3. Phase 1 — Requirements checklist (4f)

Grammar / dispatch:

  • ALTER TABLE <T> ALTER COLUMN <col> TYPE <type> parses in advanced mode → SqlAlterTable { AlterColumnType { column, ty } }; <type> accepts the SQL type vocabulary + aliases (reuse SQL_TYPE).
  • The action Choice now has four branches (add/drop/rename/ alter-column), each leading on a distinct concrete keyword — trap-safe. The other three still parse; alter remains advanced-only; the table slot still rejects __rdbms_* at parse.
  • Trailing ; tolerated.

Execution (reuse do_change_column_type with ForceConversion):

  • Clean conversion (e.g. int → text) auto-converts; one undo step; auto-show.
  • Lossy conversion (e.g. real → int, 3.7 → 3) is performed and surfaces the [client-side] … discarded information (lossy) note (no force flag, no refusal) — the §7 advanced behaviour.
  • Incompatible conversion is refused (friendly, engine-neutral) — e.g. text → int where a cell holds a non-numeric string.
  • Static-refused refused: text → serial (non-int source), ↔ blob, date ↔ datetime direct, same-type. And int → serial is allowed (auto-fills nulls, adds UNIQUE — ADR-0018 §8): test it as a supported conversion, not a refusal.
  • FK-involved column type change refuses per the existing executor rules (outbound/inbound), via the SQL path too.
  • The converted column round-trips and survives rebuild (the user_type metadata update is the existing path).

Internal-table guard (both surfaces):

  • do_change_column_type refuses an internal __rdbms_* table (as "no such table"); the simple change column surface is closed. A test on the simple surface.

Testing

  • Tier 1 (extend sql_alter_table_tests in ddl.rs): the alter-column-type parse (incl. a type alias) → AlterColumnType; the four-branch dispatch still routes add/drop/rename correctly.
  • Tier 3 (extend tests/sql_alter_table.rs, via run_replay): clean convert (int → text); lossy convert (real → int) performs — assert the value was converted (e.g. 3.7 stored as 3); incompatible refused; text → serial (and/or ↔ blob) refused; int → serial allowed (auto-fill); rebuild survival; one undo step. (Assert the lossy data conversion as the primary signal; the [client-side] … lossy note is rendered through the shared ChangeColumn path.)
  • Internal guard (extend tests/column_op_guards.rs): simple change column on __rdbms_* refused.
  • Catalog lockstep + vocab audit for the refreshed sql_alter_table help/usage (it now lists the alter-column-type form).

4. Architecture & design

4.1 Command (src/dsl/command.rs)

  • AlterTableAction gains AlterColumnType { column: String, ty: Type } (small variant — no boxing needed; the boxed AddColumn already dominates the enum's size).

4.2 Grammar (src/dsl/grammar/ddl.rs)

  • New action branch AT_ALTER_COLUMN = Seq[ Word("alter"), Word("column"), COLUMN_NAME, Word("type"), super::sql_create_table::SQL_TYPE ]. Add it to AT_ACTION_CHOICES (now [AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN, AT_ALTER_COLUMN]). Leads on the concrete alter keyword (distinct from add/drop/rename) — trap-safe. The action's alter is the 4th token (the entry-word alter is consumed by dispatch first).
  • build_sql_alter_table: add a branch. Discriminate on the type keyword (path.contains_word("type")) — unique to ALTER COLUMN TYPE (ADD COLUMN's type is a col_type ident, not the literal type keyword). Extract column = require_ident("column_name") and the target Type from the col_type ident (Type::from_sql_name) / the double precision two-word case — mirror the type-extraction in build_alter_add_column_spec. Order the type check before add/rename/drop.

4.3 Worker / executor (src/db.rs)

  • No new method. Add the internal-table guard: reject_internal_table_name(table)?; at the top of do_change_column_type (mirrors the 4e column executors).
  • ForceConversion's lossy path + ClientSideNote are unchanged.

4.4 Runtime (src/runtime.rs)

  • Extend the SqlAlterTable action match: AlterColumnType { column, ty } => database.change_column_type(table, column, ty, ChangeColumnMode::ForceConversion, src).await .map(CommandOutcome::ChangeColumn) — the same outcome the simple change column uses (its event/app rendering surfaces the client-side lossy note).

4.5 app.rs failure-translate + typing_surface

  • app.rs build_translate_context: the SqlAlterTable arm's inner match action gains AlterColumnType { column, .. } => (Operation::ChangeColumnType, Some(table), Some(column)).
  • typing_surface already labels SqlAlterTable; no change (the action is internal to the variant).

4.6 Catalog (keys.rs + en-US.yaml)

  • Refresh the existing help.ddl.sql_alter_table + parse.usage.sql_alter_table bodies to add the alter table <T> alter column <col> type <type> line. No new keys.
  • Update the stale client_side // fires under --force-conversion comment to note advanced ALTER COLUMN TYPE also triggers the lossy note (comment only).

5. Phase 2 — Candidate approaches

(M1) Reuse do_change_column_type with ForceConversion (lead) — the mode already encodes the §7 advanced policy exactly; the lossy note already exists and is engine-neutral. Zero executor logic beyond the internal-table guard. (M2) Add a new ChangeColumnMode::Advanced variantrejected. It would duplicate ForceConversion's behaviour; the only conceivable difference (note wording) is already neutral and correct. No new variant earns its keep. (If a future need to distinguish "user forced" from "advanced auto" arises — e.g. for telemetry — revisit then.)

(G1) A fourth action branch leading on alter, discriminated in the builder by the type keyword (lead) — consistent with the three 4e branches; type is an unambiguous discriminator. (G2) A separate SQL_ALTER_COLUMN command/noderejected. The AlterTableAction enum is the established extension point (built for this in 4e); a separate command would fork the alter table dispatch.

6. Phase 3 — Selection vs the checklist

M1 + G1 satisfy every §3 item: parse (G1 four-branch), the §7 tiers (clean/lossy/incompatible/static-refused — all from the reused executor

  • ForceConversion), the lossy note (existing transformed_lossy), rebuild survival + undo parity (the executor's existing rebuild path), the internal-table guard (one-line add, both surfaces). No requirement unmet, no new model/persistence. Selected: M1 + G1.

7. Devil's Advocate review of this plan

  • Fork escalated? The only 4f fork — the internal-table guard on do_change_column_type — was put to the user (2026-05-25, "fix in 4f, both surfaces"). The §7 lossy policy and the no-force-flag/USING exclusions are ADR-settled, not autonomous. ✓
  • Is ForceConversion truly the §7 advanced policy? — verified in code. run_change_column_with_dry_run: incompatible cells refuse in both modes; lossy cells refuse only when mode != ForceConversion (db.rs:4575) — so under ForceConversion they are performed and counted (ClientSideNote.lossy), firing client_side.transformed_lossy (engine-neutral, no --force-conversion in the rendered text). static_refusal was read in full (the §2.3 list) — correcting the plan's first draft: int → serial is allowed, not refused. Matches the §7 intent once that nuance is captured. ✓
  • Grammar trap / discriminator? Four concrete-keyword-led branches; the builder keys on the type keyword (unique — ADD COLUMN's type is an ident). A parse test for all four branches + the alias case is on the checklist. The implementer must probe the type-keyword discriminator and the SQL_TYPE extraction (mirror build_alter_add_column_spec) — low-risk, 4e-proven patterns. ✓
  • Lossy note actually fires on the SQL path? The e2e (run_replay) lossy test asserts it; the note path is CommandOutcome::ChangeColumn → the existing app rendering, shared with simple change column. ✓
  • Undo parity? One change_column_type call = one snapshot (the executor's existing rebuild is one undo step). A per-action undo check in the e2e. ✓
  • Anything dropped? SET DATA TYPE synonym and the --force/--dont SQL spellings are deliberately out (stated, ADR-backed). The broader internal-table guard (do_add_constraint / do_add_relationship) stays a 4g follow-up — flagged, not silent. ✓
  • Help/usage refresh? The sql_alter_table skeleton is refreshed (existing node, existing keys) to list the new form — not deferred to 4i (that deferral is only for the CREATE TABLE skeleton). ✓

8. Out of 4f scope (tracked, not dropped)

  • ALTER TABLE add/drop constraint + add FK (4g) — and the internal guard on do_add_constraint / do_add_relationship rides there.
  • ALTER TABLE … RENAME TO table rename (4h).
  • SET DATA TYPE synonym; the Postgres USING <expr> clause; any SQL spelling of --force-conversion / --dont-convert (ADR §7/§12).
  • 4i verification sweep (the running deferral tail).

9. Implementation sequence (test-first)

  1. Internal-table guard (isolated, both surfaces). reject_internal_table_name at the top of do_change_column_type; a Tier-3 test via the simple change column (column_op_guards.rs) → green. Lands the latent-bug fix on its own.
  2. Command + grammar + builder. AlterColumnType variant; the AT_ALTER_COLUMN branch + the builder discriminator; Tier-1 parse + four-branch dispatch tests (probe the type discriminator first) → the exhaustive arms (app.rs) → green (parse only).
  3. Runtime + catalog. Wire AlterColumnTypechange_column_type(…, ForceConversion); refresh the sql_alter_table help/usage; Tier-3 (sql_alter_table.rs via run_replay): clean / lossy+note / incompatible / static-refused / rebuild / undo → green.
  4. Full sweepcargo test (no regression from 1854) + cargo clippy --all-targets -- -D warnings.
  5. Docs — ADR-0035 Status + §13 4f implemented; README; requirements Q1. Run /runda over the finished slice. Propose commit; wait for approval.

10. Exit gate

  • All §3 items satisfied; four tiers green, zero skips; no regression from 1854; /runda / written-DA PASS; clippy clean; ADR-0035 §13 4f + README + requirements.md lockstep.