diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 04919e5..0f125b5 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -4,15 +4,17 @@ 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 / -4e / 4f / 4g** (`CREATE TABLE` with column- and table-level constraints -and foreign keys, `DROP TABLE [IF EXISTS]`, `CREATE [UNIQUE] INDEX` / -`DROP INDEX [IF EXISTS]`, `ALTER TABLE` add/drop/rename column, -`ALTER TABLE … ALTER COLUMN TYPE`, and `ALTER TABLE` add/drop constraint -+ add foreign key, implemented 2026-05-25 — plans +4e / 4f / 4g / 4h** (`CREATE TABLE` with column- and table-level +constraints and foreign keys, `DROP TABLE [IF EXISTS]`, +`CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]`, `ALTER TABLE` +add/drop/rename column, `ALTER TABLE … ALTER COLUMN TYPE`, `ALTER TABLE` +add/drop constraint + add foreign key, and `ALTER TABLE … RENAME TO`, +implemented 2026-05-25/26 — 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`, -`…-4e.md`, `…-4f.md`, `…-4g.md`), so the decision is accepted while the -remaining sub-phases (**4h–4i**, §13) continue. This is **Phase 4** of the ADR-0030 roadmap (the +`…-4e.md`, `…-4f.md`, `…-4g.md`, +`docs/plans/20260526-adr-0035-sql-ddl-4h.md`), so the decision is accepted +while the remaining sub-phase (**4i**, §13) continues. 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. @@ -482,6 +484,37 @@ ADR-0033's structure: into `do_add_constraint` / `do_add_relationship`, completing the 4d/4e/4f guard class. One undo step per statement. - **4h — `ALTER TABLE … RENAME TO`** (the §6 new low-level op). + *(Implemented 2026-05-26 — plan + `docs/plans/20260526-adr-0035-sql-ddl-4h.md`.)* The one genuinely new + low-level executor in Phase 4 (`do_rename_table`): a native engine + `RENAME TO` (structure-preserving — no rebuild) plus reconciliation, in + one transaction (commit-db-last), of everything the engine does not + track — every metadata row that names the table + (`__rdbms_playground_columns`, **both ends** of + `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), + the CSV file (via the existing persistence rewrite+delete path: + `rewritten_tables = [new]`, `deleted_tables = [old]` — no new + persistence method), and **CHECK text that qualifies a column with the + old table name** (`T.age` → `U.age`, both column- and table-level — a + planning-`/runda` finding: the engine rewrites the *live* CHECK but the + *stored* text would drift and break a fresh rebuild; + `rewrite_check_table_qualifier` keeps them in step; bounded because a + CHECK references only its own table). Grammar: a fifth action, + `AlterTableAction::RenameTable { new }`, added by splitting the `rename` + verb into one branch with an inner `Choice` on a distinct second keyword + (`column` → rename-column, `to` → rename-table — the §6.1 trap-safe + pattern); the new-name slot mirrors the `CREATE TABLE` name slot + (`IdentSource::NewName` + the `reject_internal_table` parse validator). + **Refusals (user-confirmed 2026-05-26):** rename to the same name, to an + existing other table, to an `__rdbms_*` name, or of a non-existent + table. Collision checks are **case-insensitive** (the engine matches + names that way), with an engine-neutral pre-check so a case-only rename + or a case-insensitive clash never surfaces the raw engine error + (a finished-slice `/runda` finding). **Auto-named indexes *and* + relationships keep their stale names** (only the table-name *columns* + update; ADR-0035 §6 scope — user-confirmed; documented collision + caveat). One undo step (the whole-project snapshot). Advanced-mode only; + closes the rename half of `C1`. - **4i — Verification sweep.** Typing-surface + matrix coverage, engine-neutral error pass, undo-parity check (one step per statement), `help`/usage for the new forms. **Carried in from earlier diff --git a/docs/adr/README.md b/docs/adr/README.md index a89b8a0..72f4cf4 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]` + 4e `ALTER TABLE` add/drop/rename column + 4f `ALTER TABLE … ALTER COLUMN TYPE` + 4g `ALTER TABLE` add/drop constraint + add FK, implemented 2026-05-25; 4h–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) and **4f** (`ALTER TABLE … ALTER COLUMN TYPE` → a fourth `AlterTableAction`, runtime-decomposed to the existing `change_column_type` with `ChangeColumnMode::ForceConversion` — which **is** the §7 advanced policy: lossy converts *with a note* (no force flag), incompatible + ADR-0017 static refusals (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) still refuse, while **`int → serial` is allowed** (auto-fills nulls + UNIQUE, ADR-0018 §8 — the §7 "→serial refused" summary is looser than the code); the builder discriminates the fourth branch by the **`type` keyword** (unique — ADD COLUMN's type is an ident), the type slot reuses `SQL_TYPE`; the internal-`__rdbms_*` guard was folded into `do_change_column_type`, closing the simple `change column` exposure too — user-confirmed) and **4g** (`ALTER TABLE … ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; ADD = CHECK + composite UNIQUE + FK, with `ADD PRIMARY KEY` and a *named* UNIQUE refused — composite UNIQUE is anonymous in our model; each ADD reuses a low-level path (table-CHECK/UNIQUE rebuild with a dry-run guard; FK → `add_relationship`, bare `REFERENCES

` → parent single-PK), DROP CONSTRAINT resolves the name to a table-CHECK then a child-side FK; **named table-CHECKs round-trip** via a nullable `name` column on `__rdbms_playground_table_checks` (**rebuild-only** arrival — pre-4g projects gain it on `rebuild`, a named add on an un-upgraded project is refused with a friendly "rebuild first" message) *and* a `project.yaml` `check_constraints` extension to an `{expr, name}` mapping (the bare-string form still reads); the internal-`__rdbms_*` guard was folded into `do_add_constraint`/`do_add_relationship`, completing that guard class — all user-confirmed); 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 + 4f `ALTER TABLE … ALTER COLUMN TYPE` + 4g `ALTER TABLE` add/drop constraint + add FK + 4h `ALTER TABLE … RENAME TO`, implemented 2026-05-25/26; 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) and **4f** (`ALTER TABLE … ALTER COLUMN TYPE` → a fourth `AlterTableAction`, runtime-decomposed to the existing `change_column_type` with `ChangeColumnMode::ForceConversion` — which **is** the §7 advanced policy: lossy converts *with a note* (no force flag), incompatible + ADR-0017 static refusals (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) still refuse, while **`int → serial` is allowed** (auto-fills nulls + UNIQUE, ADR-0018 §8 — the §7 "→serial refused" summary is looser than the code); the builder discriminates the fourth branch by the **`type` keyword** (unique — ADD COLUMN's type is an ident), the type slot reuses `SQL_TYPE`; the internal-`__rdbms_*` guard was folded into `do_change_column_type`, closing the simple `change column` exposure too — user-confirmed) and **4g** (`ALTER TABLE … ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; ADD = CHECK + composite UNIQUE + FK, with `ADD PRIMARY KEY` and a *named* UNIQUE refused — composite UNIQUE is anonymous in our model; each ADD reuses a low-level path (table-CHECK/UNIQUE rebuild with a dry-run guard; FK → `add_relationship`, bare `REFERENCES

` → parent single-PK), DROP CONSTRAINT resolves the name to a table-CHECK then a child-side FK; **named table-CHECKs round-trip** via a nullable `name` column on `__rdbms_playground_table_checks` (**rebuild-only** arrival — pre-4g projects gain it on `rebuild`, a named add on an un-upgraded project is refused with a friendly "rebuild first" message) *and* a `project.yaml` `check_constraints` extension to an `{expr, name}` mapping (the bare-string form still reads); the internal-`__rdbms_*` guard was folded into `do_add_constraint`/`do_add_relationship`, completing that guard class — all user-confirmed) and **4h** (`ALTER TABLE … RENAME TO` — the one genuinely new low-level op, `do_rename_table`: a native engine rename plus one-transaction reconciliation of every metadata row naming the table (`__rdbms_playground_columns`, **both ends** of `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), the CSV file (the existing rewrite+delete path — no new persistence method), and **CHECK text that qualifies a column with the old table name** (`T.age`→`U.age`, a planning-`/runda` finding — the engine rewrites the live CHECK but the stored text would drift and break a fresh rebuild; `rewrite_check_table_qualifier` keeps them in step); grammar splits the `rename` verb into one branch with an inner Choice on a distinct second keyword (`column` vs `to`), the new-name slot mirroring the `CREATE TABLE` name slot; refuses same-name / existing-target / `__rdbms_*` / non-existent, with **case-insensitive** collision checks behind an engine-neutral pre-check (a finished-slice `/runda` finding — the engine matches names case-insensitively); auto-named indexes *and* relationships keep their stale names (only table-name columns update — §6 scope); one undo step; advanced-only, closing the rename half of `C1` — 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/20260526-adr-0035-sql-ddl-4h.md b/docs/plans/20260526-adr-0035-sql-ddl-4h.md new file mode 100644 index 0000000..cb1de1d --- /dev/null +++ b/docs/plans/20260526-adr-0035-sql-ddl-4h.md @@ -0,0 +1,437 @@ +# Plan: ADR-0035 Phase 4, sub-phase 4h — `ALTER TABLE … RENAME TO` + +Add the advanced-mode SQL form: + +- `ALTER TABLE RENAME TO ` — rename a table. **Advanced-mode + only** (no simple-mode rename-table verb; ADR-0035 §6). Closes the + rename half of `C1` for the advanced surface. + +This is the **one genuinely new low-level op** in Phase 4 (ADR-0035 §6) — +not a reuse of an existing executor. Within one transaction it renames +the table in the database, renames its `data/.csv` → `data/.csv` +(via the persistence layer), and updates **every** metadata row that +names it. + +**User-confirmed scope (2026-05-26):** +- **Same-name rename** (`rename T to T`) → **refuse** with a friendly + error (mirrors the existing `rename column` identical-rename refusal). +- **CHECK-text drift → rewrite the stored CHECK text** (the §7 DA + finding). The engine rewrites table-qualified column references inside + the renamed table's own CHECKs in the *live* schema (`CHECK (T.age>0)` + → `CHECK ("U".age>0)` — confirmed empirically on SQLite 3.48). Our + *stored* CHECK text (both `__rdbms_playground_columns.check_expr` and + `__rdbms_playground_table_checks.check_expr`) must be rewritten the same + way, or a fresh rebuild emits `CHECK (T.age>0)` for a table now named + `U` and fails. **Bounded problem:** a CHECK constraint may reference + only the table's own columns (SQLite forbids subqueries / other tables + in CHECK), so the *only* table qualifier that can appear is the old + table name — the rewrite target is unambiguous (`old`/`"old"` → `new`/ + `"new"`). Reuses/extends the 4e CHECK tokenizer (`check_references_ + column`, `db.rs:5489`) which already skips string literals and is + case-insensitive. +- **Auto-named labels (indexes *and* relationships) → left stale** on + rename. ADR-0035 §6 lists only CSV + column/relationship/table-CHECK + metadata to update; auto-named indexes (`__idx`) and + auto-named relationships (`{parent}_{pcol}_to_{child}_{ccol}`, + `db.rs:5982`) keep their old names — functional, just cosmetically + referencing the old table name. **Documented caveat:** index names are + schema-global and relationship names are `UNIQUE`, so recreating a + table under the *old* name later could collide with a stale label; + this is an accepted consequence (cosmetic refresh is a possible + 4i/follow-up, out of 4h). + +**Decided-and-noted (conventional defaults, no user fork):** +- **Rename to an existing *other* table** → **refuse** "table `` + already exists" (an explicit, engine-neutral pre-check before the + native rename, which would otherwise surface engine wording). +- **Rename of an FK parent or child** → **allowed** (unlike DROP, which + refuses inbound FKs). The native rename rewrites child FK references in + the live schema; we update both ends of the relationship metadata. +- **Success feedback** → **auto-show the renamed table** under its new + name (returns a `TableDescription`, mirroring `rename column`). +- **Target name** → parse-time `reject_internal_table` validator on the + new-name slot (mirrors the `CREATE TABLE` name slot) + executor + `reject_internal_table_name` guard for defense in depth. + +## 1. Baseline (at handoff 40) + +- After 4g + the rebuild fix: **1885 passing, 0 failed, 0 skipped, 1 + ignored** (the `friendly/mod.rs` ` ```ignore ` doctest); clippy clean + (`cargo clippy --all-targets -- -D warnings`). Branch `main`, HEAD + `6112859` (the handoff-40 docs commit; `6ff97f6`/`50a889e`/`6112859` + are local-only — normal). Re-verified this session: **1885 / 0 / 0 / + 1**. + +## 2. Decisions (settled) + +1. **One new low-level executor, `do_rename_table`.** No existing reuse + (§6 is explicit: rename is a genuinely new op). It uses the engine's + native `ALTER TABLE RENAME TO ` (structure-preserving — no + rebuild needed), then updates the three `__rdbms_*` metadata tables, + then drives persistence, then commits the db last (ADR-0015 §6 + ordering). Mirrors the shape of `do_rename_column` (`src/db.rs:4314`). + +2. **CSV rename reuses the existing persistence machinery — no new + method.** `finalize_persistence` (`src/db.rs:2663`) already (a) on + `schema_dirty` rewrites the *entire* `project.yaml` from the live db + schema — so the rename is reflected automatically — and (b) writes a + CSV per `rewritten_tables` entry (read in-tx by name) and deletes a + CSV per `deleted_tables` entry. So: + ```rust + Changes { schema_dirty: true, + rewritten_tables: vec![new.to_string()], // writes data/.csv + deleted_tables: vec![old.to_string()] } // removes data/.csv + ``` + The renamed table is read by its new name (visible in-tx after the + native rename), serialised to `data/.csv`, and `data/.csv` + is deleted. Empty tables produce no CSV on either side (the existing + `write_table_data` empty-→-delete rule), preserving the ADR-0015 + "empty tables → no CSV" invariant. NULL-vs-empty fidelity is preserved + because the rows are re-serialised from the db (where NULL is NULL), + not byte-copied. **No `rename_table_data` method, no `Changes` field + added.** (The Explore-suggested "add a file-rename method" is rejected + in §5 R-alt.) + +3. **Metadata updates — all three tables, both relationship ends, plus + CHECK-text reconciliation.** Within the tx, after the native rename: + - `__rdbms_playground_columns` (`META_TABLE`): `table_name old→new`; + **and** rewrite any column-level `check_expr` whose text qualifies a + reference with the old table name (`old.`/`"old".` → `new.`/`"new".` + — §2.9). + - `__rdbms_playground_relationships` (`REL_TABLE`): `parent_table + old→new` **and** `child_table old→new` (two UPDATEs — covers a table + that is an FK parent, a child, or **self-referential** at once). The + relationship `name` is **not** touched (left stale per the user + decision; FK endpoints are stored as table-name *values*, not as + expression text, so they need no rewrite — only the column UPDATE). + - `__rdbms_playground_table_checks` (`CHECK_TABLE`): `table_name + old→new`; **and** rewrite each table-level `check_expr` the same way + (§2.9). + No index metadata table exists (indexes are PRAGMA-derived with a + `unique` flag; ADR-0025 Amd 1), so nothing to update there — indexes + follow the renamed table natively and keep their (stale) names per the + user decision. DEFAULT expressions do **not** drift (SQLite defaults + cannot reference the table). + +4. **Grammar — split the `rename` verb into an inner Choice** (the §6.1 + "no same-leading-keyword Choice siblings" rule). Today `AT_RENAME_COLUMN` + is the lone `rename`-led branch in `AT_ACTION_CHOICES`. Replace it with + one `AT_RENAME` branch (`rename` + an inner `Choice`) whose two tails + lead on **distinct** second keywords: `column` (→ rename column) and + `to` (→ rename table). Mirrors the 4g `add`/`drop` restructure. + +5. **New-name ident slot is distinct from the target slot.** The table + being altered binds role `table_name` (`AT_TABLE_NAME`, + `IdentSource::Tables`). The rename target binds a **new** role + `new_table_name` (`IdentSource::NewName`, `validator: + Some(reject_internal_table)`, all `writes_*: false`, wrapped in + `NEW_NAME_HINT`) — mirrors the `CREATE TABLE` name slot for parse-time + `__rdbms_*` refusal and the `NEW_COLUMN_NAME` hint treatment. Distinct + roles keep `require_ident(path, "new_table_name")` unambiguous. + +6. **Builder discrimination** (`build_sql_alter_table`, `ddl.rs:2131`): + insert a `rename` branch **before** the final `else` (DropConstraint). + Order becomes `type` → `column` → `add` → **`rename`** → else `drop`. + By the time control reaches the `rename` check, `column` is absent + (caught earlier), so `rename` present ⇒ table rename: + `AlterTableAction::RenameTable { new: require_ident(path, + "new_table_name")? }`. + +7. **One undo step.** `do_rename_table` is one user mutation carrying a + `source`, snapshotted by the worker `snapshot_then` hook (whole-project + snapshot — db backup + yaml/csv copy), so a rename is exactly one + `undo` step. Same wiring as every other `SqlAlterTable` action. + +8. **Replay / history.** `finalize_persistence` appends the literal SQL + line to `history.log`; `alter` is a schema-write entry word (not in the + ADR-0034 Amd 1 app-lifecycle skip set), so the rename replays as a + write with no replay-filter change. + +9. **CHECK-text qualifier rewrite (§7 DA Finding 1).** A new helper + `rewrite_check_table_qualifier(check_expr, old, new) -> String` + rewrites every occurrence of the old table name **used as a qualifier** + (immediately followed by `.`), in both the bare (`old.`) and quoted + (`"old".`) forms, case-insensitively, **skipping string literals** — + extending the 4e tokenizer that `check_references_column` already uses. + A bare token equal to the old name but *not* followed by `.` (e.g. a + column literally named like the table) is left untouched, so the + common unqualified CHECK (`age > 0`) is a no-op. Applied in + `do_rename_table` to every column-level `check_expr` (META) and every + table-level `check_expr` (CHECK_TABLE) of the renamed table. Re-enters + in advanced mode (ADR-0030 §11) — the rewritten text is still valid SQL + the user could retype. + +10. **Existence check is explicit (§7 DA Finding 2).** `read_schema` does + **not** error on a missing table (`pragma_table_info` returns zero + rows). The "no such table" guard uses an explicit + `do_list_tables(conn)?.iter().any(|t| t == old)` check, not a reliance + on `read_schema` failing. + +## 3. Phase 1 — Requirements checklist (4h) + +### Grammar / dispatch +- [ ] `AlterTableAction` gains `RenameTable { new: String }`. +- [ ] `NEW_TABLE_NAME` ident node (role `new_table_name`, + `IdentSource::NewName`, `reject_internal_table` validator, `NEW_NAME_HINT`). +- [ ] `AT_RENAME` = `Seq[rename, Choice[AT_RENAME_COLUMN_TAIL, + AT_RENAME_TABLE_TAIL]]`; the two tails lead on distinct keywords + (`column` / `to`). `AT_RENAME_COLUMN_TAIL` = `column to `; + `AT_RENAME_TABLE_TAIL` = `to `. `AT_ACTION_CHOICES` swaps + `AT_RENAME_COLUMN` → `AT_RENAME`. +- [ ] `build_sql_alter_table` routes `rename` (no `column`) → `RenameTable`. +- [ ] Existing four action branches still route (add/drop/rename/alter + column, alter-column-type, add/drop constraint); trailing `;` tolerated; + `alter` stays advanced-only; source table slot rejects `__rdbms_*` at + parse (existing `AT_TABLE_NAME` validator); target slot rejects + `__rdbms_*` at parse (new validator). + +### Execution +- [ ] `do_rename_table(conn, persistence, source, old, new)`: + `reject_internal_table_name(old)` + `(new)`; **existence** — explicit + `do_list_tables` contains `old` (→ friendly "no such table", *not* a + reliance on `read_schema` erroring — §2.10); **same-name** (`old==new`) + → friendly refusal; **existing-target** (`do_list_tables` contains + `new`) → friendly "already exists" refusal (pre-empts the engine's own + collision wording); tx: native `ALTER TABLE … RENAME TO` + the metadata + UPDATEs (§2.3) **+ the CHECK-text rewrite** (§2.9, both META and + CHECK_TABLE `check_expr`); `do_describe_table(conn, new)` for auto-show; + `finalize_persistence` with the §2.2 `Changes`; `tx.commit()`. +- [ ] `rewrite_check_table_qualifier` helper (§2.9) + its own unit tests + (bare `T.age`→`U.age`; quoted `"T".age`→`"U".age`; case-insensitive; + string literal `'T.x'` untouched; bare column named `T` untouched; + unqualified `age > 0` unchanged). +- [ ] Worker `Request::RenameTable { name, new, source, reply }`; + `Database::rename_table(table, new, source)` method; handler arm wrapped + in `snapshot_then` (one undo step). +- [ ] `runtime.rs` `SqlAlterTable` match: `RenameTable { new }` → + `database.rename_table(table, new, src)` mapped like the `RenameColumn` + arm. +- [ ] `app.rs` `build_translate_context`: `RenameTable { .. }` → + `(Operation::RenameTable, Some(table), None)`. +- [ ] `Operation::RenameTable` added; `keyword()` arm → `"rename table"`. + +### Testing +- [ ] **Tier 1** (`sql_alter_table_tests` in `ddl.rs`): parse `alter table + T rename to U` → `RenameTable { new: "U" }`; `alter table T rename + column a to b` still → `RenameColumn`; the other four actions still + route; target `__rdbms_*` refused at parse. +- [ ] **Tier 3** (`tests/sql_alter_table.rs` via `run_replay`): + - rename a table **with rows** → the CSV follows (`data/.csv` + present, `data/.csv` gone), data intact incl. a NULL. + - rename an **FK parent** → relationship metadata `parent_table` + updates; the child's FK still enforces (a violating child insert is + rejected under the new name). + - rename an **FK child** → `child_table` updates; FK still enforces. + - rename a **self-referential** table → both ends update, no PK + conflict on `REL_TABLE`. + - rename a table with a **table-level CHECK** → `table_checks` rows + follow; the CHECK still enforces. + - rename a table with a **table-qualified CHECK** (both a column-level + `CHECK (T.age > 0)` and a table-level `CHECK (T.a <> T.b)`) → the + stored `check_expr` is rewritten to the new name, the CHECK still + enforces, **and the project survives a fresh rebuild** (the precise + §7 Finding-1 regression — without the rewrite, rebuild fails with "no + such table T"). + - rename a table with an **index** → index still present + functional + (name unchanged, per the user decision). + - **survives a fresh rebuild** — delete the `.db`, `rebuild` from + `project.yaml`/CSV: the renamed table + all its metadata round-trip + (the §6.4 fresh-rebuild discipline). + - **one undo step**: rename, `undo`, the table is back under its old + name with its rows/relationship/CHECK; `redo` reapplies. + - **refusals**: rename to an existing other table; rename to the same + name; rename to an `__rdbms_*` name (executor guard, in case the + parse validator is bypassed by a synthesised command); rename a + non-existent table. +- [ ] **Catalog** lockstep + vocab audit for the refreshed + `sql_alter_table` usage (now listing `rename to `); the wording + stays engine-neutral. + +## 4. Architecture & change list (file by file) + +- **`src/dsl/command.rs`**: `AlterTableAction::RenameTable { new: String }`. +- **`src/dsl/grammar/ddl.rs`**: + - `NEW_TABLE_NAME_IDENT` / `NEW_TABLE_NAME` nodes (≈ near + `AT_RENAME_COLUMN`, mirroring `NEW_COLUMN_NAME` at line 501 + the + `CREATE TABLE` name slot's `reject_internal_table` validator). + - `AT_RENAME_COLUMN_TAIL` (the existing `column …` body minus the + leading `rename`), `AT_RENAME_TABLE_TAIL` (`to `), + `AT_RENAME_TAIL` (`Choice`), `AT_RENAME` (`Seq[rename, tail]`); swap + into `AT_ACTION_CHOICES` (line 1998). + - `build_sql_alter_table` (line 2131): the `rename` branch + doc-comment + update (discrimination now `type → column → add → rename → drop`). +- **`src/db.rs`**: + - `Request::RenameTable` variant (the worker request enum, ≈452–650). + - `Database::rename_table` method (mirror `rename_column`). + - handler dispatch arm (≈ the `RenameColumn` arm) wrapped in + `snapshot_then`. + - `do_rename_table` executor (model: `do_rename_column` at 4314 + + `do_drop_table` at 3227 for the persistence-cleanup shape). Uses an + explicit `do_list_tables` existence/collision check (§2.10), not + `read_schema`-erroring. + - `rewrite_check_table_qualifier` helper near the 4e CHECK tokenizer + (`check_references_column`, `db.rs:5489`); applied in + `do_rename_table` to META + CHECK_TABLE `check_expr` (§2.9). +- **`src/runtime.rs`**: `SqlAlterTable` inner match → `RenameTable` arm. +- **`src/app.rs`**: `build_translate_context` inner match → `RenameTable` + arm (≈1595). +- **`src/friendly/translate.rs`**: `Operation::RenameTable` + `keyword()` + arm `"rename table"`. +- **`src/friendly/strings/en-US.yaml`**: add the `rename to ` + line to the `sql_alter_table` usage; any new refusal-message keys + (same-name / existing-target) — engine-neutral, vocab-audit-clean. + (`parse.usage.sql_alter_table` / `help.ddl.sql_alter_table` keys already + registered in `keys.rs`.) + +## 5. Phase 2 — Candidate approaches (key forks) + +**Rename mechanism.** (M1) the engine's native `ALTER TABLE … RENAME TO` ++ manual metadata UPDATEs *(lead — structure-preserving, atomic, no data +movement; the engine also rewrites child FK references in the live schema +since `legacy_alter_table` is off by default)*. (M2) the ADR-0013 +rebuild-table primitive (create new, copy rows, drop old) — *rejected* +(heavyweight; rebuilds the whole table to change only its name; rename is +not a structural change). (M3) drop + recreate — *rejected* (loses rows; +absurd for a rename). + +**CSV persistence.** (R1) reuse `finalize_persistence` with +`rewritten_tables=[new]` + `deleted_tables=[old]` *(lead — zero new +machinery; re-serialises by the new name, deletes the old, handles +empty-table-no-CSV for free)*. (R-alt) add a `Persistence::rename_table_data` ++ a `Changes.renamed_tables` field and `fs::rename` the file — +*rejected* (new public method + new struct field for behaviour the +existing rewrite+delete path already delivers; byte-copy buys nothing over +re-serialisation since rows come from the db). (R3) leave the CSV under +the old name and special-case the loader — *rejected* (breaks the +`data/
.csv` invariant; confuses a human reading the project dir). + +**Grammar.** (G1) split `rename` into one branch with an inner `Choice` +on the distinct second keyword (`column` / `to`) *(lead — the established +§6.1 + 4g pattern; trap-safe)*. (G2) two sibling `rename`-led branches in +`AT_ACTION_CHOICES` — *rejected* (the walker `Choice` does not backtrack +between same-leading-keyword branches; this is exactly the 4g trap). (G3) +make the `column` keyword optional and disambiguate purely in the builder +— *rejected* (ambiguous grammar; the optional-keyword shape invites the +same backtracking trap and muddies completion). + +**Target `__rdbms_*` refusal.** (V1) parse-time validator on the +new-name slot (mirrors `CREATE TABLE`) **and** an executor guard *(lead — +earliest feedback + defense in depth; the worker is directly reachable by +synthesised commands/tests)*. (V2) executor guard only — *weaker* (loses +the pre-submit `[ERR]` indicator the CREATE name slot gives). (V3) +parse-only — *rejected* (a synthesised `RenameTable` command would slip a +metadata-table rename past the guard). + +## 6. Phase 3 — Selection + +**M1 + R1 + G1 + V1.** Satisfies every §3 item with the smallest faithful +change: native rename is the one new low-level op §6 calls for; the CSV +rename rides the existing rewrite+delete path (no new persistence surface); +the grammar mirrors the trap-safe 4g restructure; the target gets the same +parse-time `__rdbms_*` refusal as `CREATE TABLE` plus an executor guard. +The same-name and existing-target refusals (user-confirmed / conventional) +keep the surface honest; auto-named index *and* relationship names are +left as-is per the ADR §6 scope and the user decision; and the CHECK-text +rewrite (§2.9) keeps the stored metadata in step with the live schema so +a fresh rebuild round-trips. + +## 7. Devil's Advocate review of this plan + +**Planning `/runda` pass (2026-05-26) — three findings, all resolved:** + +- **Finding 1 (BLOCKING, resolved → rewrite).** CHECK-expression text + drift: empirically confirmed that SQLite (3.48, `legacy_alter_table` + off) rewrites a table-qualified column reference in the renamed table's + *live* CHECK (`T.age`→`"U".age`), while the *stored* `check_expr` would + stay `T.age` and break a fresh rebuild. The original plan was silent on + it. **Resolved** by §2.9 (rewrite the stored CHECK text in both + metadata tables), user-confirmed; bounded because a CHECK can only + reference its own table; regression test added (§3). This is the exact + class as the `50a889e` rebuild-metadata bug and the 4e column-CHECK + drift — the `/runda` "probe, don't reason" pass earned its keep again. +- **Finding 2 (correction, resolved).** `read_schema` does not error on a + missing table; the existence/“no such table” guard is now an explicit + `do_list_tables` check (§2.10). +- **Finding 3 (consistency, resolved → leave stale).** Auto-named + relationships embed the table name (`db.rs:5982`) exactly like auto- + named indexes; both are left stale on rename per the user decision, + with the UNIQUE/global-name collision caveat documented. + +- **Forks escalated?** All four genuine forks (same-name behaviour; + auto-named-label handling; CHECK-text drift; relationship-vs-index + consistency) were put to the user (2026-05-26) and answered. The + conventional edges (existing-target refuse; FK-parent/child allowed; + auto-show; target-name validation) are decided-and-noted with + rationale, inviting correction. No silent autonomous design call. ✓ +- **Grammar trap (the recurring 4g bite)?** The two `rename` tails lead + on **distinct** keywords (`column` vs `to`) under one `rename` branch — + exactly the §6.1 rule. A parse test for both tails + the four other + actions guards it. ✓ +- **New-name role collision?** The target binds a *distinct* role + (`new_table_name`), so `require_ident` cannot confuse it with the + `table_name` target slot. ✓ +- **Fresh-rebuild metadata loss (the `50a889e` class)?** 4h changes + metadata *values* (renames), not the schema — `do_rebuild_from_text` + already wipes + repopulates META/REL/CHECK from yaml, and the yaml is + rewritten under the new name by `schema_dirty`. A fresh-rebuild test is + mandatory (§3) and is the precise probe for this class. ✓ +- **Both relationship ends + self-ref?** Two UPDATEs (`parent_table`, + `child_table`); a self-referential table updates both with no + `REL_TABLE` PK `(child_table, child_column)` conflict (single row, + `new` was not previously a child_table). Explicit self-ref test. ✓ +- **CSV fidelity (NULL vs empty, empty tables)?** Re-serialised from the + db, not byte-copied — NULL stays NULL; an empty renamed table writes no + `.csv` and deletes `.csv` (the existing empty-→-delete rule). + Test renames a table with a NULL cell. ✓ +- **FK enforcement after rename?** `legacy_alter_table` is off, so the + native rename rewrites child FK references in the live schema; the + metadata UPDATE keeps `REL_TABLE` consistent; rebuild regenerates FK + DDL from the updated metadata. Tests assert enforcement under the new + name + a rebuild round-trip. ✓ +- **Engine neutrality?** The same-name / existing-target refusals are + authored engine-neutral ("table", "the database"); the native rename's + own collision error is pre-empted by the explicit `do_list_tables` + check so the engine wording never surfaces. Vocab audit + catalog + lockstep guard it. ✓ +- **One undo step?** One executor call = one `snapshot_then` = one + whole-project snapshot. e2e undo/redo test. ✓ +- **Anything dropped?** Auto-named index refresh (out of scope, user + decision, noted as a possible 4i/follow-up). No silent drops. + +## 8. Implementation sequence (test-first) + +1. **Command + Operation + grammar + builder.** Add `RenameTable` + variant, `Operation::RenameTable`, the `NEW_TABLE_NAME` node + the + `AT_RENAME` split, the builder branch. Tier-1 parse tests + (rename-table vs rename-column dispatch; the four other actions; + target `__rdbms_*` refusal) → red, then exhaustive arms (compiler + finds `runtime.rs` / `app.rs` / `keyword()`) → green (parse only). +2. **CHECK-text rewrite helper.** `rewrite_check_table_qualifier` (§2.9) + with unit tests first (bare/quoted/case/string-literal/bare-column/ + unqualified) → red → green. Isolated, lands before the executor uses + it. +3. **Executor + worker wiring.** `do_rename_table` (explicit existence + check; metadata UPDATEs; the CHECK-text rewrite over META + CHECK_TABLE), + `Request::RenameTable`, `Database::rename_table`, the `snapshot_then` + handler arm, the `runtime.rs` + `app.rs` arms. Tier-3 e2e (rows/CSV, FK + parent, FK child, self-ref, table-CHECK, **table-qualified CHECK + + fresh rebuild** (the Finding-1 regression), index, fresh rebuild, + undo/redo, all four refusals) → red where they exercise new behaviour, + then green. +4. **Catalog + docs.** Refresh `sql_alter_table` usage (`rename to`); + add refusal-message keys; ADR-0035 Status + §13 4h; README; + `requirements.md` `Q1`/`C1` — all lockstep. +5. **Full sweep.** `cargo test` (no regression from 1885) + `cargo + clippy --all-targets -- -D warnings`. +6. **Finished-slice `/runda`** (per handoff §7 — budget at least one, + covering 4h; it found the rebuild bug last session). Fix anything it + surfaces, re-green. +7. **Commit proposal** — propose the message, wait for approval. No AI + attribution. (Push is the user's step.) + +## 9. Exit gate + +- All §3 items satisfied; all tiers green, zero skips; no regression from + 1885; written-DA / `/runda` PASS; clippy clean; ADR-0035 §13 4h + README + + `requirements.md` lockstep. After 4h, only **4i** (the verification + sweep) remains to complete Phase 4. diff --git a/docs/requirements.md b/docs/requirements.md index 0292400..7f5767e 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -150,7 +150,10 @@ handoff-14 cleanup; 449 after B2/C2.) ## DSL data commands - [ ] **C1** Table operations: create / drop / rename. - *(Progress: create + drop done; rename pending.)* + *(Progress: create + drop done; **rename done on the advanced + surface** — `ALTER TABLE … RENAME TO`, ADR-0035 §6 / 4h. A simple-mode + rename-table verb is deliberately not provided — table rename is + advanced-mode only.)* - [x] **C2** Column operations: add / drop / rename / change type. `drop column` and `rename column` use SQLite native ALTER TABLE (3.35+ / 3.25+); `change column` routes through @@ -247,7 +250,13 @@ handoff-14 cleanup; 449 after B2/C2.) `name` column on `__rdbms_playground_table_checks` + a `project.yaml` `check_constraints` `{expr, name}` extension; the internal-table guard completed across `do_add_constraint`/`do_add_relationship`)). - Remaining DDL — `ALTER TABLE … RENAME TO` (4h) — is phased per + then `ALTER TABLE … RENAME TO` (4h — the one genuinely new low-level + op, `do_rename_table`: native rename + one-transaction reconciliation of + the CSV file and every metadata row naming the table, incl. **rewriting + CHECK text that qualifies a column with the old table name** so a fresh + rebuild round-trips; refuses same-name / existing-target / `__rdbms_*` / + non-existent; auto-named indexes + relationships kept stale per §6 + scope; one undo step). Remaining: the 4i verification sweep per ADR-0035 §13.)* - [ ] **Q2** Non-standard syntax rejected with a clear message pointing at the supported subset. diff --git a/src/app.rs b/src/app.rs index b5e2f21..02a29f3 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1608,6 +1608,12 @@ impl App { AlterTableAction::DropConstraint { .. } => { (Operation::DropConstraint, Some(table.as_str()), None) } + // `RENAME TO ` — the failure concerns the table being + // renamed (the old name); the executor authors the + // existing-target / same-name refusals (ADR-0035 §6, 4h). + AlterTableAction::RenameTable { .. } => { + (Operation::RenameTable, Some(table.as_str()), None) + } }, C::SqlCreateTable { name, .. } => { (Operation::CreateTable, Some(name.as_str()), None) diff --git a/src/db.rs b/src/db.rs index 5a9b61d..b893308 100644 --- a/src/db.rs +++ b/src/db.rs @@ -528,6 +528,13 @@ enum Request { source: Option, reply: oneshot::Sender>, }, + /// `ALTER TABLE
RENAME TO ` (ADR-0035 §6, 4h). + RenameTable { + table: String, + new: String, + source: Option, + reply: oneshot::Sender>, + }, ChangeColumnType { table: String, column: String, @@ -1209,6 +1216,24 @@ impl Database { recv.await.map_err(|_| DbError::WorkerGone)? } + /// `ALTER TABLE
RENAME TO ` (ADR-0035 §6, 4h). + pub async fn rename_table( + &self, + table: String, + new: String, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::RenameTable { + table, + new, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + pub async fn change_column_type( &self, table: String, @@ -2050,6 +2075,20 @@ fn handle_request( &new, )); } + Request::RenameTable { + table, + new, + source, + reply, + } => { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || do_rename_table( + conn, + persistence, + source.as_deref(), + &table, + &new, + )); + } Request::ChangeColumnType { table, column, @@ -4414,6 +4453,188 @@ fn do_rename_column( Ok(description) } +/// Rename a table (ADR-0035 §6, sub-phase 4h) — the one genuinely new +/// low-level op in Phase 4. +/// +/// Uses SQLite's native `ALTER TABLE … RENAME TO` (structure-preserving, +/// so no rebuild), which also rewrites references to the old name in +/// other tables' FK declarations and inside the renamed table's own +/// CHECK / self-FK definitions (the engine's modern, non-legacy +/// behaviour). We then reconcile everything the engine does *not* know +/// about, all in one transaction (commit-db-last, ADR-0015 §6): +/// +/// 1. every metadata row that names the table — `__rdbms_playground_columns` +/// (`table_name`), **both ends** of `__rdbms_playground_relationships` +/// (`parent_table` *and* `child_table`, covering a self-referential +/// table), and `__rdbms_playground_table_checks` (`table_name`); +/// 2. CHECK *text* that qualifies a column with the old table name +/// (`T.age` → `U.age`), in both metadata tables — the live schema was +/// already rewritten, so the stored text must match or a rebuild fails; +/// 3. the CSV file, via the existing persistence rewrite+delete path +/// (`rewritten_tables = [new]`, `deleted_tables = [old]`). +/// +/// Auto-named indexes and relationships keep their (now stale but +/// functional) names — only the table-name *columns* update (ADR-0035 §6 +/// scope; user-confirmed). One undo step (the worker's whole-project +/// snapshot). +fn do_rename_table( + conn: &Connection, + persistence: Option<&Persistence>, + source: Option<&str>, + old: &str, + new: &str, +) -> Result { + reject_internal_table_name(old)?; + reject_internal_table_name(new)?; + + // Existence + collision: `read_schema` does not error on a missing + // table (`pragma_table_info` returns no rows), so check explicitly — + // and pre-empt the engine's own collision error so the refusal stays + // engine-neutral (ADR-0035 §9). + let tables = do_list_tables(conn)?; + if !tables.iter().any(|t| t == old) { + return Err(DbError::Sqlite { + message: format!("no such table: {old}"), + kind: SqliteErrorKind::NoSuchTable, + }); + } + if old == new { + return Err(DbError::Unsupported(format!( + "rename: new name is identical to the existing one (`{old}`)." + ))); + } + // The database matches table names case-insensitively, so collision + // checks must be case-insensitive too — otherwise the native rename + // surfaces a raw engine collision error (ADR-0035 §9). A target that + // differs from the source only in capitalization is a no-op the engine + // cannot perform; a target colliding with a *different* table is the + // ordinary "already exists" refusal. + if old.eq_ignore_ascii_case(new) { + return Err(DbError::Unsupported(format!( + "`{old}` and `{new}` differ only in capitalization; the database \ + treats them as the same table, so there is nothing to rename." + ))); + } + if tables.iter().any(|t| t.eq_ignore_ascii_case(new)) { + return Err(DbError::Unsupported(format!( + "table `{new}` already exists; pick a different name." + ))); + } + + let tx = conn + .unchecked_transaction() + .map_err(DbError::from_rusqlite)?; + let ddl = format!( + "ALTER TABLE {old_t} RENAME TO {new_t};", + old_t = quote_ident(old), + new_t = quote_ident(new), + ); + debug!(ddl = %ddl, "rename_table"); + tx.execute_batch(&ddl).map_err(DbError::from_rusqlite)?; + + // (1) Rename the table name in every metadata row that names it. + tx.execute( + &format!("UPDATE {META_TABLE} SET table_name = ?1 WHERE table_name = ?2;"), + [new, old], + ) + .map_err(DbError::from_rusqlite)?; + // Both relationship ends — parent and child — so an FK parent, an FK + // child, and a self-referencing table are all covered. The + // relationship `name` is left as-is (auto-names embed the old table + // name; stale-but-functional per the user decision, like index names). + tx.execute( + &format!("UPDATE {REL_TABLE} SET parent_table = ?1 WHERE parent_table = ?2;"), + [new, old], + ) + .map_err(DbError::from_rusqlite)?; + tx.execute( + &format!("UPDATE {REL_TABLE} SET child_table = ?1 WHERE child_table = ?2;"), + [new, old], + ) + .map_err(DbError::from_rusqlite)?; + tx.execute( + &format!("UPDATE {CHECK_TABLE} SET table_name = ?1 WHERE table_name = ?2;"), + [new, old], + ) + .map_err(DbError::from_rusqlite)?; + + // (2) Reconcile CHECK *text* that qualifies a reference with the old + // table name. Read the rows (now keyed by `new`), rewrite, write back + // only when changed — the common unqualified CHECK is a no-op. + let col_checks: Vec<(String, String)> = { + let mut stmt = tx + .prepare(&format!( + "SELECT column_name, check_expr FROM {META_TABLE} \ + WHERE table_name = ?1 AND check_expr IS NOT NULL;" + )) + .map_err(DbError::from_rusqlite)?; + let rows = stmt + .query_map([new], |r| Ok((r.get::<_, String>(0)?, r.get::<_, String>(1)?))) + .map_err(DbError::from_rusqlite)?; + let mut v = Vec::new(); + for row in rows { + v.push(row.map_err(DbError::from_rusqlite)?); + } + v + }; + for (column_name, expr) in col_checks { + let rewritten = rewrite_check_table_qualifier(&expr, old, new); + if rewritten != expr { + tx.execute( + &format!( + "UPDATE {META_TABLE} SET check_expr = ?1 \ + WHERE table_name = ?2 AND column_name = ?3;" + ), + rusqlite::params![rewritten, new, column_name], + ) + .map_err(DbError::from_rusqlite)?; + } + } + let table_checks: Vec<(i64, String)> = { + let mut stmt = tx + .prepare(&format!( + "SELECT seq, check_expr FROM {CHECK_TABLE} WHERE table_name = ?1;" + )) + .map_err(DbError::from_rusqlite)?; + let rows = stmt + .query_map([new], |r| Ok((r.get::<_, i64>(0)?, r.get::<_, String>(1)?))) + .map_err(DbError::from_rusqlite)?; + let mut v = Vec::new(); + for row in rows { + v.push(row.map_err(DbError::from_rusqlite)?); + } + v + }; + for (seq, expr) in table_checks { + let rewritten = rewrite_check_table_qualifier(&expr, old, new); + if rewritten != expr { + tx.execute( + &format!( + "UPDATE {CHECK_TABLE} SET check_expr = ?1 \ + WHERE table_name = ?2 AND seq = ?3;" + ), + rusqlite::params![rewritten, new, seq], + ) + .map_err(DbError::from_rusqlite)?; + } + } + + let description = do_describe_table(conn, new)?; + // (3) CSV follows the table: write `data/.csv` from the renamed + // table (read in-tx by its new name) and delete `data/.csv` — the + // existing rewrite+delete path; an empty table writes no CSV on either + // side. `schema_dirty` rewrites `project.yaml`, which reflects the new + // name automatically. + let changes = Changes { + schema_dirty: true, + rewritten_tables: vec![new.to_string()], + deleted_tables: vec![old.to_string()], + }; + finalize_persistence(conn, persistence, source, &changes)?; + tx.commit().map_err(DbError::from_rusqlite)?; + Ok(description) +} + /// Change a column's type. /// /// Change a column's user-facing type, per ADR-0017's per-cell @@ -5541,6 +5762,141 @@ mod check_references_column_tests { } } +/// Rewrite the old table name where it is used as a **qualifier** in a +/// CHECK expression (the identifier immediately followed by `.`) to the +/// new name — both the bare (`old.col`) and double-quoted (`"old".col`) +/// forms, case-insensitively — leaving everything else byte-for-byte +/// intact (ADR-0035 §6, sub-phase 4h). +/// +/// **Why this is needed.** A native `ALTER TABLE old RENAME TO new` +/// rewrites table-qualified column references inside the renamed table's +/// *live* CHECK (`CHECK (T.age>0)` → `CHECK ("U".age>0)`), but our +/// *stored* CHECK text would keep the old name and break a later rebuild +/// (`schema_to_ddl` would emit `CHECK (T.age>0)` for a table now named +/// `U` → "no such table T"). This keeps the stored text in step with the +/// live schema. **Bounded problem:** a CHECK may reference only its own +/// table's columns (SQLite forbids subqueries / other tables in a CHECK), +/// so the only table qualifier that can appear is the renamed table's +/// name — the rewrite target is unambiguous. +/// +/// Extends the [`check_references_column`] tokenizer: skips single-quoted +/// string literals (a literal containing the old name is untouched), and +/// a bare identifier equal to the old name but **not** used as a qualifier +/// (not followed by `.` — e.g. a column literally named like the table) +/// is left alone, so the common unqualified CHECK (`age > 0`) is a no-op. +fn rewrite_check_table_qualifier(check_expr: &str, old: &str, new: &str) -> String { + use crate::dsl::walker::lex_helpers::{consume_ident, consume_string_literal}; + let bytes = check_expr.as_bytes(); + // Whether the next byte at `pos` begins a `.` qualifier separator. + let dot_follows = |pos: usize| bytes.get(pos) == Some(&b'.'); + let mut out = String::with_capacity(check_expr.len()); + let mut i = 0; + while i < check_expr.len() { + // Single-quoted string literal — copy verbatim, never rewrite. + if let Some(((start, end), _)) = consume_string_literal(check_expr, i) { + out.push_str(&check_expr[start..end]); + i = end; + continue; + } + // Double-quoted identifier — scan to the closing quote (`""` is an + // escaped quote). Rewrite when it is the old name used as a + // qualifier; table names never contain quotes, so the raw inner + // text suffices for comparison. + if bytes[i] == b'"' { + let mut j = i + 1; + while j < check_expr.len() { + if bytes[j] == b'"' { + if bytes.get(j + 1) == Some(&b'"') { + j += 2; // escaped quote inside the identifier + continue; + } + break; // closing quote + } + j += 1; + } + let end = (j + 1).min(check_expr.len()); // byte after closing `"` + let inner = &check_expr[i + 1..j.min(check_expr.len())]; + if inner.eq_ignore_ascii_case(old) && dot_follows(end) { + out.push('"'); + out.push_str(new); + out.push('"'); + } else { + out.push_str(&check_expr[i..end]); + } + i = end; + continue; + } + // Bare identifier. + if let Some((start, end)) = consume_ident(check_expr, i) { + if check_expr[start..end].eq_ignore_ascii_case(old) && dot_follows(end) { + out.push_str(new); + } else { + out.push_str(&check_expr[start..end]); + } + i = end; + continue; + } + // Operator / paren / number / punctuation — copy one char. + let ch_len = check_expr[i..].chars().next().map_or(1, char::len_utf8); + out.push_str(&check_expr[i..i + ch_len]); + i += ch_len; + } + out +} + +#[cfg(test)] +mod rewrite_check_table_qualifier_tests { + use super::rewrite_check_table_qualifier; + + #[test] + fn rewrites_a_bare_qualifier() { + assert_eq!(rewrite_check_table_qualifier("T.age > 0", "T", "U"), "U.age > 0"); + // multiple occurrences, table-level CHECK shape + assert_eq!(rewrite_check_table_qualifier("T.a <> T.b", "T", "U"), "U.a <> U.b"); + } + + #[test] + fn is_case_insensitive_on_the_qualifier() { + assert_eq!(rewrite_check_table_qualifier("t.age > 0", "T", "U"), "U.age > 0"); + } + + #[test] + fn rewrites_a_quoted_qualifier() { + assert_eq!( + rewrite_check_table_qualifier("\"T\".age > 0", "T", "U"), + "\"U\".age > 0" + ); + } + + #[test] + fn leaves_string_literals_untouched() { + assert_eq!( + rewrite_check_table_qualifier("note <> 'T.x' AND T.age > 0", "T", "U"), + "note <> 'T.x' AND U.age > 0" + ); + } + + #[test] + fn leaves_a_bare_name_that_is_not_a_qualifier() { + // A column literally named like the table, not followed by `.`. + assert_eq!(rewrite_check_table_qualifier("T > 0", "T", "U"), "T > 0"); + } + + #[test] + fn unqualified_check_is_a_no_op() { + assert_eq!(rewrite_check_table_qualifier("age > 0", "T", "U"), "age > 0"); + } + + #[test] + fn does_not_match_a_longer_identifier() { + // `Total` merely starts with `T`; not the qualifier `T`. + assert_eq!( + rewrite_check_table_qualifier("Total.x > 0", "T", "U"), + "Total.x > 0" + ); + } +} + /// 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 diff --git a/src/dsl/command.rs b/src/dsl/command.rs index f461dfc..481723b 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -753,6 +753,12 @@ pub enum AlterTableAction { /// `DROP CONSTRAINT ` — drops a named table-level CHECK or a /// named FK (relationship), resolved by name (ADR-0035 §4g). DropConstraint { name: String }, + /// `RENAME TO ` — rename the table (ADR-0035 §6, sub-phase 4h). + /// The one genuinely new low-level op in Phase 4: a native table + /// rename plus reconciliation of the CSV file name and every metadata + /// row that names the table (columns, both relationship ends, table + /// CHECKs, and any table-qualified CHECK *text*). Advanced-mode only. + RenameTable { new: String }, } /// A table-level constraint added via `ALTER TABLE … ADD [CONSTRAINT diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 8ce76ff..33c60ff 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -1904,14 +1904,49 @@ const AT_ADD_COLUMN_TAIL: Node = Node::Seq(AT_ADD_COLUMN_TAIL_NODES); static AT_DROP_COLUMN_TAIL_NODES: &[Node] = &[Node::Word(Word::keyword("column")), COLUMN_NAME]; const AT_DROP_COLUMN_TAIL: Node = Node::Seq(AT_DROP_COLUMN_TAIL_NODES); -static AT_RENAME_COLUMN_NODES: &[Node] = &[ - Node::Word(Word::keyword("rename")), +// New-table-name slot for `RENAME TO ` (ADR-0035 §6, sub-phase 4h). +// Mirrors the `CREATE TABLE` name slot: `IdentSource::NewName` (a name +// being introduced, not completed from existing tables) + the same +// `reject_internal_table` parse-time validator, so an `__rdbms_*` target +// is refused before submit. Wrapped in `NEW_NAME_HINT` like +// `NEW_COLUMN_NAME`. `writes_table: false` — nothing downstream of +// `rename to ` references the schema cache. +const NEW_TABLE_NAME_IDENT: Node = Node::Ident { + source: IdentSource::NewName, + role: "new_table_name", + validator: Some(super::sql_select::reject_internal_table), + highlight_override: None, + writes_table: false, + writes_column: false, + writes_user_listed_column: false, + writes_table_alias: false, + writes_cte_name: false, + writes_projection_alias: false, +}; +const NEW_TABLE_NAME: Node = Node::Hinted { + mode: NEW_NAME_HINT, + inner: &NEW_TABLE_NAME_IDENT, +}; + +// The `rename` verb fans out (like `add`/`drop`, §6.1) to an inner +// `Choice` whose two tails lead on DISTINCT second keywords: `column` +// (rename column) and `to` (rename table — 4h). The walker `Choice` +// selects by the leading token and never backtracks between branches, so +// the distinct keywords keep them apart. +static AT_RENAME_COLUMN_TAIL_NODES: &[Node] = &[ 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); +const AT_RENAME_COLUMN_TAIL: Node = Node::Seq(AT_RENAME_COLUMN_TAIL_NODES); +static AT_RENAME_TABLE_TAIL_NODES: &[Node] = + &[Node::Word(Word::keyword("to")), NEW_TABLE_NAME]; +const AT_RENAME_TABLE_TAIL: Node = Node::Seq(AT_RENAME_TABLE_TAIL_NODES); +static AT_RENAME_TAIL_CHOICES: &[Node] = &[AT_RENAME_COLUMN_TAIL, AT_RENAME_TABLE_TAIL]; +const AT_RENAME_TAIL: Node = Node::Choice(AT_RENAME_TAIL_CHOICES); +static AT_RENAME_NODES: &[Node] = &[Node::Word(Word::keyword("rename")), AT_RENAME_TAIL]; +const AT_RENAME: Node = Node::Seq(AT_RENAME_NODES); // `ALTER COLUMN TYPE ` (ADR-0035 §4f). The type slot reuses // SQL_TYPE (the same alias map + `double precision` pair the CREATE @@ -1995,7 +2030,7 @@ const AT_DROP: Node = Node::Seq(AT_DROP_NODES); // concrete keywords, trap-safe. (The branch's `alter` is the action // word; the entry-word `alter` was already consumed by dispatch.) The // second-keyword fan-out happens in `AT_ADD` / `AT_DROP`'s inner Choice. -static AT_ACTION_CHOICES: &[Node] = &[AT_ADD, AT_DROP, AT_RENAME_COLUMN, AT_ALTER_COLUMN]; +static AT_ACTION_CHOICES: &[Node] = &[AT_ADD, AT_DROP, AT_RENAME, AT_ALTER_COLUMN]; const AT_ACTION: Node = Node::Choice(AT_ACTION_CHOICES); static SQL_ALTER_TABLE_SHAPE_NODES: &[Node] = &[ @@ -2127,7 +2162,12 @@ fn build_alter_column_type(path: &MatchedPath) -> Result`. +/// 4. **`rename`** — `rename to ` (table rename, 4h). Reached only +/// when `column` is absent (caught by step 2), so a lone `rename` +/// means the table form. The new name binds a *distinct* role +/// (`new_table_name`), so it never collides with the `table_name` +/// target slot. +/// 5. else **`drop`** — `drop constraint `. fn build_sql_alter_table(path: &MatchedPath, source: &str) -> Result { let table = require_ident(path, "table_name")?; let action = if path.contains_word("type") { @@ -2147,6 +2187,10 @@ fn build_sql_alter_table(path: &MatchedPath, source: &str) -> Result` — the `rename` verb fans out + // on a distinct second keyword (`to` vs `column`). + let (table, action) = alter("alter table Orders rename to Purchases"); + assert_eq!(table, "Orders"); + match action { + AlterTableAction::RenameTable { new } => assert_eq!(new, "Purchases"), + other => panic!("expected RenameTable, got {other:?}"), + } + // trailing semicolon tolerated + assert!(matches!( + alter("alter table Orders rename to Purchases;").1, + AlterTableAction::RenameTable { .. } + )); + } + + #[test] + fn rename_table_does_not_steal_rename_column() { + // The two `rename` tails coexist: `rename to` → table, + // `rename column … to …` → column. Neither misroutes. + assert!(matches!( + alter("alter table T rename to U").1, + AlterTableAction::RenameTable { .. } + )); + assert!(matches!( + alter("alter table T rename column a to b").1, + AlterTableAction::RenameColumn { .. } + )); + } + + #[test] + fn rename_to_internal_target_refused_at_parse() { + // The target slot carries the `reject_internal_table` validator + // (mirroring CREATE TABLE), so an `__rdbms_*` target is refused + // before submit — engine-neutral, not a raw engine error. + assert!(parse_command_in_mode("alter table T rename to __rdbms_evil", Mode::Advanced).is_err()); + } + #[test] fn alter_column_type_parses() { // ADR-0035 §4f: the fourth action, discriminated by the `type` diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index bad5bf4..c4b0aab 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -274,6 +274,7 @@ help: alter table add column [not null] [unique] [default …] [check …] alter table drop column alter table rename column to + alter table rename to alter table alter column type alter table add [constraint ] check () | unique (, …) | foreign key () references

