# Plan: ADR-0035 Phase 4, sub-phase 4e — `ALTER TABLE` add/drop/rename column Add the advanced-only `alter` entry word and `ALTER TABLE ` → `SqlAlterTable`, where `` is `ADD COLUMN `, `DROP COLUMN `, or `RENAME COLUMN TO ` (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 ` 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`.** `` = ` [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 ADD COLUMN [constraints]` → `SqlAlterTable { AddColumn(ColumnSpec) }` (constraints: NOT NULL / UNIQUE / DEFAULT / CHECK, reusing the create-table column-def grammar). - [ ] `ALTER TABLE DROP COLUMN ` → `… DropColumn`. - [ ] `ALTER TABLE RENAME COLUMN TO ` → `… RenameColumn`. - [ ] Trailing `;` tolerated; the `` 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 [;]`. - 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`). - `` = `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), , , ]`. **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), ]` (existing-column ident). - `AT_RENAME_COLUMN = Seq[ Word(rename), Word(column), , Word(to), ]` (`` existing-column ident; `` `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 add column [constraints] | drop column | rename column to `. 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.