diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 8b91116..d2e62c3 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -3,14 +3,15 @@ ## Status Accepted. Design agreed with the user (2026-05-24); the approach is -**validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c / 4d** -(`CREATE TABLE` with column- and table-level constraints and foreign -keys, `DROP TABLE [IF EXISTS]`, and `CREATE [UNIQUE] INDEX` / -`DROP INDEX [IF EXISTS]`, implemented 2026-05-25 — plans +**validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c / 4d / +4e** (`CREATE TABLE` with column- and table-level constraints and foreign +keys, `DROP TABLE [IF EXISTS]`, `CREATE [UNIQUE] INDEX` / +`DROP INDEX [IF EXISTS]`, and `ALTER TABLE` add/drop/rename column, +implemented 2026-05-25 — plans `docs/plans/20260524-adr-0035-sql-ddl-4a.md`, `…-4a2.md`, `…-4a3.md`, -`docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`, `…-4d.md`), so -the decision is accepted while the remaining sub-phases (**4e–4i**, §13) -continue. This is **Phase 4** of the ADR-0030 roadmap (the +`docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`, `…-4d.md`, +`…-4e.md`), so the decision is accepted while the remaining sub-phases +(**4f–4i**, §13) continue. This is **Phase 4** of the ADR-0030 roadmap (the advanced-mode SQL surface), the peer of ADR-0031 (expression grammar), ADR-0032 (`SELECT`), and ADR-0033 (DML). It **clarifies ADR-0030 §4** on how DDL is represented and executed. @@ -401,13 +402,39 @@ ADR-0033's structure: Simple-mode `add unique index` stays deferred. `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch (`decide` tries every advanced candidate). -- **4e — `ALTER TABLE` add/drop/rename column.** Drop/rename column - must guard against a **table-level CHECK that references the column** - (4a.3): today the rebuild rejects it cleanly via the engine (the - rebuilt table's `CHECK` names a missing column), leaving the table - intact — 4e adds the up-front detection (parsing column references - out of the raw CHECK text in `__rdbms_playground_table_checks`) so the - refusal is deliberate; the friendly wording itself is **H1** work. +- **4e — `ALTER TABLE` add/drop/rename column.** *(Implemented + 2026-05-25 — plan `docs/plans/20260525-adr-0035-sql-ddl-4e.md`.)* + `alter` is a new advanced-**only** entry word (like `select`/`with`); + `ALTER TABLE ADD COLUMN [NOT NULL|UNIQUE|DEFAULT| + CHECK] | DROP COLUMN | RENAME COLUMN TO ` → + `SqlAlterTable { AlterTableAction }`, **runtime-decomposed** to the + existing `do_add_column` / `do_drop_column` / `do_rename_column` (one + undo step each) — no new worker layer. The `COLUMN` keyword is + required (reserves bare `RENAME TO` for 4h, `ADD CONSTRAINT` for 4g); + ADD COLUMN takes column constraints only (no PK / inline REFERENCES). + **`do_add_column` was extended** to consume the SQL raw-text + `default_sql` / `check_sql` (DEFAULT/CHECK; `sql_expr` is validate-only + — the 4a.2 mechanism), so ADD COLUMN reaches parity with `CREATE + TABLE`'s column constraints. Drop/rename column now **refuse a column a + CHECK references** — the 4a.3 deferral, extended (user-confirmed) to + **both table-level and column-level CHECKs** — detected up-front by + tokenizing the raw CHECK text (`check_references_column`, skipping + string literals); for RENAME the column's *own* column-level CHECK + counts (it drifts too), for DROP it does not (it drops with the + column). This lives in the shared executors, so it guards **both** the + simple `drop/rename column` and the SQL surface, fixing a latent + rename-drift bug (a native rename rewrites the live CHECK but leaves the + stored text — table-level in `__rdbms_playground_table_checks` or + column-level in `__rdbms_playground_columns` — stale, breaking a later + rebuild). SQL + `DROP COLUMN` over an index-covered column is **refused** (no + `--cascade` SQL spelling — matches SQLite + the simple default; + user-confirmed). The shared column executors (and `do_add_index`) also + gained an internal-`__rdbms_*`-table guard (refuse as "no such table"), + closing a pre-existing exposure on both surfaces (user-confirmed). The + friendly wording of the CHECK-guard refusal is **H1**. *(The broader + internal-table guard on `do_change_column_type` / `do_add_constraint` / + `do_add_relationship` is a tracked follow-up.)* - **4f — `ALTER TABLE … ALTER COLUMN TYPE`** (the §7 conversion model + the lossy-with-note path). - **4g — `ALTER TABLE` add/drop constraint, add foreign key.** diff --git a/docs/adr/README.md b/docs/adr/README.md index eb9cb6f..36c9157 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -40,4 +40,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses - [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Accepted** (implemented + verified through sub-phase 3k, 2026-05-23; phase-exit report `docs/handoff/20260523-phase-3-verification.md`), the Phase-3 grammar commissioned by ADR-0030 §3: single- and multi-row `INSERT` (incl. `INSERT … SELECT` recursing through ADR-0032's `SQL_SELECT_COMPOUND`), `UPDATE` with `SET` assignment list, `DELETE`, all three optionally followed by `RETURNING projection_list`, plus full `ON CONFLICT … DO NOTHING / DO UPDATE` UPSERT on INSERT; **fixes the DSL-vs-SQL dispatch architecture for shared entry words (`insert`/`update`/`delete`)**: SQL-first / DSL-fallback in Advanced mode via a `Choice(SQL_shape, DSL_shape)` per shape, gated by a new walker capability `Node::Guard(fn)` — a zero-byte-consumption gating node that fails the enclosing Seq with a `ValidationError`; carries `Command::SqlInsert` / `SqlUpdate` / `SqlDelete` variants and `do_sql_*` worker handlers each of which knows the target table (for re-persistence) and the `returning: bool` flag (for DataResult routing); `shortid` auto-fill mirrors the DSL `do_insert` mechanism via worker post-fill; SQL DELETE produces the same per-relationship cascade summary the DSL DELETE does (ADR-0014 parity); three new walker diagnostics (`insert_arity_mismatch` ERROR, `auto_column_overridden` WARNING, `not_null_missing` WARNING) with positive + negative tests each; OOS list explicitly carves out `DEFAULT VALUES` (the project's planned seed feature), SQLite-specific `OR REPLACE` / `OR IGNORE` / `OR ABORT` / `OR FAIL` / `OR ROLLBACK` prefixes, `UPDATE FROM` multi-table updates, and WITH-prefixed DML; the `excluded` keyword inside `ON CONFLICT DO UPDATE` is a deliberate carve-out from ADR-0030 §7's engine-neutral posture (no standard-SQL UPSERT spelling exists that SQLite and PostgreSQL share); eleven phased sub-phases each with explicit exit gates + written DA gate, opening with the dispatch mechanism before any DML grammar lands; initial DA review recorded seven critiques that were resolved before status moved to Proposed; **Amendment 1 supersedes §2's dispatch mechanism**: the originally-chosen `Node::Guard(fn)` + `Choice(SQL_shape, DSL_shape)` was found during 3a to be unworkable as framed (any guard-in-`Choice` mechanism forces a `walk_choice` change — `walk_choice` only falls through on `NoMatch`, so Simple-mode valid-DSL would wrongly surface "this is SQL", and `walk_seq` treats a `NoMatch` past `idx 0` as a hard `Failed`, breaking Advanced-mode DSL fall-through); replaced by **category-grouped, mode-aware dispatch** in `walker::walk` (each `REGISTRY` entry tagged `CommandCategory::{Simple, Advanced}`, generalising the existing whole-command `is_advanced_only` gate), shared entry words carrying a node in both groups, no `Node::Guard` and no `walk_choice`/`walk_seq` change, advanced-mode completion SQL-first with DSL as a full-line fallback; **Amendment 2 (sub-phase 3f) supersedes §7's cascade mechanism**: the WHERE-injected per-child pre-count rested on a premise that was factually wrong about the DSL handler (which detects cascades by before/after row-count diffing inside a transaction, not by `Expr`-derived pre-count subqueries) and would have broken the §2 parity promise by reporting `SET NULL` the DSL path doesn't; replaced by mirroring `do_delete`'s count-diff exactly (verbatim DELETE executes, child-count diff observes the cascade — `ON DELETE CASCADE` row removals only, SET NULL deferred for both paths to preserve parity), which shares the render-layer formatter for free via `CommandOutcome::Delete` and **withdraws risk R2** (no WHERE-byte extraction, no N+1 subquery); **Amendment 3 (sub-phase 3j) records the command-identity model and defers the execution-mode side-channel**: a command is the typed outcome of a *mode-rooted* grammar path and its identity is intrinsic (Advanced mode tries SQL first, falls back to the *Simple* DSL command when no SQL branch matches a token, e.g. `delete … --all-rows`; note `update … --all-rows` does *not* fall back — the SQL `SET` expression eats `--all-rows`, harmless since the engine treats it as a comment); **Simple mode commits the DSL candidate for shared words** so the *real* DSL error surfaces, and when that line would also run in advanced mode the rendering layer **combines** them — DSL error **plus** an `advanced_mode.also_valid_sql` pointer ("… (valid as SQL in advanced mode)") — keeping the actionable DSL fix while pointing at advanced mode; bare "this is SQL" is reserved for entry words with no DSL form (`select`/`with`); a fully-overlapping input (`insert … values …`) legitimately yields *two distinct commands* (`Command::Insert` typed-AST vs `Command::SqlInsert` validated-text) that do the same thing but execute differently (ADR-0030 §4), so each is tested in the mode that produces it; **corrects the plan's 3j exit-gate premise** that the DSL DML tests run in Simple mode (they call `parse_command`, which defaults to Advanced) — the real invariant is "Simple-mode behaviour unchanged, Advanced mode SQL-first, DSL grammar tested in Simple mode, both variants tested in their producing mode", with §6/§7 parity keeping the paths observably equivalent; and **defers to its own future ADR** the execution-time mode side-channel (three-way `Mode`: simple/advanced/advanced-one-shot threaded through `Action`→worker, for mode-dependent *output* like echoing generated SQL) — today only the *rendering* side-channel `OutputLine.mode_at_submission` exists, and the three-way distinction is not required for Phase 3 dispatch correctness - [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration; **implemented 2026-05-24** (plan `docs/plans/20260524-adr-0034-history-journal.md`); **Amendment 1 (2026-05-24): replay filters out app-lifecycle commands** — a working `replay history.log` (the §3 fix) exposed that the journal also records `save as`/`load`/`new`/`export`/`import`/`rebuild`/`mode` (which would panic the worker dispatch or abort the replay), so replay now re-applies **only** schema/data write commands and **skips** every `Command::App` + nested `Command::Replay`; **all skips continue** (never abort — reversing the prior nested-`replay` refusal, so a journal containing a once-run `replay` needs no hand-editing, and the infinite-loop footgun is closed by construction), with a `[skip]` **warning** on `import` and nested-`replay` skips (their omission can leave replayed state incomplete) and silent skips for the rest; `replay.error_nested` removed, `replay.skipped_import`/`replay.skipped_replay` added, `ReplayCompleted` carries `warnings` -- [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]`, implemented 2026-05-25; 4e–4i pending), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch); the remaining DDL forms stay "not yet supported" until their sub-phases land. Each sub-phase has exit + DA gates +- [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` + 4e `ALTER TABLE` add/drop/rename column, implemented 2026-05-25; 4f–4i pending), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch) and **4e** (`ALTER TABLE` add/drop/rename column → `SqlAlterTable`; `alter` is a new advanced-**only** entry word, runtime-decomposed to the existing `do_add_column`/`do_drop_column`/`do_rename_column` — no new worker layer; `do_add_column` extended to consume raw `default_sql`/`check_sql` so ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a column any CHECK references (table-level AND column-level, incl. a column's own self-check on rename) — the 4a.3 deferral, via a raw-CHECK-text tokenizer in the shared executors, so it guards both surfaces and fixes a latent rename-drift bug; SQL DROP COLUMN refuses an index-covered column with no `--cascade` spelling; the column executors + `do_add_index` gained an internal-`__rdbms_*`-table guard — all user-confirmed); the remaining DDL forms stay "not yet supported" until their sub-phases land. Each sub-phase has exit + DA gates diff --git a/docs/plans/20260525-adr-0035-sql-ddl-4e.md b/docs/plans/20260525-adr-0035-sql-ddl-4e.md new file mode 100644 index 0000000..d4833f6 --- /dev/null +++ b/docs/plans/20260525-adr-0035-sql-ddl-4e.md @@ -0,0 +1,335 @@ +# 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. diff --git a/docs/requirements.md b/docs/requirements.md index fd7eec7..6747458 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -230,8 +230,13 @@ handoff-14 cleanup; 449 after B2/C2.) (4c — reuses `do_drop_table`; `IF EXISTS` is a no-op-with-note), then `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` (4d — reuse `do_add_index`/`do_drop_index`; `CREATE UNIQUE INDEX` admitted in - advanced mode via the `IndexSchema.unique` flag, ADR-0025 Amendment 1)). - Remaining DDL — `ALTER TABLE` (4e–4h) — is phased per ADR-0035 §13.)* + advanced mode via the `IndexSchema.unique` flag, ADR-0025 Amendment 1), + then `ALTER TABLE` add/drop/rename column (4e — `alter` is advanced-only, + runtime-decomposed to the existing column executors; ADD COLUMN reaches + CREATE-TABLE constraint parity; drop/rename refuse a table-CHECK- + referenced column)). + Remaining DDL — `ALTER TABLE … ALTER COLUMN TYPE` / add-drop-constraint / + add-FK / `RENAME TO` (4f–4h) — is phased per ADR-0035 §13.)* - [ ] **Q2** Non-standard syntax rejected with a clear message pointing at the supported subset. *(Design done — ADR-0030 §8: out-of-subset statements are diff --git a/src/app.rs b/src/app.rs index 5604c3b..a024984 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1574,10 +1574,30 @@ impl App { command: &Command, facts: crate::friendly::FailureContext, ) -> crate::friendly::TranslateContext { - use crate::dsl::{Command as C, IndexSelector, RelationshipSelector}; + use crate::dsl::{AlterTableAction, Command as C, IndexSelector, RelationshipSelector}; use crate::friendly::{Operation, TranslateContext}; let (operation, fallback_table, fallback_column) = match command { C::CreateTable { name, .. } => (Operation::CreateTable, Some(name.as_str()), None), + // SQL `ALTER TABLE` routes engine/validation errors through + // the operation matching its action, with the parsed table + // (and column, where the action names one) — ADR-0035 §4e. + C::SqlAlterTable { table, action } => match action { + AlterTableAction::AddColumn(spec) => ( + Operation::AddColumn, + Some(table.as_str()), + Some(spec.name.as_str()), + ), + AlterTableAction::DropColumn { column } => ( + Operation::DropColumn, + Some(table.as_str()), + Some(column.as_str()), + ), + AlterTableAction::RenameColumn { old, .. } => ( + Operation::RenameColumn, + Some(table.as_str()), + Some(old.as_str()), + ), + }, C::SqlCreateTable { name, .. } => { (Operation::CreateTable, Some(name.as_str()), None) } diff --git a/src/db.rs b/src/db.rs index 1f6c658..38d27d1 100644 --- a/src/db.rs +++ b/src/db.rs @@ -3132,10 +3132,12 @@ fn do_add_column( table: &str, column: &ColumnSpec, ) -> Result { + reject_internal_table_name(table)?; if matches!(column.ty, Type::Serial | Type::ShortId) { // ADR-0029 §6: a `serial` / `shortid` column auto-fills // its own values, so a separate `default` is ambiguous. - if column.default.is_some() { + // (`default_sql` is the advanced-mode raw form — ADR-0035 §4e.) + if column.default.is_some() || column.default_sql.is_some() { return Err(DbError::Unsupported(format!( "`{name}` is a {ty} column — it auto-fills its own values, \ so it cannot also carry a `default`.", @@ -3146,8 +3148,9 @@ fn do_add_column( // A CHECK on an auto-generated column is supported at // `create table` time; adding one to a `serial` / // `shortid` column afterwards is not (the auto-fill - // rebuild path does not thread it). - if column.check.is_some() { + // rebuild path does not thread it). `check_sql` is the + // advanced-mode raw form. + if column.check.is_some() || column.check_sql.is_some() { return Err(DbError::Unsupported(format!( "a `check` constraint on the auto-generated column `{}` \ can only be set when the table is created.", @@ -3158,9 +3161,14 @@ fn do_add_column( } // SQLite's `ALTER TABLE ADD COLUMN` cannot express `UNIQUE` // or `CHECK`, and a `NOT NULL` column added that way must - // carry a default — all route through the rebuild - // primitive instead (ADR-0029 §6). - if column.unique || column.check.is_some() || (column.not_null && column.default.is_none()) + // carry a default — all route through the rebuild primitive + // instead (ADR-0029 §6). The advanced-mode raw forms + // (`check_sql` / `default_sql`, ADR-0035 §4e) count alongside + // the typed AST forms. + if column.unique + || column.check.is_some() + || column.check_sql.is_some() + || (column.not_null && column.default.is_none() && column.default_sql.is_none()) { do_add_constrained_column_via_rebuild(conn, persistence, source, table, column) } else { @@ -3370,7 +3378,12 @@ fn do_add_constrained_column_via_rebuild( // ADR-0029 §6 pre-flight refusals — caught before any SQL // write, surfaced as friendly messages. - if spec.not_null && spec.default.is_none() && row_count > 0 { + // A default may come from the typed AST (`default`, simple mode) or + // the raw advanced-mode text (`default_sql`, ADR-0035 §4e); either + // satisfies the NOT-NULL backfill and triggers the UNIQUE collision + // guard. + let has_default = spec.default.is_some() || spec.default_sql.is_some(); + if spec.not_null && !has_default && row_count > 0 { return Err(DbError::Unsupported(format!( "adding the NOT NULL column `{}` to `{table}`, which already \ has {row_count} row(s), needs a `default` — every existing \ @@ -3378,7 +3391,7 @@ fn do_add_constrained_column_via_rebuild( spec.name, ))); } - if spec.unique && spec.default.is_some() && row_count > 1 { + if spec.unique && has_default && row_count > 1 { return Err(DbError::Unsupported(format!( "adding the UNIQUE column `{}` with a default to `{table}` \ would give all {row_count} existing rows the same value, \ @@ -3387,6 +3400,12 @@ fn do_add_constrained_column_via_rebuild( ))); } + // The DEFAULT literal: the raw advanced-mode text wins over the typed + // form (ADR-0035 §4e, mirroring `column_constraints_sql`). + let default_sql = match &spec.default_sql { + Some(raw) => Some(raw.clone()), + None => default_sql_literal(spec)?, + }; // Append the new column to the schema; the rebuild's // column-by-name copy leaves it at its DEFAULT (or NULL). let mut new_schema = old_schema.clone(); @@ -3396,16 +3415,17 @@ fn do_add_constrained_column_via_rebuild( notnull: spec.not_null, primary_key: false, unique: spec.unique, - default_sql: default_sql_literal(spec)?, + default_sql, check: None, user_type: Some(spec.ty), }); - // The CHECK is compiled against the post-add schema, so it - // may reference the new column itself. + // The CHECK: the raw advanced-mode text (`check_sql`) wins; otherwise + // the typed `check` AST is compiled against the post-add schema (so + // it may reference the new column itself). let check_sql = spec - .check - .as_ref() - .map(|e| compile_check_sql(e, &new_schema)); + .check_sql + .clone() + .or_else(|| spec.check.as_ref().map(|e| compile_check_sql(e, &new_schema))); if let Some(last) = new_schema.columns.last_mut() { last.check.clone_from(&check_sql); } @@ -4006,6 +4026,7 @@ fn do_drop_column( column: &str, cascade: bool, ) -> Result { + reject_internal_table_name(table)?; let schema = read_schema(conn, table)?; let col_info = schema .columns @@ -4059,6 +4080,19 @@ fn do_drop_column( ))); } + // A CHECK (table-level, or a *different* column's column-level CHECK) + // that references this column (ADR-0035 §4e, the 4a.3 deferral): a + // deliberate up-front refusal — dropping the column would break that + // CHECK and the rebuilt DDL would name a missing column. The column's + // own column-level CHECK drops with it, so it does not block. + // Friendly wording is H1. Guards both surfaces. + if column_referenced_by_check(conn, table, &schema, column, false)? { + return Err(DbError::Unsupported(format!( + "cannot drop `{table}.{column}` while a CHECK references it; \ + drop the constraint first." + ))); + } + let tx = conn .unchecked_transaction() .map_err(DbError::from_rusqlite)?; @@ -4115,6 +4149,7 @@ fn do_rename_column( old: &str, new: &str, ) -> Result { + reject_internal_table_name(table)?; let schema = read_schema(conn, table)?; if !schema.columns.iter().any(|c| c.name == old) { return Err(DbError::Sqlite { @@ -4122,6 +4157,19 @@ fn do_rename_column( kind: SqliteErrorKind::NoSuchColumn, }); } + // A CHECK that references this column (ADR-0035 §4e): refuse — a + // native RENAME COLUMN rewrites the live CHECK but the stored CHECK + // text (table-level in `__rdbms_playground_table_checks`, or + // column-level in `__rdbms_playground_columns`) keeps the old name, + // drifting the metadata and breaking a later rebuild. A column's + // *own* column-level CHECK drifts too (`include_self = true`). + // Deliberate refusal (friendly wording is H1); guards both surfaces. + if column_referenced_by_check(conn, table, &schema, old, true)? { + return Err(DbError::Unsupported(format!( + "cannot rename `{table}.{old}` while a CHECK references it; \ + drop the constraint first." + ))); + } if old == new { // Nothing to do; refusing keeps behaviour // predictable rather than appearing to "succeed" @@ -5213,6 +5261,107 @@ fn read_table_checks(conn: &Connection, table: &str) -> Result, DbEr Ok(out) } +/// Whether the raw CHECK expression `check_expr` references the column +/// `column` (ADR-0035 §4e — the 4a.3-deferred drop/rename guard). +/// +/// Tokenizes the expression with the shared lex helpers, **skipping +/// single-quoted string literals** so an identifier that only appears +/// inside a literal does not false-match, and compares each bare +/// identifier case-insensitively. Playground column names are always +/// valid bare identifiers (`validate_user_name` rejects the characters +/// that would force quoting), so bare-identifier scanning is sufficient; +/// the engine's native `DROP`/`RENAME COLUMN` remains the backstop for +/// any miss. +fn check_references_column(check_expr: &str, column: &str) -> bool { + use crate::dsl::walker::lex_helpers::{consume_ident, consume_string_literal, skip_whitespace}; + let mut i = 0; + while i < check_expr.len() { + i = skip_whitespace(check_expr, i); + if i >= check_expr.len() { + break; + } + if let Some(((_, end), _)) = consume_string_literal(check_expr, i) { + i = end; + } else if let Some((start, end)) = consume_ident(check_expr, i) { + if check_expr[start..end].eq_ignore_ascii_case(column) { + return true; + } + i = end; + } else { + // Operator / paren / number / punctuation — advance one char. + i += check_expr[i..].chars().next().map_or(1, char::len_utf8); + } + } + false +} + +#[cfg(test)] +mod check_references_column_tests { + use super::check_references_column; + + #[test] + fn detects_a_bare_identifier() { + assert!(check_references_column("price > 0", "price")); + assert!(check_references_column("a < b AND b < c", "b")); + } + + #[test] + fn is_case_insensitive() { + assert!(check_references_column("Price > 0", "price")); + assert!(check_references_column("price > 0", "PRICE")); + } + + #[test] + fn ignores_identifier_inside_a_string_literal() { + // `status` appears only inside a literal → not a reference. + assert!(!check_references_column("kind <> 'status'", "status")); + // A genuine reference alongside a literal is still found. + assert!(check_references_column("status <> 'archived'", "status")); + } + + #[test] + fn ignores_a_longer_identifier_that_merely_contains_the_name() { + // `price` is a substring of these idents but not a whole match. + assert!(!check_references_column("price_cents > 0", "price")); + assert!(!check_references_column("unit_price > 0", "price")); + } +} + +/// Whether any CHECK constraint on `table` references `column` — both the +/// table-level CHECKs (`read_table_checks`) and the column-level CHECKs +/// (`schema.columns[].check`), the guard for drop/rename column +/// (ADR-0035 §4e). +/// +/// `include_self` controls whether the column's *own* column-level CHECK +/// counts: a RENAME rewrites the live CHECK but leaves the stored text +/// stale (drift) even for a self-check, so it must be included; a DROP +/// removes the column's own CHECK alongside it, so it is excluded (only a +/// *cross-referencing* CHECK blocks the drop). +fn column_referenced_by_check( + conn: &Connection, + table: &str, + schema: &ReadSchema, + column: &str, + include_self: bool, +) -> Result { + for expr in read_table_checks(conn, table)? { + if check_references_column(&expr, column) { + return Ok(true); + } + } + for col in &schema.columns { + if !include_self && col.name == column { + continue; + } + if let Some(check) = &col.check + && check_references_column(check, column) + { + return Ok(true); + } + } + Ok(false) +} + /// Read the user-created indexes on `table` (ADR-0025). /// /// `pragma_index_list` reports every index; we keep only those @@ -6042,6 +6191,23 @@ fn resolve_index_name(name: Option<&str>, table: &str, columns: &[String]) -> St ) } +/// Refuse an internal `__rdbms_*` table as "no such table" — the same +/// opacity the rest of the app presents (internal tables are filtered +/// from `list_tables` and never offered in completion). Guards the +/// user-facing schema-mutation executors so a deliberately-typed +/// internal name cannot index or alter the metadata tables (ADR-0035 +/// §4d/§4e; the grammar's `reject_internal_table` covers only the typed +/// SQL family, not the simple DSL nodes). +fn reject_internal_table_name(table: &str) -> Result<(), DbError> { + if table.to_ascii_lowercase().starts_with("__rdbms_") { + return Err(DbError::Sqlite { + message: format!("no such table: {table}"), + kind: SqliteErrorKind::NoSuchTable, + }); + } + Ok(()) +} + /// Whether an index named `name` exists (ADR-0035 §4d skip checks). /// /// `user_only = true` counts only explicit `CREATE INDEX` objects @@ -6072,19 +6238,10 @@ fn do_add_index( columns: &[String], unique: bool, ) -> Result { - // 0. Internal `__rdbms_*` tables are not user tables (they are - // filtered from `list_tables` and never offered in completion), so - // indexing one is refused as "no such table" — the same opacity - // the rest of the app presents. Guards BOTH the simple `add index` - // and the SQL `CREATE INDEX` surfaces, since both reach here - // (ADR-0025 / ADR-0035 §4d; the grammar's `reject_internal_table` - // only covers the typed SQL family, not the simple node). - if table.to_ascii_lowercase().starts_with("__rdbms_") { - return Err(DbError::Sqlite { - message: format!("no such table: {table}"), - kind: SqliteErrorKind::NoSuchTable, - }); - } + // 0. Internal tables are not user tables (ADR-0025 / ADR-0035 §4d) — + // refused on both the simple `add index` and SQL `CREATE INDEX` + // surfaces, which both reach here. + reject_internal_table_name(table)?; // 1. Table must exist; gather its columns. let schema = read_schema(conn, table)?; // 2. Every indexed column must exist on the table. diff --git a/src/dsl/command.rs b/src/dsl/command.rs index 92dbfb7..4edef10 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -301,6 +301,15 @@ pub enum Command { unique: bool, if_not_exists: bool, }, + /// Advanced-mode SQL `ALTER TABLE
` (ADR-0035 §4, + /// sub-phase 4e). `alter` is advanced-only. Each action maps to an + /// existing column executor — the runtime decomposes it to + /// `add_column` / `drop_column` / `rename_column` (one undo step + /// each). 4f/4g/4h extend [`AlterTableAction`]. + SqlAlterTable { + table: String, + action: AlterTableAction, + }, /// Add a column-level constraint to an existing column /// (ADR-0029 §2.2). Applied through the rebuild-table /// primitive after a §5 dry-run guards populated columns. @@ -705,6 +714,25 @@ pub enum IndexSelector { Columns { table: String, columns: Vec }, } +/// The action of an advanced-mode `ALTER TABLE` (ADR-0035 §4). Sub-phase +/// 4e carries the column actions; 4f/4g/4h add `AlterColumnType`, +/// `AddConstraint`/`AddForeignKey`/`DropConstraint`, and `RenameTo`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AlterTableAction { + /// `ADD COLUMN [NOT NULL] [UNIQUE] [DEFAULT …] + /// [CHECK …]` — column constraints only (no PK / inline REFERENCES; + /// those are create-table / 4g). Reuses `do_add_column`. Boxed so + /// the large `ColumnSpec` doesn't bloat the enum (and `Command` / + /// `Action` that embed it) — `clippy::large_enum_variant`. + AddColumn(Box), + /// `DROP COLUMN ` — reuses `do_drop_column` (cascade = false: + /// an index-covered column is refused, matching SQLite + the + /// simple-mode default; there is no `--cascade` SQL spelling). + DropColumn { column: String }, + /// `RENAME COLUMN TO ` — reuses `do_rename_column`. + RenameColumn { old: String, new: String }, +} + impl std::fmt::Display for IndexSelector { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -735,6 +763,7 @@ impl Command { Self::DropIndex { .. } => "drop index", Self::SqlDropIndex { .. } => "drop index", Self::SqlCreateIndex { .. } => "create index", + Self::SqlAlterTable { .. } => "alter table", Self::AddConstraint { .. } => "add constraint", Self::DropConstraint { .. } => "drop constraint", Self::ShowTable { .. } => "show table", @@ -813,6 +842,7 @@ impl Command { // `DropIndex` / `SqlDropTable` fallback). Self::SqlDropIndex { name, .. } => name, Self::SqlCreateIndex { table, .. } => table, + Self::SqlAlterTable { table, .. } => table, // Replay isn't tied to a single table; the path is // the most identifying thing for log output. Self::Replay { path } => path, diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index fb7198f..3b62475 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -13,8 +13,8 @@ use crate::dsl::action::ReferentialAction; use crate::dsl::command::{ - ChangeColumnMode, ColumnSpec, Command, Constraint, ConstraintKind, Expr, IndexSelector, - RelationshipSelector, SqlForeignKey, + AlterTableAction, ChangeColumnMode, ColumnSpec, Command, Constraint, ConstraintKind, Expr, + IndexSelector, RelationshipSelector, SqlForeignKey, }; use crate::dsl::value::Value; use crate::dsl::grammar::{ @@ -1841,6 +1841,182 @@ pub static SQL_CREATE_INDEX: CommandNode = CommandNode { usage_ids: &["parse.usage.sql_create_index"], }; +// ================================================================= +// SQL `ALTER TABLE ` (ADR-0035 §4, sub-phase 4e). +// `alter` is an advanced-*only* entry word (like `select`/`with`). +// Actions: ADD/DROP/RENAME COLUMN — the `COLUMN` keyword is required +// (reserves bare `RENAME TO` for 4h and `ADD CONSTRAINT` for 4g). +// ================================================================= + +// The ALTER table slot carries the SQL-family `reject_internal_table` +// validator (parse-time refusal; the executors guard the rest) and +// `writes_table` so the DROP/RENAME column slot narrows to its columns. +const AT_TABLE_NAME: Node = Node::Ident { + source: IdentSource::Tables, + role: "table_name", + validator: Some(super::sql_select::reject_internal_table), + highlight_override: None, + writes_table: true, + writes_column: false, + writes_user_listed_column: false, + writes_table_alias: false, + writes_cte_name: false, + writes_projection_alias: false, +}; + +// ADD COLUMN's constraint suffix — the SQL leaf nodes for NOT NULL / +// UNIQUE / DEFAULT / CHECK only. PK and inline REFERENCES are +// deliberately excluded (PK is invalid on ADD COLUMN; REFERENCES is 4g). +static AT_ADD_CONSTRAINT_CHOICES: &[Node] = &[ + Node::Seq(super::sql_create_table::NOT_NULL_NODES), + Node::Word(Word::keyword("unique")), + Node::Seq(super::sql_create_table::DEFAULT_NODES), + Node::Seq(super::sql_create_table::CHECK_NODES), +]; +const AT_ADD_CONSTRAINT: Node = Node::Choice(AT_ADD_CONSTRAINT_CHOICES); +const AT_ADD_CONSTRAINT_SUFFIX: Node = Node::Repeated { + inner: &AT_ADD_CONSTRAINT, + separator: None, + min: 0, +}; + +static AT_ADD_COLUMN_NODES: &[Node] = &[ + Node::Word(Word::keyword("add")), + Node::Word(Word::keyword("column")), + super::sql_create_table::COL_NAME, + super::sql_create_table::SQL_TYPE, + AT_ADD_CONSTRAINT_SUFFIX, +]; +const AT_ADD_COLUMN: Node = Node::Seq(AT_ADD_COLUMN_NODES); + +static AT_DROP_COLUMN_NODES: &[Node] = &[ + Node::Word(Word::keyword("drop")), + Node::Word(Word::keyword("column")), + COLUMN_NAME, +]; +const AT_DROP_COLUMN: Node = Node::Seq(AT_DROP_COLUMN_NODES); + +static AT_RENAME_COLUMN_NODES: &[Node] = &[ + Node::Word(Word::keyword("rename")), + Node::Word(Word::keyword("column")), + COLUMN_NAME, + Node::Word(Word::keyword("to")), + NEW_COLUMN_NAME, +]; +const AT_RENAME_COLUMN: Node = Node::Seq(AT_RENAME_COLUMN_NODES); + +// Each action branch leads on a concrete keyword (`add`/`drop`/ +// `rename`) — trap-safe. +static AT_ACTION_CHOICES: &[Node] = &[AT_ADD_COLUMN, AT_DROP_COLUMN, AT_RENAME_COLUMN]; +const AT_ACTION: Node = Node::Choice(AT_ACTION_CHOICES); + +static SQL_ALTER_TABLE_SHAPE_NODES: &[Node] = &[ + Node::Word(Word::keyword("table")), + AT_TABLE_NAME, + AT_ACTION, + Node::Optional(&Node::Punct(';')), +]; +const SQL_ALTER_TABLE_SHAPE: Node = Node::Seq(SQL_ALTER_TABLE_SHAPE_NODES); + +/// Build the single `ColumnSpec` for an `ALTER TABLE … ADD COLUMN` +/// (ADR-0035 §4e). Mirrors the SQL `CREATE TABLE` per-column extraction +/// for one column: DEFAULT/CHECK are captured as **raw text** by byte +/// span (`sql_expr` builds no AST — 4a.2), so the executor consumes +/// `default_sql`/`check_sql`. +fn build_alter_add_column_spec( + path: &MatchedPath, + source: &str, +) -> Result { + let mut spec: Option = None; + let mut pending_name: Option = None; + let mut items = path.items.iter().peekable(); + while let Some(item) = items.next() { + match &item.kind { + MatchedKind::Ident { role: "col_name", .. } => { + pending_name = Some(item.text.clone()); + } + MatchedKind::Ident { role: "col_type", .. } => { + let ty = Type::from_sql_name(&item.text).ok_or_else(|| ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "unknown type".to_string())], + })?; + let name = pending_name.take().ok_or_else(sql_col_type_without_name)?; + spec = Some(ColumnSpec::new(name, ty)); + } + MatchedKind::Word("double") => { + if matches!( + items.peek().map(|i| &i.kind), + Some(MatchedKind::Word("precision")) + ) { + items.next(); + } + let name = pending_name.take().ok_or_else(sql_col_type_without_name)?; + spec = Some(ColumnSpec::new(name, Type::Real)); + } + MatchedKind::Word("not") => { + if matches!(items.peek().map(|i| &i.kind), Some(MatchedKind::Word("null"))) { + items.next(); + if let Some(s) = spec.as_mut() { + s.not_null = true; + } + } + } + MatchedKind::Word("unique") => { + if let Some(s) = spec.as_mut() { + s.unique = true; + } + } + MatchedKind::Word("default") => { + if let Some((start, end)) = capture_expr_span(&mut items) + && let Some(s) = spec.as_mut() + { + s.default_sql = Some(source[start..end].trim().to_string()); + } + } + MatchedKind::Word("check") => { + if let Some((start, end)) = capture_parenthesised_span(&mut items) + && let Some(s) = spec.as_mut() + { + s.check_sql = Some(source[start..end].trim().to_string()); + } + } + _ => {} + } + } + spec.ok_or_else(|| ValidationError { + message_key: "parse.error_wrapper", + args: vec![("detail", "add column needs a name and type".to_string())], + }) +} + +/// Build `Command::SqlAlterTable` (ADR-0035 §4e). The action is the +/// leading concrete keyword (`add`/`drop`/`rename` — exactly one matches +/// per the action `Choice`). +fn build_sql_alter_table(path: &MatchedPath, source: &str) -> Result { + let table = require_ident(path, "table_name")?; + let action = if path.contains_word("add") { + AlterTableAction::AddColumn(Box::new(build_alter_add_column_spec(path, source)?)) + } else if path.contains_word("rename") { + AlterTableAction::RenameColumn { + old: require_ident(path, "column_name")?, + new: require_ident(path, "new_column_name")?, + } + } else { + AlterTableAction::DropColumn { + column: require_ident(path, "column_name")?, + } + }; + Ok(Command::SqlAlterTable { table, action }) +} + +pub static SQL_ALTER_TABLE: CommandNode = CommandNode { + entry: Word::keyword("alter"), + shape: SQL_ALTER_TABLE_SHAPE, + ast_builder: build_sql_alter_table, + help_id: Some("ddl.sql_alter_table"), + usage_ids: &["parse.usage.sql_alter_table"], +}; + // ================================================================= // Tests — `create table` column constraints (ADR-0029 §2.1, §9) // ================================================================= @@ -2293,3 +2469,104 @@ mod sql_create_index_tests { )); } } + +#[cfg(test)] +mod sql_alter_table_tests { + use crate::dsl::command::{AlterTableAction, ColumnSpec, Command}; + use crate::dsl::parser::parse_command_in_mode; + use crate::mode::Mode; + + fn alter(input: &str) -> (String, AlterTableAction) { + match parse_command_in_mode(input, Mode::Advanced).expect("should parse") { + Command::SqlAlterTable { table, action } => (table, action), + other => panic!("expected SqlAlterTable, got {other:?}"), + } + } + + fn added_spec(input: &str) -> ColumnSpec { + match alter(input).1 { + AlterTableAction::AddColumn(spec) => *spec, + other => panic!("expected AddColumn, got {other:?}"), + } + } + + #[test] + fn add_column_plain() { + let (table, action) = alter("alter table T add column note text"); + assert_eq!(table, "T"); + match action { + AlterTableAction::AddColumn(spec) => { + assert_eq!(spec.name, "note"); + assert_eq!(spec.ty, crate::dsl::types::Type::Text); + assert!(!spec.not_null && !spec.unique); + assert!(spec.default_sql.is_none() && spec.check_sql.is_none()); + } + other => panic!("expected AddColumn, got {other:?}"), + } + } + + #[test] + fn add_column_with_not_null_and_unique() { + let spec = added_spec("alter table T add column code text not null unique"); + assert!(spec.not_null && spec.unique); + } + + #[test] + fn add_column_with_default_and_check_capture_raw_text() { + // DEFAULT / CHECK are captured as raw SQL text (sql_expr is + // validate-only) — ADR-0035 §4e. + let spec = added_spec("alter table T add column qty int default 0 check (qty >= 0)"); + assert_eq!(spec.default_sql.as_deref(), Some("0")); + assert_eq!(spec.check_sql.as_deref(), Some("qty >= 0")); + } + + #[test] + fn add_column_accepts_sql_type_alias() { + // `varchar(255)` → text, length discarded (ADR-0035 §3). + let spec = added_spec("alter table T add column name varchar(255)"); + assert_eq!(spec.ty, crate::dsl::types::Type::Text); + } + + #[test] + fn drop_column() { + match alter("alter table T drop column note").1 { + AlterTableAction::DropColumn { column } => assert_eq!(column, "note"), + other => panic!("expected DropColumn, got {other:?}"), + } + } + + #[test] + fn rename_column() { + match alter("alter table T rename column a to b").1 { + AlterTableAction::RenameColumn { old, new } => { + assert_eq!(old, "a"); + assert_eq!(new, "b"); + } + other => panic!("expected RenameColumn, got {other:?}"), + } + // trailing semicolon tolerated + assert!(matches!( + alter("alter table T rename column a to b;").1, + AlterTableAction::RenameColumn { .. } + )); + } + + #[test] + fn alter_is_advanced_only() { + // No simple `alter`; in simple mode it does not parse as a + // command (the dispatcher emits the "this is SQL" hint). + assert!(parse_command_in_mode("alter table T drop column c", Mode::Simple).is_err()); + } + + #[test] + fn internal_table_is_rejected_at_parse() { + // The ALTER table slot carries `reject_internal_table`. + assert!( + parse_command_in_mode( + "alter table __rdbms_playground_columns drop column table_name", + Mode::Advanced + ) + .is_err() + ); + } +} diff --git a/src/dsl/grammar/mod.rs b/src/dsl/grammar/mod.rs index a426814..79212de 100644 --- a/src/dsl/grammar/mod.rs +++ b/src/dsl/grammar/mod.rs @@ -594,6 +594,10 @@ pub static REGISTRY: &[(&CommandNode, CommandCategory)] = &[ // `create [unique] index …` → SQL_CREATE_INDEX). (&ddl::SQL_CREATE_TABLE, CommandCategory::Advanced), (&ddl::SQL_CREATE_INDEX, CommandCategory::Advanced), + // `alter` is a new advanced-*only* DDL entry word (ADR-0035 §2/§4e), + // like `select`/`with` — no simple node, so `is_advanced_only` is + // true and simple-mode `alter …` gets the "this is SQL" hint. + (&ddl::SQL_ALTER_TABLE, CommandCategory::Advanced), // Shared `drop` entry word: `ddl::DROP` (simple) and these advanced // SQL nodes. SQL-first in advanced mode; `drop table [if exists] T` // → SQL_DROP_TABLE, `drop index [if exists] ` → SQL_DROP_INDEX diff --git a/src/dsl/grammar/sql_create_table.rs b/src/dsl/grammar/sql_create_table.rs index 5b4582e..3efbab5 100644 --- a/src/dsl/grammar/sql_create_table.rs +++ b/src/dsl/grammar/sql_create_table.rs @@ -95,11 +95,11 @@ static SQL_TYPE_CHOICES: &[Node] = &[ Node::Seq(TYPE_WITH_LENGTH_NODES), ]; /// `double precision | [ '(' n [, n] ')' ]`. -const SQL_TYPE: Node = Node::Choice(SQL_TYPE_CHOICES); +pub(crate) const SQL_TYPE: Node = Node::Choice(SQL_TYPE_CHOICES); // --- Column-level constraints (4a clean-reuse set only) ----------- -static NOT_NULL_NODES: &[Node] = &[ +pub(crate) static NOT_NULL_NODES: &[Node] = &[ Node::Word(Word::keyword("not")), Node::Word(Word::keyword("null")), ]; @@ -132,8 +132,8 @@ static DEFAULT_VALUE_CHOICES: &[Node] = &[ Node::Word(Word::keyword("false")), ]; const DEFAULT_VALUE: Node = Node::Choice(DEFAULT_VALUE_CHOICES); -static DEFAULT_NODES: &[Node] = &[Node::Word(Word::keyword("default")), DEFAULT_VALUE]; -static CHECK_NODES: &[Node] = &[ +pub(crate) static DEFAULT_NODES: &[Node] = &[Node::Word(Word::keyword("default")), DEFAULT_VALUE]; +pub(crate) static CHECK_NODES: &[Node] = &[ Node::Word(Word::keyword("check")), Node::Punct('('), Node::Subgrammar(&sql_expr::SQL_OR_EXPR), @@ -217,7 +217,7 @@ const COL_CONSTRAINT_SUFFIX: Node = Node::Repeated { // --- Column definition: ` [constraints…]` ------------ -const COL_NAME: Node = Node::Ident { +pub(crate) const COL_NAME: Node = Node::Ident { source: IdentSource::NewName, role: "col_name", validator: None, diff --git a/src/dsl/mod.rs b/src/dsl/mod.rs index d2e9895..a5caff7 100644 --- a/src/dsl/mod.rs +++ b/src/dsl/mod.rs @@ -20,8 +20,9 @@ pub mod walker; pub use action::ReferentialAction; pub use command::{ - AppCommand, ChangeColumnMode, ColumnSpec, Command, CompareOp, Expr, IndexSelector, - MessagesValue, ModeValue, Operand, Predicate, RelationshipSelector, RowFilter, SqlForeignKey, + AlterTableAction, AppCommand, ChangeColumnMode, ColumnSpec, Command, CompareOp, Expr, + IndexSelector, MessagesValue, ModeValue, Operand, Predicate, RelationshipSelector, RowFilter, + SqlForeignKey, }; pub use parser::{ParseError, parse_command}; pub use types::Type; diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index 0209d87..de10dfd 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -175,6 +175,7 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("help.ddl.sql_drop_table", &[]), ("help.ddl.sql_create_index", &[]), ("help.ddl.sql_drop_index", &[]), + ("help.ddl.sql_alter_table", &[]), // Advanced-mode SQL CREATE TABLE / DROP TABLE no-op notes (ADR-0035 §4). ("ddl.create_skipped_exists", &["name"]), ("ddl.drop_skipped_absent", &["name"]), @@ -255,6 +256,7 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("parse.usage.sql_drop_table", &[]), ("parse.usage.sql_create_index", &[]), ("parse.usage.sql_drop_index", &[]), + ("parse.usage.sql_alter_table", &[]), ("parse.usage.delete", &[]), ("parse.usage.drop_column", &[]), ("parse.usage.drop_constraint", &[]), diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index 8d2b3a4..8aa09c5 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -270,6 +270,10 @@ help: — create an index (advanced SQL) sql_drop_index: |- drop index [if exists] — remove an index (advanced SQL) + sql_alter_table: |- + alter table add column [not null] [unique] [default …] [check …] + alter table drop column + alter table rename column to — change a table's columns (advanced SQL) drop: |- drop table — remove a table drop column [from] [table] : [--cascade] — remove a column @@ -465,6 +469,10 @@ parse: sql_drop_table: "drop table [if exists] " sql_create_index: "create [unique] index [if not exists] [] on
([, ...])" sql_drop_index: "drop index [if exists] " + sql_alter_table: |- + alter table
add column [not null] [unique] [default ] [check ()] + alter table
drop column + alter table
rename column to drop_table: "drop table " drop_column: "drop column [from] [table]
: " drop_relationship: |- diff --git a/src/runtime.rs b/src/runtime.rs index f5d0570..afb2a58 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -33,7 +33,7 @@ use crate::db::{ Database, DbError, DeleteResult, DropColumnResult, DropIndexOutcome, DropOutcome, InsertResult, QueryPlan, TableDescription, UpdateResult, }; -use crate::dsl::{Command, ColumnSpec}; +use crate::dsl::{AlterTableAction, Command, ColumnSpec}; use crate::dsl::walker::Severity; use crate::event::AppEvent; use crate::project::{ @@ -2095,6 +2095,26 @@ async fn execute_command_typed( CreateIndexOutcome::Created(d) => CommandOutcome::Schema(Some(d)), CreateIndexOutcome::Skipped(n) => CommandOutcome::SchemaCreateIndexSkipped(n), }), + // `ALTER TABLE` (ADR-0035 §4e) decomposes to the existing column + // executors — each already one snapshot (one undo step) and + // journalled. No new worker layer; the outcomes reuse the + // simple-mode add/drop/rename column paths. + Command::SqlAlterTable { table, action } => match action { + AlterTableAction::AddColumn(spec) => database + .add_column(table, *spec, src) + .await + .map(CommandOutcome::AddColumn), + // cascade = false: an index-covered column is refused (no SQL + // `--cascade` spelling), matching SQLite + the simple default. + AlterTableAction::DropColumn { column } => database + .drop_column(table, column, false, src) + .await + .map(CommandOutcome::DropColumn), + AlterTableAction::RenameColumn { old, new } => database + .rename_column(table, old, new, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), + }, Command::AddConstraint { table, column, diff --git a/tests/column_op_guards.rs b/tests/column_op_guards.rs new file mode 100644 index 0000000..7ad51f4 --- /dev/null +++ b/tests/column_op_guards.rs @@ -0,0 +1,212 @@ +//! Executor-level guards on the shared column operations (ADR-0035 §4e). +//! +//! These guards live in `do_add_column` / `do_drop_column` / +//! `do_rename_column`, so they apply to BOTH the simple-mode DSL +//! commands (exercised here) and the advanced-mode SQL `ALTER TABLE` +//! (which reaches the same executors). Two guards: +//! 1. internal `__rdbms_*` tables are refused as "no such table"; +//! 2. dropping/renaming a column a table-level CHECK references is +//! refused up-front (the 4a.3 deferral; it also fixes a latent +//! rename-drift bug that would break a later rebuild). + +use rdbms_playground::db::Database; +use rdbms_playground::dsl::{ColumnSpec, Type}; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open() -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let persistence = Persistence::new(project.path().to_path_buf()); + let db = Database::open_with_persistence_and_undo(project.db_path(), persistence, true) + .expect("open db with persistence"); + (project, db, dir) +} + +/// `T (id int pk, a int, b int, c text)` with a table-level CHECK +/// `a < b`. +fn make_t_with_check(db: &Database, r: &tokio::runtime::Runtime) { + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ + ColumnSpec::new("id", Type::Int), + ColumnSpec::new("a", Type::Int), + ColumnSpec::new("b", Type::Int), + ColumnSpec::new("c", Type::Text), + ], + vec!["id".to_string()], + vec![], + vec!["a < b".to_string()], + vec![], + false, + Some("create table T (id int primary key, a int, b int, c text, check (a < b))".to_string()), + )) + .expect("create T with table CHECK"); +} + +#[test] +fn simple_column_ops_refuse_internal_tables() { + let (_p, db, _d) = open(); + let r = rt(); + let internal = "__rdbms_playground_columns".to_string(); + assert!( + r.block_on(db.add_column( + internal.clone(), + ColumnSpec::new("x", Type::Int), + Some("add column".to_string()) + )) + .is_err(), + "add column on an internal table is refused" + ); + assert!( + r.block_on(db.drop_column(internal.clone(), "table_name".to_string(), false, None)) + .is_err(), + "drop column on an internal table is refused" + ); + assert!( + r.block_on(db.rename_column(internal, "table_name".to_string(), "tn".to_string(), None)) + .is_err(), + "rename column on an internal table is refused" + ); +} + +#[test] +fn drop_column_referenced_by_a_table_check_is_refused() { + let (_p, db, _d) = open(); + let r = rt(); + make_t_with_check(&db, &r); + // `a` is referenced by the CHECK `a < b` → refused (both surfaces; + // here via the simple `drop column`). + assert!( + r.block_on(db.drop_column("T".to_string(), "a".to_string(), false, None)) + .is_err(), + "dropping a CHECK-referenced column is refused" + ); + // `c` is not referenced → the drop succeeds. + r.block_on(db.drop_column("T".to_string(), "c".to_string(), false, None)) + .expect("dropping an unreferenced column succeeds"); +} + +#[test] +fn rename_column_referenced_by_a_table_check_is_refused() { + let (_p, db, _d) = open(); + let r = rt(); + make_t_with_check(&db, &r); + // `a` is referenced → refused (without this guard, a native rename + // would silently drift the CHECK metadata and break rebuild). + assert!( + r.block_on(db.rename_column("T".to_string(), "a".to_string(), "z".to_string(), None)) + .is_err(), + "renaming a CHECK-referenced column is refused" + ); + // `c` is not referenced → rename succeeds. + r.block_on(db.rename_column("T".to_string(), "c".to_string(), "note".to_string(), None)) + .expect("renaming an unreferenced column succeeds"); +} + +/// `T (id int pk, price int, discount int CHECK(discount < price), +/// qty int CHECK(qty >= 0))` — column-level CHECKs (ADR-0035 §4e). +fn make_t_with_column_checks(db: &Database, r: &tokio::runtime::Runtime) { + let mut discount = ColumnSpec::new("discount", Type::Int); + discount.check_sql = Some("discount < price".to_string()); + let mut qty = ColumnSpec::new("qty", Type::Int); + qty.check_sql = Some("qty >= 0".to_string()); + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ + ColumnSpec::new("id", Type::Int), + ColumnSpec::new("price", Type::Int), + discount, + qty, + ], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table T (...)".to_string()), + )) + .expect("create T with column CHECKs"); +} + +#[test] +fn rename_column_with_a_column_level_check_is_refused() { + // A native RENAME would leave the stored column-level CHECK text + // stale (drift → broken rebuild), so it is refused — including a + // column's own self-check. + let (_p, db, _d) = open(); + let r = rt(); + make_t_with_column_checks(&db, &r); + // `qty`'s own check `qty >= 0` references qty → refused. + assert!( + r.block_on(db.rename_column("T".to_string(), "qty".to_string(), "amount".to_string(), None)) + .is_err(), + "renaming a column with its own column-level CHECK is refused" + ); + // `price` is referenced by `discount`'s check `discount < price`. + assert!( + r.block_on(db.rename_column("T".to_string(), "price".to_string(), "cost".to_string(), None)) + .is_err(), + "renaming a column referenced by another column's CHECK is refused" + ); + // `id` is referenced by no CHECK → rename succeeds. + r.block_on(db.rename_column("T".to_string(), "id".to_string(), "pk".to_string(), None)) + .expect("renaming an unreferenced column succeeds"); +} + +#[test] +fn drop_column_referenced_by_another_columns_check_is_refused_but_own_check_drops() { + let (p, db, _d) = open(); + let r = rt(); + make_t_with_column_checks(&db, &r); + // `price` is referenced by `discount`'s check → refused. + assert!( + r.block_on(db.drop_column("T".to_string(), "price".to_string(), false, None)) + .is_err(), + "dropping a column another column's CHECK references is refused" + ); + // `qty` has only its OWN check → it drops with the column. + r.block_on(db.drop_column("T".to_string(), "qty".to_string(), false, None)) + .expect("dropping a column whose only CHECK is its own succeeds"); + // Rebuild still works (the remaining `discount < price` CHECK's + // columns survive). + r.block_on(db.rebuild_from_text(p.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild succeeds after dropping the self-checked column"); +} + +#[test] +fn rebuild_survives_after_dropping_an_unreferenced_column() { + // Guard is not over-broad: a table that carries a CHECK still + // rebuilds after an unrelated column is dropped (the CHECK's + // referenced columns remain). + let (p, db, _d) = open(); + let r = rt(); + make_t_with_check(&db, &r); + r.block_on(db.drop_column("T".to_string(), "c".to_string(), false, None)) + .expect("drop unreferenced column"); + r.block_on(db.rebuild_from_text(p.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild succeeds — the CHECK still references existing columns"); + // The CHECK is intact: it still enforces a < b. + assert!( + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "a".to_string(), "b".to_string()]), + vec![ + rdbms_playground::dsl::Value::Number("1".to_string()), + rdbms_playground::dsl::Value::Number("5".to_string()), + rdbms_playground::dsl::Value::Number("3".to_string()), + ], + Some("insert".to_string()), + )) + .is_err(), + "CHECK a < b still enforced after the rebuild (5 < 3 is false)" + ); +} diff --git a/tests/sql_alter_table.rs b/tests/sql_alter_table.rs new file mode 100644 index 0000000..444a5e3 --- /dev/null +++ b/tests/sql_alter_table.rs @@ -0,0 +1,140 @@ +//! Sub-phase 4e Tier-3 end-to-end tests for advanced-mode SQL +//! `ALTER TABLE` add/drop/rename column (ADR-0035 §4e). +//! +//! These drive the **full advanced-mode pipeline** via `run_replay`: a +//! literal `alter table …` line is parsed in Advanced mode, routed to +//! `Command::SqlAlterTable`, decomposed by the runtime to the existing +//! column executor, and persisted. They prove the decomposition for all +//! three actions and the **raw-text DEFAULT/CHECK ADD COLUMN** path (the +//! 4e executor extension). The drop/rename refusals (PK / FK / index / +//! table-CHECK) live in the shared executors and are covered by +//! `tests/column_op_guards.rs` — the SQL surface reaches the same code. + +use rdbms_playground::db::Database; +use rdbms_playground::dsl::Value; +use rdbms_playground::event::AppEvent; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; +use rdbms_playground::runtime::run_replay; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open() -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let db = Database::open_with_persistence( + project.db_path(), + Persistence::new(project.path().to_path_buf()), + ) + .expect("db"); + (project, db, dir) +} + +fn column_names(db: &Database, r: &tokio::runtime::Runtime) -> Vec { + r.block_on(db.describe_table("T".to_string(), None)) + .expect("describe") + .columns + .into_iter() + .map(|c| c.name) + .collect() +} + +#[test] +fn e2e_alter_table_add_rename_drop_and_raw_default_check() { + let (project, db, _d) = open(); + let r = rt(); + // A script exercising all three actions through the full pipeline. + // `v` is added (simple) so there is a non-PK column to rename/drop; + // a row is inserted before the ADD so the DEFAULT backfill is + // exercised by the rebuild. + std::fs::write( + project.path().join("alter.commands"), + "create table T with pk id(int)\n\ + add column T: v (text)\n\ + insert into T (id, v) values (1, 'a')\n\ + alter table T add column qty int default 0 check (qty >= 0)\n\ + alter table T rename column v to label\n\ + alter table T add column note text\n\ + alter table T drop column note\n", + ) + .expect("write script"); + + let events = r.block_on(run_replay(&db, project.path(), "alter.commands")); + match events.last().expect("at least one event") { + AppEvent::ReplayCompleted { count, .. } => { + assert_eq!(*count, 7, "all seven lines replayed; events: {events:?}"); + } + other => panic!("expected ReplayCompleted, got {other:?} (events: {events:?})"), + } + + // Final schema: id, label (renamed from v), qty; `note` added then + // dropped. + let cols = column_names(&db, &r); + assert_eq!(cols, vec!["id".to_string(), "label".to_string(), "qty".to_string()]); + + // The DEFAULT backfilled the pre-existing row to qty = 0. + let rows = r + .block_on(db.query_data("T".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows.len(), 1); + // qty is the third column; the rebuild backfilled the default. + assert_eq!(rows[0][2].as_deref(), Some("0"), "DEFAULT 0 backfilled the existing row"); + + // The CHECK (qty >= 0) is enforced: a negative qty is refused. + assert!( + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "qty".to_string()]), + vec![Value::Number("2".to_string()), Value::Number("-1".to_string())], + Some("insert".to_string()), + )) + .is_err(), + "the raw-text CHECK (qty >= 0) added via ALTER is enforced" + ); + // A non-negative qty is accepted. + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "qty".to_string()]), + vec![Value::Number("3".to_string()), Value::Number("7".to_string())], + Some("insert".to_string()), + )) + .expect("qty = 7 satisfies the CHECK"); +} + +#[test] +fn e2e_alter_add_column_survives_rebuild() { + // The column added via SQL ALTER (with a raw CHECK) round-trips + // through the text artifacts and survives a rebuild. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("alter.commands"), + "create table T with pk id(int)\n\ + alter table T add column qty int check (qty >= 0)\n", + ) + .expect("write script"); + r.block_on(run_replay(&db, project.path(), "alter.commands")); + assert!(column_names(&db, &r).contains(&"qty".to_string())); + + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + // The CHECK survives the rebuild — a negative qty is still refused. + assert!(column_names(&db, &r).contains(&"qty".to_string())); + assert!( + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "qty".to_string()]), + vec![Value::Number("1".to_string()), Value::Number("-5".to_string())], + Some("insert".to_string()), + )) + .is_err(), + "the ALTER-added CHECK is intact after rebuild" + ); +} diff --git a/tests/typing_surface/mod.rs b/tests/typing_surface/mod.rs index 9bd989d..d16e4c7 100644 --- a/tests/typing_surface/mod.rs +++ b/tests/typing_surface/mod.rs @@ -229,6 +229,7 @@ fn command_kind_label(cmd: &rdbms_playground::dsl::Command) -> String { DropIndex { .. } => "DropIndex".into(), SqlDropIndex { .. } => "SqlDropIndex".into(), SqlCreateIndex { .. } => "SqlCreateIndex".into(), + SqlAlterTable { .. } => "SqlAlterTable".into(), AddConstraint { .. } => "AddConstraint".into(), DropConstraint { .. } => "DropConstraint".into(), ShowTable { .. } => "ShowTable".into(),