diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 11b3b7b..5f5e74e 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -305,20 +305,28 @@ with an explicit exit gate + a written Devil's-Advocate gate, mirroring ADR-0033's structure: - **4a — Dispatch + `CREATE TABLE` core.** Advanced `create` - dispatch; `SqlCreateTable` for columns + types (the §3 map) + - column constraints + single/compound `PRIMARY KEY`, plus - `IF NOT EXISTS` (no-op-with-note, §4). Single-column table-level - `UNIQUE`/`CHECK` normalise into the column; **no FK** (4b). Reuses - `do_create_table`. -- **4a.2 — Composite `UNIQUE(a,b)` / multi-column table `CHECK`.** - Split out (2026-05-24, user-confirmed) because these are the first - structures the data model cannot already represent: `TableSchema` - has no slot for them. A self-contained slice that extends - `TableSchema` + the YAML round-trip + `read_schema` detection + - `do_create_table` DDL emission, with save/load/rebuild tests. (The - general rule: a DDL feature needs data-model work only when it - introduces a structure simple mode could never produce — cf. the - `UNIQUE`-index flag in 4d and the new rename op in 4h.) + dispatch; `SqlCreateTable` for columns + types (the §3 map, incl. the + two-word `double precision` and discarded length args) + the + **clean-reuse column constraints only** — `NOT NULL` / `UNIQUE` / + column-level `PRIMARY KEY` — + single/compound table-level + `PRIMARY KEY`, plus `IF NOT EXISTS` (no-op-with-note, §4). Reuses + `do_create_table` (extended so a `serial` sole-PK inlines `PRIMARY + KEY` in a multi-column table, preserving autoincrement). **No FK** + (4b); **no `DEFAULT`/`CHECK`/table-level `UNIQUE`** (4a.2). +- **4a.2 — The constraint slice.** Split out (2026-05-24, + user-confirmed) for the constraints that are *not* a clean reuse: + (1) **`CHECK`/`DEFAULT`** via the full `sql_expr` surface stored as + **raw SQL text** — needed because `sql_expr` is validate-only and + yields no `Expr` AST for `compile_check_sql`/`ColumnSpec`, so it is a + separate execution path; (2) **composite `UNIQUE(a,b)` and + multi-column table `CHECK`** — the first structures `TableSchema` + cannot already represent, needing a model + YAML round-trip + + `read_schema` detection + `do_create_table` emission extension, with + save/load/rebuild tests. Until then 4a rejects all of these + "not yet supported". (The general rule: a DDL feature needs new + model/execution work only when it introduces a structure simple mode + could never produce, or an expression the structural helper cannot + consume — cf. the `UNIQUE`-index flag in 4d and the rename op in 4h.) - **4b — Foreign keys in `CREATE TABLE`.** Inline `REFERENCES` + table-level `FOREIGN KEY` → relationship metadata, one undo step. - **4c — `DROP TABLE [IF EXISTS]`** → `SqlDropTable` (cascade parity; diff --git a/docs/adr/README.md b/docs/adr/README.md index 77c2910..c96a723 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) — **Proposed** (design agreed 2026-05-24; implementation phased + 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 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 a 4a.2 split for composite `UNIQUE(a,b)` / multi-column table `CHECK` — the first structures `TableSchema` can't already represent, needing a round-trip extension), each with exit + DA gates +- [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Proposed** (design agreed 2026-05-24; implementation phased + 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 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 a 4a.2 **constraint slice** split out for `CHECK`/`DEFAULT` — `sql_expr` is validate-only, yielding no `Expr` AST, so they store as raw SQL text via a separate path — and for composite `UNIQUE(a,b)` / multi-column table `CHECK`, the first structures `TableSchema` can't already represent; 4a rejects these "not yet supported"), each with exit + DA gates diff --git a/docs/plans/20260524-adr-0035-sql-ddl-4a.md b/docs/plans/20260524-adr-0035-sql-ddl-4a.md index d34ec3d..ff077bb 100644 --- a/docs/plans/20260524-adr-0035-sql-ddl-4a.md +++ b/docs/plans/20260524-adr-0035-sql-ddl-4a.md @@ -54,17 +54,16 @@ From the ADR-0035 design (handoff 36 §3) and the pre-implementation **advanced mode** and produces `Command::SqlCreateTable`. - [ ] Simple-mode `create table T with pk …` is **unchanged** and still parses as `Command::CreateTable` (dispatch fallback verified). -- [ ] Column element: ` [constraints…]` where constraints - are `NOT NULL`, `UNIQUE`, `PRIMARY KEY`, `DEFAULT `, - `CHECK ()` — the per-column ADR-0029 set spelled in SQL. -- [ ] Type slot: ten keywords + the §2.5 alias map; length/precision - arg accepted-and-ignored. -- [ ] `INTEGER PRIMARY KEY` → plain `int` PK (no auto-increment). +- [ ] Column element: ` [NOT NULL] [UNIQUE] + [PRIMARY KEY]` — the clean-reuse constraints only. `DEFAULT` / + `CHECK` are **not** in 4a (→ 4a.2, §6.1). +- [ ] Type slot: ten keywords + the §2.5 alias map (incl. the two-word + `double precision`, §6.3); length/precision arg accepted-and-ignored. +- [ ] `INTEGER PRIMARY KEY` → plain `int` PK (no auto-increment); + `serial` PK autoincrements even in a multi-column table (§6.4). - [ ] Table-level `PRIMARY KEY (, …)` — **single and compound**. -- [ ] Single-column table-level `UNIQUE(a)` / `CHECK(expr-over-a)` - accepted by **normalizing into the column spec**; composite - `UNIQUE(a,b)` / multi-column table `CHECK` rejected "not yet - supported" (→ 4a.2, §6). +- [ ] `DEFAULT`, `CHECK`, and any table-level `UNIQUE`/`CHECK` rejected + "not yet supported" (→ 4a.2, §6.1). - [ ] `IF NOT EXISTS` → no-op-with-note on an existing table. - [ ] Structural execution: reuses `do_create_table`; the new object is a first-class playground table — `__rdbms_playground_columns` @@ -126,18 +125,17 @@ Mirror `src/dsl/grammar/sql_insert.rs`: - After `table` keyword: an optional `IF NOT EXISTS` keyword run that sets a flag in the builder. - Column list: a `Repeated` of column-or-table elements inside `( … )`. - - Column element: name slot (`IdentSource::NewName`) + type slot - (`IdentSource::Types`) + a constraint walk. The type slot consumes - an optional parenthesised number/number-pair and discards it - (length/precision ignored). - - Table element (4a): `PRIMARY KEY ( col, … )`. **No** `FOREIGN KEY`, - no inline `REFERENCES` in 4a (those parse-error for now; 4b adds - them). -- `CHECK ()` / `DEFAULT ` reuse the ADR-0031 fragment via - `Node::Subgrammar(&sql_expr::SQL_OR_EXPR)` (the same mechanism - `sql_insert.rs` uses for value expressions). The matched expression - is compiled to storable SQL via the existing `compile_check_sql` - path used by `do_create_table`. + - Column element: name slot (`IdentSource::NewName`) + type slot + a + constraint walk admitting only `NOT NULL` / `UNIQUE` / + `PRIMARY KEY` (column-level). `DEFAULT` / `CHECK` are **not** in 4a + (→ 4a.2); typing them is "not yet supported". + - Type slot: `Choice[ Seq[Word("double"), Word("precision")], + Seq[ Ident{source: Types, validator: SQL_TYPE_VALIDATOR}, + Optional<( number [, number] )> ] ]` (§6.3). The validator uses + `Type::from_sql_name`; the length arg is discarded. + - Table element (4a): `PRIMARY KEY ( col, … )` only. **No** + `FOREIGN KEY` / inline `REFERENCES` (4b), **no** table-level + `UNIQUE`/`CHECK` (4a.2) — those parse-error / "not yet supported". Register in `REGISTRY` (`src/dsl/grammar/mod.rs`, Advanced group): `(&data::SQL_CREATE_TABLE, CommandCategory::Advanced)` alongside the @@ -233,53 +231,86 @@ enforces it. - `DROP TABLE [IF EXISTS]` → 4c. (`IF EXISTS` no-op-with-note reuses the 4.4 mechanism.) - Indexes, `ALTER TABLE`, `ALTER COLUMN TYPE`, table rename → 4d–4h. -- **Composite `UNIQUE(a,b)` / multi-column table `CHECK` → 4a.2** (a - persistence-model extension; see §6). 4a accepts **single-column** - table-level `UNIQUE(a)` / `CHECK(expr-over-one-col)` by normalizing - them into the column spec (free — `ColumnSchema` already carries - per-column `unique`/`check`), and rejects the composite/multi-column - forms with a "not yet supported" message until 4a.2 lands. +- **`CHECK`, `DEFAULT`, and all table-level `UNIQUE`/`CHECK` → 4a.2** + (the constraint slice; see §6.1). 4a rejects them with a + "not yet supported" message. 4a keeps only the constraints that are a + clean `do_create_table` reuse: column-level `NOT NULL`, `UNIQUE`, and + `PRIMARY KEY`, plus table-level `PRIMARY KEY (cols)`. - `CREATE TABLE … AS SELECT` (CTAS): **not in ADR-0035's surface** at all — an ordinary parse error, not a deferral. -## 6. Decisions settled this round (user-confirmed) + the 4a/4a.2 split +## 6. Decisions settled this round (all user-confirmed unless noted) -1. **4a/4a.2 split (user-confirmed).** Composite `UNIQUE(a,b)` and - multi-column table `CHECK` are **deferred to a dedicated slice - 4a.2**. Rationale (the general rule): a DDL feature needs - data-model work exactly when it introduces a **structure simple - mode could never produce** — not merely new syntax. Almost all of - advanced DDL (per-column constraints, compound PK, FK, indexes, - drop, alter-column) maps onto structures the model already persists - (`ColumnSchema`, `TableSchema.primary_key`, `RelationshipSchema`, - `IndexSchema`), so it is syntax-only + reuse. Composite UNIQUE / - table CHECK are the **first structures with no slot in - `TableSchema`**, so they need a model + round-trip extension and - earn their own slice. **Same class, already on the radar:** - `CREATE UNIQUE INDEX` (4d — `IndexSchema` has no `unique` field; - ADR-0025 deferred it) and table rename (4h — a new low-level op the - ADR already flags). +**The general rule (the litmus test).** A DDL feature needs data-model +or new-execution work exactly when it introduces a **structure simple +mode could never produce, or an expression representation the structural +helper can't consume** — not merely new syntax. Almost all of advanced +DDL (per-column NOT NULL/UNIQUE, compound PK, FK, indexes, drop, +alter-column) maps onto structures the model already persists +(`ColumnSchema`, `TableSchema.primary_key`, `RelationshipSchema`, +`IndexSchema`) and feeds the existing helpers, so it is syntax-only + +reuse. The exceptions earn their own slice. **Same class, already on the +radar:** `CREATE UNIQUE INDEX` (4d — `IndexSchema` has no `unique` +field; ADR-0025 deferred it) and table rename (4h — a new low-level op +the ADR flags). - **4a.2 scope** (its own test-first slice, after 4a core): - - Extend `TableSchema` (`src/persistence/mod.rs`) with table-level - constraint slots — e.g. `unique_constraints: Vec>` and - `check_constraints: Vec`. - - Extend the YAML writer/parser + `RawTable` - (`src/persistence/yaml.rs`) — additive, backward-compatible, - optional-on-read (the pattern used when `unique`/`not_null`/ - `check` were added; **no migration needed**). - - Extend `read_schema_snapshot` (`src/db.rs` ~2225) to **detect** - composite UNIQUE / table CHECK from the database. - - Extend `do_create_table` to **emit** them in the DDL. - - Extend the `SqlCreateTable` command shape + grammar to carry/parse - them (lifting the 4a "not yet supported" rejection). - - **Round-trip tests** (save → load → rebuild) proving they survive, - on top of parse/execute/undo coverage. +1. **The constraint slice 4a.2 (user-confirmed).** A dedicated + test-first slice, after 4a core, gathering every constraint that is + *not* a clean reuse: + - **`CHECK` / `DEFAULT`** — via the full ADR-0031 `sql_expr` surface, + captured as **raw SQL text** and stored directly (`ColumnSchema. + check` / `.default` are already `String`/`Option`). Needed + because `sql_expr` is **validate-only** — it builds no `Expr` AST, + so it cannot feed `compile_check_sql(expr: &Expr)` / + `ColumnSpec.check: Option`. This is a **separate execution + path** (raw-text, not the `Expr`→compile path), threaded through + `SqlCreateTable` + a `do_create_table` variant — hence its own + slice rather than 4a. + - **Composite `UNIQUE(a,b)` and multi-column table `CHECK`** — the + first structures with no slot in `TableSchema`; need the + model + round-trip extension: extend `TableSchema` + (`src/persistence/mod.rs`, e.g. `unique_constraints: + Vec>`, `check_constraints: Vec`); extend the + YAML writer/parser + `RawTable` (`src/persistence/yaml.rs`, + additive/backward-compatible/optional-on-read — the + `unique`/`not_null`/`check` pattern, **no migration**); extend + `read_schema_snapshot` (`src/db.rs` ~2225) to **detect** them; + extend `do_create_table` to **emit** them; **round-trip tests** + (save → load → rebuild). + - Until 4a.2: `CREATE TABLE` with `CHECK`, `DEFAULT`, or any + table-level `UNIQUE`/`CHECK` errors **"not yet supported"** (same + treatment as composite UNIQUE) — not a confusing parse error. 2. **No-op-with-note plumbing — mechanism (a)** (the `CreateOutcome` enum, §4.4). Explicit and reused by `DROP TABLE IF EXISTS` (4c); chosen now so the worker reply type is designed once. +3. **`double precision` (implementer call).** The lone two-word alias is + handled by a dedicated keyword-pair branch in the type slot + (`Choice[ Seq[Word("double"), Word("precision")], ]`); the builder maps the word-pair to `Type::Real`. + Single-word aliases + an optional discarded `(len[,len])` cover the + rest. Delivers the ADR §3 item without bending it. + +4. **`serial` PK inline emission (implementer call, step 3/worker).** + `do_create_table` today inlines `PRIMARY KEY` (the rowid-alias that + makes `serial` autoincrement) **only when the table has one column**. + SQL mode allows `CREATE TABLE t (id serial primary key, name text)` — + a `serial` sole-PK in a multi-column table — which would otherwise + get a table-level PK and **lose autoincrement**. Step 3 extends the + inline condition to "sole-PK column whose type is `serial` → inline + `PRIMARY KEY` on that column," leaving the simple-mode paths + (single-column, or compound) unchanged. Covered by a worker test. + +5. **Redundant PK constraints (implementer call).** SQL mode is + **lenient** like real engines: `id int primary key not null` / + `… unique` is accepted, and the builder silently de-dups the + redundant `not_null`/`unique` flag off a sole-PK column (so the + emitted DDL/metadata stay clean for `do_create_table`). Simple + mode's ADR-0029 §9 *rejection* is unchanged. (Diverges from simple + mode deliberately, matching the advanced-mode "trust the user like + SQL" posture, ADR-0035 §7.) + ## 7. Devil's Advocate review of this plan - **Does it reuse rather than fork execution?** Yes — `do_create_table` @@ -307,18 +338,22 @@ enforces it. (canonical + aliases + length-ignored + unknown→None) → red → add `resolve_type_name` → green. 2. **Command + parser** — write Tier-1 `ast_builder` tests (valid - columns/types/constraints, compound PK, single-column table-level - `UNIQUE`/`CHECK` normalized, `INTEGER PRIMARY KEY`→int, - `IF NOT EXISTS` flag; FK-shape and composite `UNIQUE(a,b)` / - multi-column `CHECK` rejected "not yet supported") → red → add - `Command::SqlCreateTable`, `sql_create_table.rs`, REGISTRY entry → + columns/types incl. aliases + `double precision` + discarded + `(len)`, column-level `NOT NULL`/`UNIQUE`/`PRIMARY KEY`, compound + table-level PK, `INTEGER PRIMARY KEY`→int, `IF NOT EXISTS` flag; + `DEFAULT`/`CHECK`/table-level `UNIQUE`/FK-shape rejected "not yet + supported") → red → add `Command::SqlCreateTable`, + `sql_create_table.rs`, REGISTRY entry, exhaustive-match stubs → green. Include a fallback test: simple `create … with pk` still - parses in advanced mode. + parses in advanced mode. *(Compiles only once the worker dispatch + handles the new `Command`, so steps 2–3 land together.)* 3. **Worker** — write Tier-3 `tests/sql_create_table.rs` (metadata, CSV, - describe; alias end-to-end; `IF NOT EXISTS` no-op-with-note; - duplicate-table plain error) → red → add `Request::SqlCreateTable` + - `snapshot_then` arm + the §4.4 outcome + reuse `do_create_table` → - green. + describe; alias end-to-end; `serial` PK autoincrements in a + multi-column table, §6.4; `IF NOT EXISTS` no-op-with-note; + duplicate-table plain error; redundant PK constraint de-duped, §6.5) + → red → add `Request::SqlCreateTable` + `snapshot_then` arm + the + §4.4 `CreateOutcome` + the §6.4 `do_create_table` serial-inline + extension + reuse `do_create_table` → green. 4. **Undo** — write the undo Tier-3 test (one step; undo/redo) → red → confirm it passes (the `snapshot_then` wrap should make it green with no extra code; if not, the wrap is wrong) → green.