[(

)] alter table drop constraint — evolve a table's columns and constraints (advanced SQL) @@ -482,6 +483,7 @@ parse: alter table
add column [not null] [unique] [default ] [check ()] alter table
drop column alter table
rename column to + alter table
rename to alter table
alter column type alter table
add [constraint ] check () | unique (, ...) | foreign key () references [()] alter table
drop constraint diff --git a/src/friendly/translate.rs b/src/friendly/translate.rs index 938452a..e89dd3b 100644 --- a/src/friendly/translate.rs +++ b/src/friendly/translate.rs @@ -62,6 +62,7 @@ pub enum Operation { AddColumn, DropColumn, RenameColumn, + RenameTable, ChangeColumnType, AddRelationship, DropRelationship, @@ -93,6 +94,7 @@ impl Operation { Self::AddColumn => "add column", Self::DropColumn => "drop column", Self::RenameColumn => "rename column", + Self::RenameTable => "rename table", Self::ChangeColumnType => "change column", Self::AddRelationship => "add relationship", Self::DropRelationship => "drop relationship", diff --git a/src/runtime.rs b/src/runtime.rs index b0080e0..7367b9b 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -2149,6 +2149,13 @@ async fn execute_command_typed( .alter_drop_constraint(table, name, src) .await .map(CommandOutcome::Schema), + // `RENAME TO ` — the one genuinely new low-level op + // (ADR-0035 §6, 4h): native table rename + CSV + metadata + // reconciliation, one undo step. + AlterTableAction::RenameTable { new } => database + .rename_table(table, new, src) + .await + .map(|d| CommandOutcome::Schema(Some(d))), }, Command::AddConstraint { table, diff --git a/tests/sql_alter_table.rs b/tests/sql_alter_table.rs index 3db690b..72119d7 100644 --- a/tests/sql_alter_table.rs +++ b/tests/sql_alter_table.rs @@ -652,3 +652,426 @@ fn e2e_named_check_metadata_survives_a_fresh_rebuild() { )) .expect("DROP CONSTRAINT after a fresh rebuild — the CHECK metadata was reconstructed"); } + +// --- 4h: ALTER TABLE … RENAME TO (ADR-0035 §6) -------------------------- + +/// Path to a table's CSV in the project data dir. +fn csv_path(project: &project::Project, table: &str) -> std::path::PathBuf { + project + .path() + .join(project::DATA_DIR) + .join(format!("{table}.csv")) +} + +/// Drop the current db handle, delete the `.db`, reopen, and rebuild from +/// the text artifacts (`project.yaml` + CSVs) only — the FRESH rebuild +/// that re-emits DDL from stored metadata via `schema_to_ddl`. This is +/// where the CHECK-text drift (Finding-1) and the FK / metadata +/// reconciliation actually round-trip, unlike an in-place rebuild whose +/// wipe leaves the user tables untouched. +fn fresh_rebuild( + old: Database, + project: &project::Project, + r: &tokio::runtime::Runtime, +) -> Database { + use rdbms_playground::project::PLAYGROUND_DB; + // Drop only the db handle (release the .db file) and reuse the live + // `project` — re-opening the Project would re-acquire its lock file, + // which the still-alive `project` already holds. The Database does not + // hold the project lock; only the Project does. + drop(old); + std::fs::remove_file(project.path().join(PLAYGROUND_DB)).expect("remove db"); + let db = Database::open_with_persistence( + project.db_path(), + Persistence::new(project.path().to_path_buf()), + ) + .expect("db"); + r.block_on(db.rebuild_from_text(project.path().to_path_buf(), None)) + .expect("rebuild"); + db +} + +fn table_names(db: &Database, r: &tokio::runtime::Runtime) -> Vec { + r.block_on(db.list_tables()).expect("list_tables") +} + +#[test] +fn e2e_rename_table_with_rows_csv_follows_and_survives_rebuild() { + // The CSV file follows the rename (data/.csv written, .csv + // removed), rows are intact including a NULL (NULL-vs-empty fidelity), + // and the renamed table round-trips through a FRESH rebuild. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table Orders with pk id(int)\n\ + add column Orders: note (text)\n\ + insert into Orders (id, note) values (1, 'first')\n\ + insert into Orders (id) values (2)\n\ + alter table Orders rename to Purchases\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + match events.last().expect("event") { + AppEvent::ReplayCompleted { count, .. } => { + assert_eq!(*count, 5, "all five lines replayed; events: {events:?}"); + } + other => panic!("expected ReplayCompleted, got {other:?} (events: {events:?})"), + } + + let tables = table_names(&db, &r); + assert!( + tables.contains(&"Purchases".to_string()) && !tables.contains(&"Orders".to_string()), + "the table is now Purchases, not Orders: {tables:?}" + ); + assert!(csv_path(&project, "Purchases").exists(), "data/Purchases.csv written"); + assert!(!csv_path(&project, "Orders").exists(), "data/Orders.csv removed"); + + let rows = r + .block_on(db.query_data("Purchases".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows.len(), 2); + assert_eq!(rows[0][1].as_deref(), Some("first")); + assert_eq!(rows[1][1], None, "the NULL note survived the rename"); + + // FRESH rebuild — the renamed table + its rows reconstruct from text. + let db = fresh_rebuild(db, &project, &r); + let tables = table_names(&db, &r); + assert!( + tables.contains(&"Purchases".to_string()) && !tables.contains(&"Orders".to_string()), + "Purchases round-tripped through a fresh rebuild: {tables:?}" + ); + let rows = r + .block_on(db.query_data("Purchases".to_string(), None, None, None)) + .expect("query") + .rows; + assert_eq!(rows.len(), 2); + assert_eq!(rows[1][1], None, "NULL preserved across the rebuild"); +} + +#[test] +fn e2e_rename_table_with_table_qualified_check_survives_fresh_rebuild() { + // Finding-1 regression. A CHECK that qualifies a column with the table + // name (`T.age`, and a table-level `T.lo < T.hi`) drifts on rename: + // the engine rewrites the LIVE CHECK, but the STORED text would stay + // `T.…` and break a FRESH rebuild (`schema_to_ddl` → "no such table + // T"). The §2.9 rewrite keeps the stored text in step. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table T (id integer primary key, age integer check (T.age > 0), lo integer, hi integer, check (T.lo < T.hi))\n\ + insert into T (id, age, lo, hi) values (1, 5, 1, 9)\n\ + alter table T rename to U\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "create with table-qualified CHECKs + rename replayed; events: {events:?}" + ); + + // The live CHECKs still enforce under the new name. + let bad_age = r.block_on(db.insert( + "U".to_string(), + Some(vec!["id".into(), "age".into(), "lo".into(), "hi".into()]), + vec![ + Value::Number("2".into()), + Value::Number("0".into()), + Value::Number("1".into()), + Value::Number("9".into()), + ], + Some("i".into()), + )); + assert!(bad_age.is_err(), "age > 0 still enforced after rename"); + + // The headline: a FRESH rebuild reconstructs from the stored CHECK + // text — which must now reference U, not T — and still enforces. + let db = fresh_rebuild(db, &project, &r); + assert!( + table_names(&db, &r).contains(&"U".to_string()), + "U rebuilt from the text artifacts (would fail on 'no such table T' without the rewrite)" + ); + let bad_after = r.block_on(db.insert( + "U".to_string(), + Some(vec!["id".into(), "age".into(), "lo".into(), "hi".into()]), + vec![ + Value::Number("3".into()), + Value::Number("-1".into()), + Value::Number("1".into()), + Value::Number("9".into()), + ], + Some("i".into()), + )); + assert!(bad_after.is_err(), "the rewritten CHECK enforces after a fresh rebuild"); +} + +#[test] +fn e2e_rename_fk_parent_updates_metadata_and_still_enforces() { + // Renaming an FK *parent* updates the relationship's parent end; the + // child FK still enforces, and the metadata is consistent enough that + // a fresh rebuild (which re-emits the FK DDL from metadata) succeeds. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table P with pk id(int)\n\ + create table C (id integer primary key, p_id integer references P(id))\n\ + insert into P (id) values (1)\n\ + alter table P rename to Parent\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "events: {events:?}" + ); + + // The child's outbound relationship now points at the new parent name. + let c = r.block_on(db.describe_table("C".to_string(), None)).expect("describe C"); + assert_eq!(c.outbound_relationships.len(), 1); + assert_eq!(c.outbound_relationships[0].other_table, "Parent"); + + // FK still enforces: a child row referencing a missing parent fails. + assert!( + r.block_on(db.insert( + "C".to_string(), + Some(vec!["id".into(), "p_id".into()]), + vec![Value::Number("9".into()), Value::Number("99".into())], + Some("i".into()), + )) + .is_err(), + "FK to the renamed parent still enforces" + ); + + // Fresh rebuild re-emits the FK from metadata (parent_table = Parent). + let db = fresh_rebuild(db, &project, &r); + let tables = table_names(&db, &r); + assert!(tables.contains(&"Parent".to_string()) && tables.contains(&"C".to_string())); + assert!( + r.block_on(db.insert( + "C".to_string(), + Some(vec!["id".into(), "p_id".into()]), + vec![Value::Number("8".into()), Value::Number("77".into())], + Some("i".into()), + )) + .is_err(), + "FK still enforces after a fresh rebuild" + ); +} + +#[test] +fn e2e_rename_fk_child_updates_metadata_and_still_enforces() { + // Renaming an FK *child* updates the relationship's child end. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table P with pk id(int)\n\ + create table C (id integer primary key, p_id integer references P(id))\n\ + insert into P (id) values (1)\n\ + alter table C rename to Child\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "events: {events:?}" + ); + + // The parent's inbound relationship now names the renamed child. + let p = r.block_on(db.describe_table("P".to_string(), None)).expect("describe P"); + assert_eq!(p.inbound_relationships.len(), 1); + assert_eq!(p.inbound_relationships[0].other_table, "Child"); + + // FK still enforces under the new child name; survives a fresh rebuild. + assert!( + r.block_on(db.insert( + "Child".to_string(), + Some(vec!["id".into(), "p_id".into()]), + vec![Value::Number("9".into()), Value::Number("99".into())], + Some("i".into()), + )) + .is_err(), + "FK from the renamed child still enforces" + ); + let db = fresh_rebuild(db, &project, &r); + assert!(table_names(&db, &r).contains(&"Child".to_string())); +} + +#[test] +fn e2e_rename_self_referential_table_updates_both_ends() { + // A self-referential FK has parent_table == child_table; both ends + // must update on rename without a relationship-metadata PK conflict. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table N (id integer primary key, parent_id integer references N(id))\n\ + insert into N (id, parent_id) values (1, null)\n\ + alter table N rename to Tree\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "events: {events:?}" + ); + + // Both ends of the self-reference now name `Tree`. + let t = r.block_on(db.describe_table("Tree".to_string(), None)).expect("describe Tree"); + assert_eq!(t.outbound_relationships[0].other_table, "Tree"); + assert_eq!(t.inbound_relationships[0].other_table, "Tree"); + + // The self-FK still enforces and survives a fresh rebuild. + assert!( + r.block_on(db.insert( + "Tree".to_string(), + Some(vec!["id".into(), "parent_id".into()]), + vec![Value::Number("2".into()), Value::Number("99".into())], + Some("i".into()), + )) + .is_err(), + "self-FK to a missing parent row is rejected" + ); + let db = fresh_rebuild(db, &project, &r); + assert!(table_names(&db, &r).contains(&"Tree".to_string())); + // A valid self-reference (parent_id = 1, which exists) is accepted. + r.block_on(db.insert( + "Tree".to_string(), + Some(vec!["id".into(), "parent_id".into()]), + vec![Value::Number("3".into()), Value::Number("1".into())], + Some("i".into()), + )) + .expect("valid self-reference accepted after rebuild"); +} + +#[test] +fn e2e_rename_table_keeps_its_index_with_a_stale_name() { + // Auto-named indexes embed the old table name and are left STALE on + // rename (user decision); the index stays functional and survives a + // fresh rebuild. + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table T with pk id(int)\n\ + add column T: email (text)\n\ + create index on T (email)\n\ + alter table T rename to Users\n", + ) + .expect("write script"); + let events = r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!( + matches!(events.last(), Some(AppEvent::ReplayCompleted { .. })), + "events: {events:?}" + ); + + let u = r.block_on(db.describe_table("Users".to_string(), None)).expect("describe Users"); + assert_eq!(u.indexes.len(), 1, "the index followed the rename"); + assert_eq!( + u.indexes[0].name, "T_email_idx", + "the auto-name is left stale (embeds the old table name) per the user decision" + ); + assert_eq!(u.indexes[0].columns, vec!["email".to_string()]); + + // Survives a fresh rebuild (recreated from IndexSchema on table Users). + let db = fresh_rebuild(db, &project, &r); + let u = r.block_on(db.describe_table("Users".to_string(), None)).expect("describe Users"); + assert_eq!(u.indexes.len(), 1); + assert_eq!(u.indexes[0].name, "T_email_idx"); +} + +#[test] +fn e2e_rename_table_is_one_undo_step() { + // The rename is one user mutation = one whole-project snapshot = one + // undo step. Undo restores the old name and its rows; redo reapplies. + let (project, db, _d) = open_with_undo(); + let r = rt(); + std::fs::write( + project.path().join("rn.commands"), + "create table Orders with pk id(int)\n\ + insert into Orders (id) values (1)\n\ + alter table Orders rename to Purchases\n", + ) + .expect("write script"); + r.block_on(run_replay(&db, project.path(), "rn.commands")); + assert!(table_names(&db, &r).contains(&"Purchases".to_string())); + + // One undo reverts the rename. + assert!(r.block_on(db.undo()).expect("undo").is_some(), "rename was one undo step"); + let tables = table_names(&db, &r); + assert!( + tables.contains(&"Orders".to_string()) && !tables.contains(&"Purchases".to_string()), + "undo restored the old table name: {tables:?}" + ); + assert_eq!( + r.block_on(db.query_data("Orders".to_string(), None, None, None)).expect("query").rows.len(), + 1, + "the row is back under the old name" + ); + + // Redo reapplies the rename. + assert!(r.block_on(db.redo()).expect("redo").is_some()); + let tables = table_names(&db, &r); + assert!( + tables.contains(&"Purchases".to_string()) && !tables.contains(&"Orders".to_string()), + "redo reapplied the rename: {tables:?}" + ); +} + +#[test] +fn e2e_rename_table_refusals() { + // The executor's guards: existing-target, same-name, non-existent + // source, and an internal `__rdbms_*` target (defense in depth — the + // parse validator also refuses it, but a synthesised command reaches + // the worker directly). + let (project, db, _d) = open(); + let r = rt(); + std::fs::write( + project.path().join("setup.commands"), + "create table T with pk id(int)\n\ + create table X with pk id(int)\n", + ) + .expect("write"); + r.block_on(run_replay(&db, project.path(), "setup.commands")); + + assert!( + r.block_on(db.rename_table("T".into(), "X".into(), Some("rn".into()))).is_err(), + "rename to an existing other table is refused" + ); + assert!( + r.block_on(db.rename_table("T".into(), "T".into(), Some("rn".into()))).is_err(), + "rename to the same name is refused" + ); + assert!( + r.block_on(db.rename_table("Ghost".into(), "G".into(), Some("rn".into()))).is_err(), + "rename of a non-existent table is refused" + ); + assert!( + r.block_on(db.rename_table("T".into(), "__rdbms_evil".into(), Some("rn".into()))).is_err(), + "rename to an internal table name is refused at the executor" + ); + + // Case-insensitive collisions are refused with engine-neutral wording + // (not the raw engine "already another table" error) — the database + // matches names case-insensitively (ADR-0035 §9). + let case_only = r.block_on(db.rename_table("T".into(), "t".into(), Some("rn".into()))); + assert!(case_only.is_err(), "a case-only rename is refused"); + if let Err(e) = case_only { + let msg = e.to_string(); + assert!( + !msg.to_lowercase().contains("another table") && !msg.to_lowercase().contains("index"), + "the refusal is engine-neutral, not a raw engine collision error: {msg}" + ); + } + assert!( + r.block_on(db.rename_table("T".into(), "x".into(), Some("rn".into()))).is_err(), + "rename to a name colliding case-insensitively with another table (X) is refused" + ); + + // The failed renames left the schema untouched. + let tables = table_names(&db, &r); + assert!(tables.contains(&"T".to_string()) && tables.contains(&"X".to_string())); +}