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.