rdbms-playground/docs/plans/20260525-adr-0035-sql-ddl-4e.md

Plan: ADR-0035 Phase 4, sub-phase 4e — ALTER TABLE add/drop/rename column

Add the advanced-only alter entry word and ALTER TABLE <T> <action> → SqlAlterTable, where <action> is ADD COLUMN <col-def>, DROP COLUMN <name>, or RENAME COLUMN <old> TO <new> (ADR-0035 §1/§2/§4 the add/drop/rename rows of the ALTER TABLE table; §13 4e). Each maps to an existing executor (do_add_column / do_drop_column / do_rename_column), like 4c/4d reused theirs. Plus the 4a.3-deferred guard: refuse dropping/renaming a column a table-level CHECK references.

Two things make this simpler than 4d:

1. No IF [NOT] EXISTS for column ALTER actions (SQLite has none; the ADR doesn't add it) — so no skip plumbing, no new outcome enums.
2. No new worker layer. The runtime decomposes SqlAlterTable into the existing db.add_column / db.drop_column / db.rename_column calls — each already snapshot_then (one undo step) and journalled.

One genuinely new thing: alter is the first advanced-only DDL entry word (CommandCategory::Advanced, like select/with) — simple mode has no alter; typing it there yields the "this is SQL" hint.

1. Baseline

• Tests: 1834 passing, 0 failing, 0 skipped, 1 ignored; clippy clean. Branch main, HEAD 701217d (4d). 4e starts here.

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

1. SQL DROP COLUMN over an index-covered column → refuse (no --cascade SQL spelling): matches raw SQLite (its native DROP COLUMN also errors on an indexed column) and the simple-mode default; the simple drop column … --cascade stays the only cascade path. Implementation: the runtime calls db.drop_column(…, cascade = false).
2. CHECK guard in the executors (both surfaces). (Finished-slice /runda, user-confirmed 2026-05-25: extended from table-level to also column-level CHECKs — a proven pre-existing rename-drift bug where a column-level CHECK, incl. a column's own self-check, would desync __rdbms_playground_columns on rename and break rebuild. The guard refuses rename when any CHECK — table- or column-level, incl. self — references the column, and refuses drop when a table-level or other column's CHECK does. Helper column_referenced_by_check.) Add the up-front refusal to do_drop_column / do_rename_column, so simple drop/rename column AND SQL ALTER TABLE both refuse a column a table CHECK references. This also fixes a latent bug: today a simple rename column of a CHECK-referenced column silently drifts __rdbms_playground_table_checks (SQLite rewrites the live CHECK; our metadata keeps the old name) and breaks a later rebuild. The refusal is deliberate (ADR §13 4e); friendly wording is H1. Consequence: a column used in a table-level CHECK can't be dropped/renamed until 4g adds drop-constraint — accepted.
3. alter is advanced-only (§2): CommandCategory::Advanced, no simple node; is_advanced_only("alter") is true → simple-mode alter gets the "this is SQL" hint. Simple mode keeps add/drop/rename/change column.
4. Require the COLUMN keyword in all three actions (ADD COLUMN / DROP COLUMN / RENAME COLUMN) — clearest pedagogically and reserves bare RENAME TO <newtable> for 4h (table rename) and ADD CONSTRAINT for 4g without grammar ambiguity. (SQLite allows ADD/RENAME without COLUMN; we standardise on the explicit form. Implementer call, flagged.) 5b. ADD COLUMN supports the full constraint set (NOT NULL / UNIQUE / DEFAULT / CHECK) — user-confirmed 2026-05-25, parity with SQL CREATE TABLE. The SQL surface stores DEFAULT/CHECK as raw text (default_sql/check_sql; sql_expr is validate-only, 4a.2), so do_add_column is extended to consume the raw text: (a) column_constraints_sql already prefers default_sql (plain path works); (b) the routing branch must send a check_sql column (and a not_null + default_sql column) to the rebuild path; (c) do_add_constrained_column_via_rebuild uses spec.check_sql / spec.default_sql when present (else the AST); (d) its pre-flight NOT-NULL/UNIQUE refusals count default_sql as a default; (e) the serial/shortid refusals count check_sql/default_sql. This mirrors the 4a.2 do_create_table raw-text mechanism.
5. ADD COLUMN carries column constraints, not inline REFERENCES. <col-def> = <name> <type> [NOT NULL] [UNIQUE] [DEFAULT …] [CHECK …] (reusing the create-table COLUMN_DEF grammar + do_add_column's native-vs-rebuild routing). Inline REFERENCES / table constraints are 4g (add FK / add constraint), matching the simple add column boundary (no inline FK there either).

3. Phase 1 — Requirements checklist (4e)

Grammar / dispatch:

• alter parses only in advanced mode; simple-mode alter … → "this is SQL" hint (advanced-only entry word).
• ALTER TABLE <T> ADD COLUMN <name> <type> [constraints] → SqlAlterTable { AddColumn(ColumnSpec) } (constraints: NOT NULL / UNIQUE / DEFAULT / CHECK, reusing the create-table column-def grammar).
• ALTER TABLE <T> DROP COLUMN <name> → … DropColumn.
• ALTER TABLE <T> RENAME COLUMN <old> TO <new> → … RenameColumn.
• Trailing ; tolerated; the <T> slot completes from the schema cache (IdentSource::Tables) and rejects internal __rdbms_* (reuse the SQL-family reject_internal_table validator on the table slot).

Execution (reuse existing executors):

• ADD COLUMN: plain + each constraint (NOT NULL+default, UNIQUE, CHECK) lands via do_add_column (native or rebuild as it already decides); one undo step; auto-show.
• DROP COLUMN: removes the column (one undo step); refuses PK / FK / index-covered (no cascade) / table-CHECK-referenced columns.
• RENAME COLUMN: renames (one undo step), metadata mirrored; refuses same-name / existing-target / table-CHECK-referenced columns.
• CHECK guard, both surfaces: simple drop column / rename column of a table-CHECK-referenced column is also refused now (was a raw engine error / silent rename-drift); a regression-style test proves the simple surface and a rebuild-after still works.
• Errors engine-neutral (inline Unsupported, matching the existing PK/FK/index refusals).
• Internal-table guard (both surfaces): do_add_column / do_drop_column / do_rename_column refuse an internal __rdbms_* table (as "no such table"); the SQL ALTER table slot also carries the parse-time reject_internal_table validator. Tested on both surfaces.

Testing

• Tier 1 (in-crate sql_alter_table_tests in ddl.rs): the three actions parse → the right SqlAlterTable action; ADD COLUMN constraints captured; alter is advanced-only (simple-mode parse is not a SqlAlterTable); table slot rejects __rdbms_*.
• Tier 3 (tests/sql_alter_table.rs): each action end-to-end via SqlAlterTable; the four DROP refusals (PK/FK/index/CHECK); the RENAME CHECK refusal; one-undo-step for each; the CHECK guard on the simple surface (simple rename column/drop column refused) + a rebuild-survives check on a table that does carry a CHECK on an un-renamed column.
• Unit (check_references_column): tokenizer detects a bare identifier, is case-insensitive, ignores a same-spelling substring inside a string literal and inside a longer identifier.
• Catalog lockstep + vocab audit for the new help/usage keys.

4. Architecture & design

4.1 Command (src/dsl/command.rs)

• SqlAlterTable { table: String, action: AlterTableAction }.
• pub enum AlterTableAction { AddColumn(ColumnSpec), DropColumn { column: String }, RenameColumn { old: String, new: String } } (4f/4g/4h extend it: AlterColumnType, AddConstraint/AddForeignKey/ DropConstraint, RenameTo).
• verb() → "alter table"; target_table() → table.

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

• alter entry word, advanced-only. Shape: ALTER is the entry; the shape after it is TABLE <table> <action> [;].
  • Reuse a table-name slot with reject_internal_table (a dedicated node like the SQL create-table TABLE_NAME, not the validator-less TABLE_NAME_EXISTING).
  • <action> = Choice[ AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN ], each branch leading on a concrete keyword (add/drop/rename) — trap-safe.
    • AT_ADD_COLUMN = Seq[ Word(add), Word(column), <col name (NewName)>, <SQL_TYPE>, <narrow-constraint-suffix> ]. Do NOT reuse COLUMN_DEF wholesale — its COL_CONSTRAINT Choice admits PRIMARY KEY (invalid on ADD COLUMN — can't add a PK to an existing table) and inline REFERENCES (4g; do_add_column would silently drop the FK). Compose a narrow constraint suffix that reuses only the leaf nodes NOT NULL / UNIQUE / DEFAULT / CHECK (the ADR-0029 set the simple add column supports), excluding PK and REFERENCES. SQL_TYPE + the four leaf constraint nodes get pub(crate)-exported from sql_create_table.rs. Typing ADD COLUMN id int PRIMARY KEY is then an ordinary parse error (unsupported constraint here) — acceptable; H1 can soften it.
    • AT_DROP_COLUMN = Seq[ Word(drop), Word(column), <col name> ] (existing-column ident).
    • AT_RENAME_COLUMN = Seq[ Word(rename), Word(column), <old>, Word(to), <new-name> ] (<old> existing-column ident; <new> NewName).
• pub static SQL_ALTER_TABLE: CommandNode { entry: "alter", shape, ast_builder: build_sql_alter_table, help_id, usage_ids }.
• REGISTRY: (&ddl::SQL_ALTER_TABLE, CommandCategory::Advanced).
• build_sql_alter_table: branch on the leading action keyword (add/drop/rename — the first Word item after the table name). For ADD COLUMN, build the single ColumnSpec by reusing collect_column_constraints(path) (the same helper the simple add column builder uses for (not_null, unique, default, check)) plus the col_name/type idents — not the create-table multi-column loop. For DROP/RENAME, pull the column idents (require_ident).

4.3 Worker / executors (src/db.rs) — only the CHECK guard is new

• No new Request / Database method. The runtime decomposes the action (4.4).
• New helper check_references_column(check_expr: &str, column: &str) -> bool: tokenize the raw CHECK text with the lex_helpers (consume_string_literal to skip string contents, consume_ident to collect identifiers), case-insensitive compare to column. Robust against same-spelling substrings in literals / longer identifiers.
• do_drop_column: after the PK / FK / index guards, add — for each read_table_checks(conn, table) expr — if check_references_column, refuse with an engine-neutral Unsupported ("cannot drop T.c while a table CHECK references it; drop the constraint first"). Wording is terse/engine-neutral (H1 polishes).
• do_rename_column: same guard on old (a rename would drift the CHECK metadata; refuse).

4.4 Runtime (src/runtime.rs)

Command::SqlAlterTable { table, action } → match action:

• AddColumn(spec) → db.add_column(table, spec, src) → CommandOutcome::AddColumn(_) (the existing add-column outcome path).
• DropColumn { column } → db.drop_column(table, column, false, src) → CommandOutcome::DropColumn(_) (cascade = false per decision 1).
• RenameColumn { old, new } → db.rename_column(table, old, new, src) → CommandOutcome::Schema(Some(d)). No new CommandOutcome variants; reuse the simple-mode ones (the auto-show / result rendering is identical — same executors).

4.5 app.rs failure-translate + typing_surface

• app.rs build_translate_context: C::SqlAlterTable { table, action } → route by action to the matching Operation (AddColumn / DropColumn / RenameColumn) with table (and the column where the action carries one).
• tests/typing_surface/mod.rs: SqlAlterTable { .. } => "SqlAlterTable".

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

• help.ddl.sql_alter_table + parse.usage.sql_alter_table (new node). Usage: alter table <T> add column <col> <type> [constraints] | drop column <col> | rename column <old> to <new>. Engine-neutral.
• No new note key — the CHECK-guard refusals are inline Unsupported strings like the existing drop-column refusals.

5. Phase 2 — Candidate approaches

(A1) SqlAlterTable + an AlterTableAction enum, runtime-decomposed to existing db methods (lead). No new worker layer; extends cleanly for 4f/4g/4h (more action variants). (A2) A new db.sql_alter_table worker method that dispatches inside the worker. Rejected — adds a Request/method that just re-calls the existing executors; the runtime-decompose (A1) reuses the shipped public methods (already snapshot/journal-correct) with less surface.

(G1) ADD COLUMN reuses the create-table COLUMN_DEF node (lead) — one column-def grammar, one constraint set, no drift from create-table. (G2) A bespoke ALTER-ADD column-def grammar. Rejected — duplicates the column-def + constraint grammar; risks divergence (the "two DDL generators stay in sync" lesson, here for the parser).

(C1) CHECK-ref detection via the lex_helpers tokenizer (lead) — skips string literals, matches whole identifiers; robust + reuses tested primitives. (C2) A \bcol\b regex / contains. Rejected — false positives inside string literals and longer identifiers would wrongly refuse a legitimate drop. (C3) Parse via sql_expr and walk an AST. Rejected — sql_expr is validate-only (no Expr AST, the 4a.2 finding); heavier with no payoff over C1.

6. Phase 3 — Selection vs the checklist

A1 + G1 + C1 satisfy every §3 item: the three actions parse (G1 + concrete-keyword Choice), dispatch (advanced-only alter), execute (reuse executors via A1), the four drop refusals + rename refusal (the existing guards + the new C1 CHECK guard in the shared executors → both surfaces), engine-neutral wording, undo parity (one executor call = one snapshot). No requirement unmet. Selected: A1 + G1 + C1.

7. Devil's Advocate review of this plan

• Both forks escalated + user-confirmed? Yes (2026-05-25): SQL drop refuses index-covered (no cascade spelling); the CHECK guard lives in the executors (both surfaces, refuse). No autonomous scope calls. ✓
• Reuse vs fork? Executors reused wholesale (A1); only the CHECK guard is added, and it's added once in the shared executors — fixing both surfaces and a latent rename-drift bug. ✓
• Grammar trap? The action Choice branches each lead on a concrete keyword (add/drop/rename); requiring COLUMN keeps them unambiguous and reserves RENAME TO/ADD CONSTRAINT for 4h/4g. The COLUMN_DEF reuse needs a pub(crate) export — verify it doesn't pull in create-table-only context. Probe the dispatch (alter advanced-only → simple-mode hint; the three actions each to the right command). ✓
• CHECK detection robustness? C1 tokenizer + a unit test with a string-literal false-positive case and a longer-identifier case. The detection is the only subtle logic; it gets dedicated unit coverage. ✓
• Latent simple-mode bug actually fixed + proven? A Tier-3 test renames/drops a CHECK-referenced column via the simple command and asserts the refusal, plus a rebuild-survives check. ✓
• Undo parity? Each action is exactly one existing executor call = one snapshot. A per-action undo test. ✓
• Anything silently dropped? IF [NOT] EXISTS for column ALTER is out of scope (SQLite has none; ADR doesn't add it) — stated, not silent. RENAME TO (table) is 4h; ADD CONSTRAINT/ADD FK / ALTER COLUMN TYPE are 4g/4f — the AlterTableAction enum is built to extend. The COLUMN-keyword-required choice is flagged (decision 4). ✓
• Help/usage skeleton? New node → its keys land now (not deferred, unlike the 4i CREATE TABLE refresh). ✓
• Internal-__rdbms_*-table exposure on the column executors — OPEN, escalated. Verified: do_add_column/do_drop_column/do_rename_column call read_schema, which succeeds for internal tables; the simple add/drop/rename column grammar uses the validator-less TABLE_NAME_EXISTING. So drop column from __rdbms_playground_columns: table_name (simple mode) parses and corrupts internal metadata — a pre-existing exposure (ADR-0013 era), more severe than 4d's phantom index, though low-likelihood (the user must type a name hidden everywhere). 4e's SQL ALTER gets a parse-time reject_internal_table guard either way; the question is whether to also close the simple surface at the executor (consistent with 4d's both-surfaces fix). RESOLVED (user, 2026-05-25): fix the executors, both surfaces — add an internal-table guard (refuse as "no such table") to do_add_column / do_drop_column / do_rename_column. The broader latent class (do_change_column_type / do_add_constraint / do_add_relationship) is flagged as a separate follow-up, not touched in 4e.

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

• ALTER COLUMN TYPE (4f), ADD/DROP CONSTRAINT + ADD FK (4g), RENAME TO table rename (4h).
• IF [NOT] EXISTS on column ALTER actions (no SQLite support; not in ADR §4) — revisit only if a future ADR adds it.
• COLUMN-keyword-optional forms (ADD c int, RENAME a TO b) — could be admitted later; 4e requires the explicit COLUMN keyword.
• Friendly wording for the CHECK-guard refusal — H1 (4e gives a terse engine-neutral message).
• Rewriting a table-CHECK's text on rename (instead of refusing) — a future enhancement; 4e refuses per ADR §13 4e.

9. Implementation sequence (test-first)

1. Executor guards (isolated, both surfaces first). (a) Unit-test check_references_column (red → impl); add the table-CHECK guard to do_drop_column / do_rename_column. (b) Add the internal-__rdbms_* guard to do_add_column / do_drop_column / do_rename_column. Tier-3 tests via the simple commands (CHECK drop+rename refusal + rebuild-survives; internal-table refusal on all three) → green. Lands both latent-bug fixes on their own, reviewable, before any SQL surface.
2. Command + grammar + alter entry word + builder. Tier-1 parse + advanced-only dispatch tests → red → add SqlAlterTable / AlterTableAction, the grammar (export COLUMN_DEF), REGISTRY entry, the exhaustive-match arms (verb / target_table / app.rs / typing surface), catalog keys → green (parse only).
3. Runtime dispatch. Wire SqlAlterTable → the three existing db methods; Tier-3 (tests/sql_alter_table.rs): each action end-to-end + the four drop refusals + rename refusal + per-action undo → green.
4. Full sweep — cargo test (no regression from 1834) + cargo clippy --all-targets -- -D warnings.
5. Docs — ADR-0035 Status + §13 4e implemented; README; requirements Q1. Run /runda. Propose commit; wait for approval.

10. Exit gate

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