From 701217d29fa82d6bf47d9cb51b41d72858274f07 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Mon, 25 May 2026 18:41:02 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20ADR-0035=204d=20=E2=80=94=20CREATE=20[U?= =?UTF-8?q?NIQUE]=20INDEX=20/=20DROP=20INDEX?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Advanced-mode SQL CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols) -> SqlCreateIndex and DROP INDEX [IF EXISTS] -> SqlDropIndex, both reusing the ADR-0025 executors (do_add_index / do_drop_index), like 4c reused do_drop_table. - CREATE UNIQUE INDEX admitted in advanced mode (ADR-0025 Amendment 1): ADR-0025 deferred UNIQUE indexes for the simple-mode DSL, but advanced mode trusts the user like SQL does. Adds an additive IndexSchema.unique flag (project.yaml, serde-default, version stays 1); rebuild re-emits CREATE UNIQUE INDEX; the redundant-set guard keys on (columns, unique). Simple-mode `add unique index` stays deferred. - IF [NOT] EXISTS on both forms reuses the 4c no-op-with-note skip (journalled, not snapshotted) via CreateIndexOutcome / DropIndexOutcome. - Unnamed CREATE INDEX auto-named (ADR-0025 convention); the [UNIQUE] prefix is a concrete-keyword Choice and the optional name an on-led-first selector (the drop-index selector precedent) — trap-safe. - create/drop each gain a second advanced node; the existing all-candidates dispatch handles it (locked by parse tests). - Unique indexes marked [unique] in the structure view and items panel. - do_add_index refuses internal __rdbms_* tables as "no such table", closing a latent exposure on both the simple `add index` and the new SQL CREATE INDEX surfaces (ADR-0025 Amendment 1). Docs: ADR-0035 status + §13 4d + 4i; ADR-0025 Amendment 1; ADR README; requirements.md Q1/C3. Plan: docs/plans/20260525-adr-0035-sql-ddl-4d.md. Tests: 1834 passing / 0 failing / 0 skipped / 1 ignored; clippy clean. --- docs/adr/0025-indexes.md | 42 +- docs/adr/0035-advanced-mode-sql-ddl.md | 43 +- docs/adr/README.md | 4 +- docs/plans/20260525-adr-0035-sql-ddl-4d.md | 439 +++++++++++++++++++++ docs/requirements.md | 13 +- src/app.rs | 27 ++ src/completion.rs | 16 +- src/db.rs | 244 +++++++++++- src/dsl/command.rs | 30 ++ src/dsl/grammar/ddl.rs | 299 ++++++++++++++ src/dsl/grammar/mod.rs | 24 +- src/event.rs | 14 + src/friendly/keys.rs | 7 + src/friendly/strings/en-US.yaml | 13 + src/output_render.rs | 31 +- src/persistence/mod.rs | 8 + src/persistence/yaml.rs | 96 +++++ src/runtime.rs | 63 ++- src/ui.rs | 15 +- tests/sql_create_index.rs | 360 +++++++++++++++++ tests/sql_drop_index.rs | 123 ++++++ tests/typing_surface/mod.rs | 2 + 22 files changed, 1865 insertions(+), 48 deletions(-) create mode 100644 docs/plans/20260525-adr-0035-sql-ddl-4d.md create mode 100644 tests/sql_create_index.rs create mode 100644 tests/sql_drop_index.rs diff --git a/docs/adr/0025-indexes.md b/docs/adr/0025-indexes.md index 1229430..dd4f014 100644 --- a/docs/adr/0025-indexes.md +++ b/docs/adr/0025-indexes.md @@ -2,7 +2,11 @@ ## Status -Accepted +Accepted. **Amendment 1 (2026-05-25):** UNIQUE indexes are admitted on +the **advanced-mode** SQL surface (`CREATE UNIQUE INDEX`) — see +*Amendment 1* below and ADR-0035 §4d. The original *Out of scope* +exclusion stands for the **simple-mode DSL** (`add unique index` remains +deferred). ## Context @@ -335,6 +339,42 @@ here so the decision text and the code agree: list with each table's indexes indented beneath — the `S2` nested view — reading the two together. +## Amendment 1 — UNIQUE indexes in advanced mode (2026-05-25) + +This ADR's *Out of scope* excluded UNIQUE indexes (`add unique index`) +on the grounds that a unique index conflates two concepts the playground +teaches separately — an index (a performance structure) and a UNIQUE +*constraint* (an integrity rule, tracked as its own `C3` sub-item). That +reasoning was written on 2026-05-16, when the **simple-mode DSL was the +only input surface**, and it still holds there: simple mode teaches the +two concepts separately, so `add unique index` stays deferred. + +ADR-0035 (advanced-mode SQL DDL) introduced a second surface whose +explicit posture is to "trust the user like SQL does" (ADR-0035 §7). On +that surface `CREATE UNIQUE INDEX` is standard SQL a learner types +verbatim, and the concept-separation argument does not transfer — so +ADR-0035 §4 lists `CREATE [UNIQUE] INDEX` and **§4d supersedes this +ADR's exclusion for the advanced surface**. The constraint track this +ADR deferred *to* (ADR-0018 → ADR-0029 → ADR-0035 §4a.2) has since +shipped, so there is no remaining dependency. + +Mechanically, the index model gains an `IndexSchema.unique` flag — an +additive, `#[serde(default)]` `project.yaml` field (`version` stays +`1`); the engine already reports uniqueness via `pragma_index_list` +(origin `c`), so **no `__rdbms_*` metadata table is added** (the §Storage +decision is unchanged — the divergence from the relationship precedent +stands). The rebuild primitive re-emits `CREATE UNIQUE INDEX`; the +structure view and items panel mark a unique index `[unique]` +(ADR-0035 §4d). The redundant-column-set guard keys on `(columns, +unique)` so a plain and a unique index over the same columns are not +mutual duplicates. + +The amendment also hardened the shared `do_add_index` executor to refuse +an internal `__rdbms_*` table as "no such table" (consistent with the +app-wide opacity of internal tables) — closing a latent exposure on +*both* the simple `add index` and the new SQL `CREATE INDEX` surfaces, +which both reach `do_add_index`. + ## See also - ADR-0004 / ADR-0015 (project file format and storage runtime) diff --git a/docs/adr/0035-advanced-mode-sql-ddl.md b/docs/adr/0035-advanced-mode-sql-ddl.md index 3099d0c..8b91116 100644 --- a/docs/adr/0035-advanced-mode-sql-ddl.md +++ b/docs/adr/0035-advanced-mode-sql-ddl.md @@ -3,12 +3,13 @@ ## Status Accepted. Design agreed with the user (2026-05-24); the approach is -**validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c** +**validated end-to-end by sub-phases 4a / 4a.2 / 4a.3 / 4b / 4c / 4d** (`CREATE TABLE` with column- and table-level constraints and foreign -keys, and `DROP TABLE [IF EXISTS]`, implemented 2026-05-25 — plans +keys, `DROP TABLE [IF EXISTS]`, and `CREATE [UNIQUE] INDEX` / +`DROP INDEX [IF EXISTS]`, implemented 2026-05-25 — plans `docs/plans/20260524-adr-0035-sql-ddl-4a.md`, `…-4a2.md`, `…-4a3.md`, -`docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`), so the -decision is accepted while the remaining sub-phases (**4d–4i**, §13) +`docs/plans/20260525-adr-0035-sql-ddl-4b.md`, `…-4c.md`, `…-4d.md`), so +the decision is accepted while the remaining sub-phases (**4e–4i**, §13) continue. This is **Phase 4** of the ADR-0030 roadmap (the advanced-mode SQL surface), the peer of ADR-0031 (expression grammar), ADR-0032 (`SELECT`), and ADR-0033 (DML). It **clarifies ADR-0030 §4** @@ -381,7 +382,25 @@ ADR-0033's structure: DSL drops still parse via fallback — 4i grows the surface as `DROP INDEX` lands in 4d. - **4d — `CREATE [UNIQUE] INDEX` / `DROP INDEX`** → `SqlCreateIndex` - / `SqlDropIndex` (ADR-0025; the `UNIQUE` flag extension if needed). + / `SqlDropIndex`. *(Implemented 2026-05-25 — plan + `docs/plans/20260525-adr-0035-sql-ddl-4d.md`.)* Both reuse the ADR-0025 + executors (`do_add_index` / `do_drop_index`), like 4c reused + `do_drop_table`. `CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON + (cols)` (the unnamed form auto-named per ADR-0025; the leading + `[UNIQUE]` is a concrete-keyword `Choice`, the optional name an + `on`-led-first selector mirroring the `drop index` positional + selector) and `DROP INDEX [IF EXISTS] ` (name-only — the + positional `drop index on T(…)` stays the simple form via fallback). + `IF [NOT] EXISTS` reuses the 4c no-op-with-note skip (journalled, not + snapshotted). **`CREATE UNIQUE INDEX` is admitted** (user-confirmed + 2026-05-25): ADR-0025 deferred UNIQUE indexes for the *simple-mode + DSL*, but advanced mode "trusts the user like SQL does" (§7) — so the + model gains an `IndexSchema.unique` flag (additive YAML, `version` 1; + rebuild re-emits `CREATE UNIQUE INDEX`; the structure view + items + panel mark `[unique]`), recorded as **ADR-0025 Amendment 1**. + Simple-mode `add unique index` stays deferred. `create`/`drop` each + gain a *second* advanced node, exercising the all-candidates dispatch + (`decide` tries every advanced candidate). - **4e — `ALTER TABLE` add/drop/rename column.** Drop/rename column must guard against a **table-level CHECK that references the column** (4a.3): today the rebuild rejects it cleanly via the engine (the @@ -398,8 +417,13 @@ ADR-0033's structure: statement), `help`/usage for the new forms. **Carried in from earlier slices:** (a) refresh the `CREATE TABLE` help/usage skeleton for the 4a.2 `DEFAULT`/`CHECK`/composite-`UNIQUE`, 4a.3 table-`CHECK`, and 4b - FK forms (deferred from each); (b) `describe` display of table-level - constraints (composite `UNIQUE` + table `CHECK`); (c) **4b self-ref + FK forms (deferred from each) — **4d's index forms already carry their + own help/usage** (`ddl.sql_create_index` / `ddl.sql_drop_index` + the + `parse.usage.*` keys), since the nodes are new; (b) `describe` display + of table-level constraints (composite `UNIQUE` + table `CHECK`) — note + the **unique-*index* marker shipped in 4d** (`[unique]` in the + structure view + items panel), so only the table-level *constraint* + display remains here; (c) **4b self-ref FK indicator** — a `CREATE TABLE` with a self-referencing FK (`references `) parses + executes correctly, but the pre-submit schema-existence diagnostic falsely flags the not-yet-created self @@ -415,7 +439,10 @@ ADR-0033's structure: entry word so advanced completion offers every valid continuation (`drop ` → table + column + relationship + index + constraint; `drop rel` → relationship); verify `create`/`insert`/`update`/`delete` - completion stays sensible. (e) **Discussion flag (user, 2026-05-25):** + completion stays sensible. **4d widened this:** `create` and `drop` + now each have *two* advanced nodes (table + index), so a shared entry + word's continuations now span two SQL shapes as well as the DSL ones — + the merge matters more. (e) **Discussion flag (user, 2026-05-25):** before/with (d), discuss **visually distinguishing simple- vs advanced-mode completions in the hint UI (likely by colour)** so a learner can see which continuations are DSL and which are SQL — a UX diff --git a/docs/adr/README.md b/docs/adr/README.md index 4655683..eb9cb6f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -30,7 +30,7 @@ This directory contains the project's ADRs, recorded per - [ADR-0022 — Ambient typing assistance: colour, hint panel, completion (I3 + I4)](0022-ambient-typing-assistance.md) — **Amendment 1 supersedes §12's simple-mode-only carve-out**: the unified mode-aware walker (ADR-0030/0031/0032) now speaks SQL, so advanced-mode ambient assistance is re-enabled. `ambient_hint_in_mode` + `hint_resolution_at_input_in_mode` + `expected_for_hint_snapshot` thread `Mode`; `render_hint_panel` calls ambient for all modes (no more advanced-mode `None`); the one-shot `:` sigil is stripped before the ambient walk. Fixes a live bug where advanced-mode SQL hinting/completion-preview were dead despite Phase 2 marking them green (validated at the engine layer, not the UI). Simple-mode gating, highlighting, and the §13 performance posture are unchanged; covered by an app-level render test plus ambient-layer regression locks; **Amendment 2 reverses the handoff-14 keywords-first candidate ordering** — schema identifiers (table/column/relationship names) now sort *before* keywords so a name the user would have to look up stays visible in the single-row, window-scrolled candidate line (keywords are learned over time; the `tok_identifier`/`tok_keyword` colour split marks the boundary); shipped with a `walk_repeated` fix that surfaces a list item's trailing optionals at a clean boundary (`order by Name ` → `asc`/`desc`, `select Name ` → `as`, `create table … Code(text) ` → `not`/`unique`/`default`/`check`; the `,` separator deliberately not surfaced); records a deferred two-line hint box for growing lists - [ADR-0023 — Unified declarative grammar tree](0023-unified-grammar-tree.md) — direction (superseded for execution detail by ADR-0024) - [ADR-0024 — Unified grammar tree: execution plan](0024-unified-grammar-tree-execution-plan.md) — **Accepted**, the executable spec — implemented (Phases A–F; Phase F shipped "minimal", `parser.rs` retained as the router — see the ADR's Phase F implementation note) -- [ADR-0025 — Indexes](0025-indexes.md) — **Accepted**, `add index` / `drop index`, persistence, rebuild-table preservation, and items-list display (`C3` index portion + `S2`) +- [ADR-0025 — Indexes](0025-indexes.md) — **Accepted** (**Amendment 1, 2026-05-25**: UNIQUE indexes admitted on the **advanced-mode** surface via `CREATE UNIQUE INDEX` — ADR-0035 §4d; the `IndexSchema.unique` flag round-trips through `project.yaml` with no new metadata table since the engine reports uniqueness natively; simple-mode `add unique index` stays deferred), `add index` / `drop index`, persistence, rebuild-table preservation, and items-list display (`C3` index portion + `S2`) - [ADR-0026 — Complex WHERE expressions](0026-complex-where-expressions.md) — **Accepted**, stratified recursive expression grammar (`AND`/`OR`/`NOT`, comparisons, `LIKE`, `IS NULL`, `IN`, `BETWEEN`) for `update` / `delete` / `show data` filters; `show data` gains `where` + `limit`; adds the `Subgrammar` node and a recursive `Expr` AST (`C5a`) - [ADR-0027 — Input-field validity indicator](0027-input-validity-indicator.md) — **Accepted**, a debounced `[ERR]` / `[WRN]` marker at the input row's right edge, backed by a walker diagnostics-severity model (parse-outcome + schema-existence); advisory, never blocks submission (`S6`); Amendment 1 adds a `LIKE`-on-numeric-column WARNING - [ADR-0028 — Query plans (`EXPLAIN QUERY PLAN`)](0028-query-plans.md) — **Accepted**, an `explain` prefix command over `show data` / `update` / `delete`; an annotated, span-styled plan tree; introduces the `OutputLine` styled-runs mechanism (ADR-0016's deferred per-span styling) (`QA1` / `QA2`) @@ -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]`, implemented 2026-05-25; 4d–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`); 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]`, implemented 2026-05-25; 4e–4i pending), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch); the remaining DDL forms stay "not yet supported" until their sub-phases land. Each sub-phase has exit + DA gates diff --git a/docs/plans/20260525-adr-0035-sql-ddl-4d.md b/docs/plans/20260525-adr-0035-sql-ddl-4d.md new file mode 100644 index 0000000..41a7b9d --- /dev/null +++ b/docs/plans/20260525-adr-0035-sql-ddl-4d.md @@ -0,0 +1,439 @@ +# Plan: ADR-0035 Phase 4, sub-phase 4d — `CREATE [UNIQUE] INDEX` / `DROP INDEX` + +Add advanced-mode SQL `CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON + (, …)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] +` → `SqlDropIndex` (ADR-0035 §1/§4/§13). Both reuse the existing +ADR-0025 low-level executors (`do_add_index` / `do_drop_index`), like 4c +reused `do_drop_table`. Two genuinely new things beyond 4c's pattern: + +1. **`UNIQUE` index → an `IndexSchema.unique` model extension** (the + escalation; user-approved 2026-05-25). ADR-0025 deferred UNIQUE + indexes for the *simple-mode DSL* (`add unique index`); ADR-0035 §4 + supersedes that for the *advanced* SQL surface. Simple-mode + `add unique index` stays deferred. An ADR-0025 **Amendment** records + this. +2. **A second advanced node per entry word.** `create` gains + `SQL_CREATE_INDEX` alongside `SQL_CREATE_TABLE`; `drop` gains + `SQL_DROP_INDEX` alongside `SQL_DROP_TABLE`. The dispatcher already + tries *all* advanced candidates (`decide`: `advanced.chain(simple)` + + the first-full-match loop), so no dispatch change is needed — but a + parse test locks it. + +`IF [NOT] EXISTS` on **both** forms (user-approved 2026-05-25), reusing +the 4c no-op-with-note skip path. + +## 1. Baseline + +- Tests: **1805 passing, 0 failing, 0 skipped, 1 ignored** (the + `friendly/mod.rs` ```ignore``` doctest); clippy clean. Branch `main`, + HEAD `e52e90c` (4c), 3 local-only commits since `origin/main`. 4d + starts here. + +## 2. Decisions (settled — escalated + user-confirmed 2026-05-25) + +1. **`CREATE UNIQUE INDEX` is in; the model extension lands now.** + `IndexSchema` gains `unique: bool`. ADR-0025's UNIQUE-index exclusion + was a statement about the *simple-mode DSL teaching surface* (the only + surface that existed on 2026-05-16); advanced mode "trusts the user + like SQL does" (ADR-0035 §7), so the concept-separation argument does + not transfer. The constraint track it deferred *to* (ADR-0018 → 0029 → + 0035 4a.2) has since shipped. ADR-0025 gets an Amendment; **simple-mode + `add unique index` stays deferred** (not in this slice). +2. **`IF [NOT] EXISTS` on both forms** — universal cross-vendor idiom + (ADR-0035 §4 reclassification), reusing the 4c + `CreateOutcome`/`DropOutcome`-style no-op-with-note. Skip is + **journalled, not snapshotted**. +3. **Reuse the ADR-0025 executors.** `do_add_index` / `do_drop_index` are + the single executors; the SQL path differs only at the `unique` flag + and the `IF [NOT] EXISTS` skip branch. No fork. +4. **`DROP INDEX ` is name-only.** SQL has no positional + `on T (cols)` drop form (that is the simple DSL `drop index on …`, + which keeps falling back to the simple `drop` node). `SqlDropIndex` + carries `{ name, if_exists }`; the worker builds + `IndexSelector::Named { name }` for `do_drop_index`. +5. **Unnamed `CREATE INDEX` is supported and auto-named** (ADR-0035 §4 + `[]`; the ADR-0025 `__idx` convention). A playground + convenience over SQLite (which requires a name). See §4.1 for the + grammar-disambiguation risk this carries. + +## 3. Phase 1 — Requirements checklist (4d) + +Grammar / parse: +- [ ] `CREATE INDEX ON ()` parses in advanced mode → + `SqlCreateIndex { unique: false, if_not_exists: false }`. +- [ ] `CREATE UNIQUE INDEX ON …` sets `unique: true`. +- [ ] `CREATE INDEX ON ()` (no name) parses → `name: None` + (auto-named at execution). The optional name must **not** swallow `on`. +- [ ] `CREATE [UNIQUE] INDEX IF NOT EXISTS ON …` sets + `if_not_exists: true`. +- [ ] `DROP INDEX ` → `SqlDropIndex { if_exists: false }`; + `DROP INDEX IF EXISTS ` → `if_exists: true`. +- [ ] Trailing `;` tolerated on both (ADR-0035 §12). +- [ ] **Dispatch (second advanced node):** in advanced mode + `create table …` → `SqlCreateTable`, `create [unique] index …` → + `SqlCreateIndex`; `drop table …` → `SqlDropTable`, `drop index …` → + `SqlDropIndex`. Simple `add index` / `drop index on T(…)` / + `drop column` etc. still parse (fallback). + +Execution / model: +- [ ] `CREATE INDEX` creates a plain index (parity with `add index`); + one undo step; `undo` removes it. +- [ ] `CREATE UNIQUE INDEX` creates a unique index; it round-trips + through `project.yaml` (the `unique` flag) and survives `rebuild` + (re-emitted as `CREATE UNIQUE INDEX`); enforces uniqueness (a duplicate + insert is refused by the engine). +- [ ] `DROP INDEX` removes the index; one undo step; `undo` restores it; + the affected table's structure is auto-shown (ADR-0014). +- [ ] `IF NOT EXISTS` on an existing index name → success + note, no + error, no snapshot, journalled. `IF EXISTS` on an absent index → + ditto. +- [ ] Plain `CREATE INDEX` on a duplicate name / `DROP INDEX` on an + unknown name → the existing friendly errors (unchanged). +- [ ] Redundant-column-set guard refined for uniqueness: a plain and a + unique index over the same columns are **not** mutual duplicates + (different semantics); an exact duplicate (same columns **and** same + uniqueness) is still refused. *(Test must use **explicit distinct + names** for the plain+unique pair — the auto-name `__idx` is + identical for both, so an unnamed second would collide on the name + guard regardless. DA finding D.)* +- [ ] `CREATE UNIQUE INDEX` on a column that **already holds duplicate + values** is refused by the engine at creation; the failure routes + through the friendly layer, engine-neutral. *(Test.)* +- [ ] `IF NOT EXISTS` only short-circuits a **name** collision into a + skip; a *different*-named but redundant-column-set create still hits + the ADR-0025 redundant-set refusal (the playground's pedagogical guard, + not raw-SQL semantics). *(Test + one-line doc note. DA finding H.)* +- [ ] Errors/notes engine-neutral. + +Persistence round-trip: +- [ ] `IndexSchema.unique` round-trips: save → YAML `unique: true` → + load → identical snapshot. Older project files (no `unique` field) + default to `false` (`#[serde(default)]`); `version` stays `1`. +- [ ] The rebuild-table primitive preserves a unique index's uniqueness + (it re-creates captured indexes; `IndexInfo.unique` already read — + verify the re-emit honours it). + +Display (unique marker — user-confirmed for 4d): +- [ ] Structure view marks a unique index — `… (cols) [unique]` — and + leaves a plain index unmarked. +- [ ] Items list (left panel) marks a unique index name. + +### Testing +- [ ] **Tier 1** (in-crate `sql_create_index_tests` / `sql_drop_index_tests` + in `ddl.rs`, mirroring `sql_drop_table_tests`): the parse + flag cases + above + the dispatch (second-advanced-node) cases. +- [ ] **Tier 3** (`tests/sql_create_index.rs`, `tests/sql_drop_index.rs`): + create plain/unique, the YAML round-trip + rebuild survival of a unique + index, uniqueness enforcement, `IF [NOT] EXISTS` skip + journal, plain + duplicate/unknown errors, one undo step + restore, drop auto-show. +- [ ] **Tier 2** (insta): a `project.yaml` snapshot carrying a + `unique: true` index, if an existing yaml snapshot test covers indexes + (extend it; else a focused new one). +- [ ] **Catalog** lockstep + vocab audit for the new note + help/usage + keys. + +## 4. Architecture & design + +### 4.1 Grammar (`src/dsl/grammar/ddl.rs`) + +**`DROP INDEX` (easy, mirrors `SQL_DROP_TABLE`).** +- `SQL_DROP_INDEX_SHAPE = Seq[ Word("index"), SQL_DROP_IF_EXISTS_OPT, + INDEX_NAME_EXISTING, Optional(Punct(';')) ]`. Reuse the existing + `SQL_DROP_IF_EXISTS_OPT` and `INDEX_NAME_EXISTING` nodes. +- `pub static SQL_DROP_INDEX: CommandNode { entry: "drop", shape, + ast_builder: build_sql_drop_index, help_id, usage_ids }`. +- `build_sql_drop_index`: `name = require_ident("index_name")`, + `if_exists = path.contains_word("if")`. + +**`CREATE INDEX`.** The shape, after the `create` entry word, must lead +with a concrete keyword (the leading-`Optional` trap, handoff §3 / +pattern 5). Both sub-problems are **settled by existing precedent** (DA +pass verified against the walker code, not reasoned): + +- *`[UNIQUE]` optional prefix.* Lead with a `Choice` whose every branch + starts on a concrete keyword: `UNIQUE_INDEX_LEAD = Choice[ + Seq[Word("unique"), Word("index")], Word("index") ]`. A leading + **`Choice`** of concrete-keyword branches is exactly the sanctioned + pattern (the §3 rule forbids a leading *`Optional`*, not a `Choice`); + `unique` presence is read in the builder via + `path.contains_word("unique")`. Smoke-probe only. +- *Optional `[]` before `on` — use the `DI_SELECTOR` precedent, not + a bare `Optional`.* **Verified:** `walk_ident` → `consume_ident` does + **not** reject reserved keywords, so a bare `Optional()` + *would* greedily eat the `on` keyword and break the unnamed form. The + drop-index selector already solves this exact shape: a `Choice` with + the **`on`-led branch first**, relying on `Choice` backtracking. Mirror + it: + - `CI_UNNAMED = Seq[ Word("on"), TABLE_NAME_EXISTING, Punct('('), + INDEX_COLUMN_LIST, Punct(')') ]` + - `CI_NAMED = Seq[ INDEX_NAME_NEW, Word("on"), TABLE_NAME_EXISTING, + Punct('('), INDEX_COLUMN_LIST, Punct(')') ]` + - `CI_SELECTOR = Choice[ CI_UNNAMED, CI_NAMED ]` (unnamed first, like + `DI_SELECTOR`'s `DI_POSITIONAL`-first). `create index on T (c)` → + `CI_UNNAMED` (no name-slot to eat `on`); `create index ix on T (c)` → + `CI_UNNAMED` fails at `Word(on)` vs `ix`, backtracks to `CI_NAMED`. +- Full shape: `Seq[ UNIQUE_INDEX_LEAD, + SQL_CREATE_INDEX_IF_NOT_EXISTS_OPT, CI_SELECTOR, Optional(Punct(';')) ]`. + The `IF NOT EXISTS` opt is mid-`Seq` (not leading) — exactly like 4c's + `SQL_DROP_TABLE` `IF EXISTS` opt, so it is trap-safe. +- `build_sql_create_index`: `unique = contains_word("unique")`, + `if_not_exists = contains_word("if")`, `name = optional_ident( + "index_name")` (present iff the `CI_NAMED` branch matched), + `table = require_ident("table_name")`, `columns` from the list. + +The single-node form is now the *primary* approach (no probe gamble): the +two-node `TABLE_FK`-style split is **not needed** — kept only as a +contingency if the smoke-probe of the leading `Choice` surprises. + +**REGISTRY (`mod.rs`):** add `(&ddl::SQL_CREATE_INDEX, +CommandCategory::Advanced)` and `(&ddl::SQL_DROP_INDEX, +CommandCategory::Advanced)`. Verify `decide` tries all advanced +candidates (it does — locked by a parse test). + +### 4.2 Command (`src/dsl/command.rs`) +- `SqlCreateIndex { name: Option, table: String, columns: + Vec, unique: bool, if_not_exists: bool }` — verb + `"create index"`, `target_table()` → `table`. +- `SqlDropIndex { name: String, if_exists: bool }` — verb + `"drop index"`, `target_table()` → `name` (the index name is the + identifying thing for logging; mirrors `DropIndex`/`SqlDropTable`). +- Exhaustive-match fallout (compiler-found): `verb`, `target_table` in + `command.rs`; `app.rs` failure-translate; `runtime.rs` dispatch + + outcome→event; `tests/typing_surface/mod.rs` `command_label` arms + (`SqlCreateIndex`/`SqlDropIndex`, alongside the existing + `SqlCreateTable`/`SqlDropTable`). Add the SQL index forms to the + `tests/typing_surface/index_ops.rs` matrix (+ insta snaps) next to the + DSL `add index`/`drop index` cases. +- **`IndexSchema` struct-literal fallout** (compiler-found, the new + `unique` field): db.rs:2414, yaml.rs:299, the yaml round-trip test + (~478) — each adds `unique: …`. + +### 4.3 Model extension — `IndexSchema.unique` (`src/persistence/`, `src/db.rs`) +- `persistence/mod.rs`: `IndexSchema` gains `pub unique: bool` (doc-note + mirroring `TableSchema.unique_constraints`: additive, defaults `false` + for older files, `version` stays `1`). +- `db.rs read_schema_snapshot` (~2414): carry `idx.unique` (already read + into `IndexInfo` by `read_table_indexes`). +- `yaml.rs`: the raw deserialize struct gains `#[serde(default)] unique: + bool`; the reader (~299) carries it; `write_index` (~66) emits + `unique: true` **only when true** (omit when false — keeps existing + project files byte-stable and matches the minimal-noise house style; + confirm against any golden-yaml test). +- `db.rs rebuild_from_text` (~7814): emit `CREATE UNIQUE INDEX` when + `index.unique`, else `CREATE INDEX`. +- `do_add_index` gains `unique: bool`: emit `CREATE UNIQUE INDEX` when + set; **refine the redundant-set guard** to compare `(columns, unique)` + so a plain vs unique index over the same columns is not a false + duplicate. Simple `add index` passes `unique: false` (call-site + update in the worker arm). + +### 4.4 Worker (`src/db.rs`) +- `Request::SqlCreateIndex { name, table, columns, unique, if_not_exists, + source, reply: oneshot> }` + + `db.sql_create_index(...)`. + - Skip branch: resolve the index name (given, or the `__idx` + auto-name — extract a tiny `resolve_index_name` helper shared with + `do_add_index`), and if `if_not_exists && index_name_exists(resolved)` + → journal the line (no snapshot), reply `CreateIndexOutcome::Skipped( + resolved)`. Else `snapshot_then(do_add_index(..., unique) → + Created(desc))`. +- `Request::SqlDropIndex { name, if_exists, source, reply: + oneshot> }` + `db.sql_drop_index(...)`. + - Skip branch: if `if_exists && !index_name_exists(name)` → journal (no + snapshot), reply `Skipped`. Else `snapshot_then(do_drop_index( + IndexSelector::Named { name }) → Dropped(desc))`. +- New outcome enums (the index peers of `CreateOutcome`/`DropOutcome`): + - `CreateIndexOutcome { Created(TableDescription), Skipped(String) }` + (skip carries the **resolved** name — the auto-name is unknown to the + command for the unnamed form). + - `DropIndexOutcome { Dropped(TableDescription), Skipped }` (drop-skip + note uses the command's `name` directly). +- `index_name_exists(conn, name)` helper (the `sqlite_master WHERE + type='index' AND name=?` lookup `do_add_index` step 5 already does). + +### 4.5 Runtime + event + app +- `runtime.rs`: `SqlCreateIndex` → `db.sql_create_index` → `Created(d)` → + `CommandOutcome::Schema(Some(d))`, `Skipped(name)` → new + `CommandOutcome::SchemaCreateIndexSkipped(name)`. `SqlDropIndex` → + `Dropped(d)` → `Schema(Some(d))` (auto-show the de-indexed table), + `Skipped` → new `CommandOutcome::SchemaDropIndexSkipped`. +- `event.rs`: `DslCreateIndexSkipped { command, name }`, + `DslDropIndexSkipped { command }`. +- `app.rs`: note `ddl.create_index_skipped_exists` (name from the event) + / `ddl.drop_index_skipped_absent` (name from `command.target_table()`). + Both **note-only, no structure** (mirrors the drop-table skip — the + index already exists / never existed; showing the whole table is + noise). Add the failure-translate arms for the two new commands. + +### 4.7 Unique-index display (user-confirmed for 4d, 2026-05-25) +A learner must be able to tell a UNIQUE index from a plain one. +- **Structure view (cheap — `IndexInfo.unique` already populated):** + `output_render.rs:143` appends a ` [unique]` marker when + `index.unique`, e.g. `Customers_email_idx (Email) [unique]`. A render + test + any insta snapshot update. No new threading. +- **Items list (left panel):** `SchemaCache.table_indexes` is + `HashMap>` (names only). Carry uniqueness — change + the value to a small `{ name, unique }` entry (or `Vec<(String, + bool)>`), populate it from the schema-cache refresh (the `unique` bit + is on the read), and render `name [unique]` in `ui.rs:516`. Update the + `items_panel_nests_indexes_under_their_table` test + any callers + constructing `table_indexes`. +- Marker wording engine-neutral (no PRAGMA/SQLite leakage) — `[unique]` + is a plain English adjective, fine. + +### 4.6 Friendly catalog (`keys.rs` + `en-US.yaml`) +New keys (engine-neutral; lockstep + vocab audit enforce both): +- `ddl.create_index_skipped_exists` → `"index '{name}' already exists — + skipped (no changes made)"` (`&["name"]`). +- `ddl.drop_index_skipped_absent` → `"index '{name}' doesn't exist — + skipped (no changes made)"` (`&["name"]`). +- `help.ddl.sql_create_index` + `parse.usage.sql_create_index`, + `help.ddl.sql_drop_index` + `parse.usage.sql_drop_index` (fresh nodes; + their keys must exist now — unlike the deferred *refresh* of the + create-table skeleton in 4i(a)). + +## 5. Phase 2 — Candidate approaches + +**(C1) CREATE INDEX grammar — single node w/ leading `Choice`** *(lead)*. +One `SQL_CREATE_INDEX` node; `[UNIQUE]` via a leading `Choice` of +concrete-keyword-led branches; `unique` read in the builder. +- *Good:* one node, one builder, one REGISTRY entry; matches how a single + statement maps to a single command. +- *Risk:* a leading `Choice` in a top-level shape is untested here; the + unnamed-form `on` disambiguation. Both are **probe-or-fall-back**, not + blockers. + +**(C2) CREATE INDEX grammar — two nodes split on `unique`/`index`** +(the 4c `TABLE_FK` split). `SQL_CREATE_INDEX` (`index`-led) + +`SQL_CREATE_INDEX_UNIQUE` (`unique`-led). +- *Good:* each node leads on a concrete keyword — guaranteed safe past the + trap; `create` then has three advanced nodes, exercising the + all-candidates dispatch even harder (a feature for the test). +- *Bad:* duplicated shape/builder; more REGISTRY noise. Adopt **only if + C1's leading `Choice` probes badly.** + +**(M1) Model extension — `IndexSchema.unique` flag** *(lead; the only +real option)*. Mirrors `TableSchema.unique_constraints` exactly (additive, +serde-default, version 1). The read side is already half-done +(`IndexInfo.unique`). +**(M2) Separate `__rdbms_playground_indexes` metadata table** — +*rejected.* ADR-0025 deliberately stores nothing app-specific for indexes +(SQLite owns the index namespace, incl. uniqueness via +`pragma_index_list`). A metadata table would duplicate engine-owned state +and create a consistency hazard. The flag is read straight from the +pragma. + +**(S1) Skip plumbing — dedicated index skip outcomes/events** *(lead)*. +New `CreateIndexOutcome`/`DropIndexOutcome` + `DslCreate/DropIndexSkipped` +events + index-specific notes. Consistent with 4a/4c (one event per skip +kind). +**(S2) Generalise the existing skip events to be object-agnostic** — +*rejected.* Would rewrite 4a/4c wording/behaviour for marginal saving; +churn on shipped code. + +## 6. Phase 3 — Selection vs the checklist + +C1 + M1 + S1 satisfy every §3 item: parse/flags (C1 grammar + builder), +dispatch (REGISTRY + the existing all-candidates `decide`), execution +(reuse `do_add_index`/`do_drop_index` + the `unique` param), persistence +round-trip + rebuild survival (M1), skip semantics (S1 + the 4c skip +branch), engine-neutral strings (catalog keys under the vocab audit). +C1's two risks are bounded with C2 as the pre-identified fallback — +neither leaves a requirement unmet. **Selected: C1 (probe; C2 fallback) + +M1 + S1.** + +## 7. Devil's Advocate review of this plan + +- **The escalation actually escalated?** Yes — `IndexSchema.unique` + the + ADR-0025 supersession were put to the user (2026-05-25) and approved, + with simple-mode `add unique index` explicitly kept deferred. The + `IF [NOT] EXISTS` scope was also confirmed (both forms). No autonomous + scope decision. ✓ +- **Reuse vs fork?** `do_add_index`/`do_drop_index` stay the single + executors; the SQL path adds only `unique` + the skip branch. The + redundant-set guard refinement is a *correctness fix the model + extension forces* (plain vs unique are not duplicates), not a fork. ✓ +- **Grammar risk owned — verified in code, not reasoned.** (1) + `INDEX_NAME_EXISTING`/`TABLE_NAME_EXISTING` have `validator: None`, so + `DROP INDEX IF EXISTS ` parses and reaches the skip path (no + hard existence check at parse). (2) `walk_ident`→`consume_ident` does + **not** reject keywords, so a bare `Optional()` would eat `on` — + hence the `DI_SELECTOR`-style `on`-led-first `Choice` (§4.1), proven by + the shipped drop-index selector. (3) Trailing input after a matched + shape is a `Mismatch` (not `Match`), and `decide` commits only on a + full `Match`, so `drop index on T (c)` fails the name-only + `SQL_DROP_INDEX` and falls back to the simple `DI_POSITIONAL` — the + advanced-mode fallback for the positional drop is intact. A parse test + for `create index on T (c)` → `name: None` and a fallback test for + `drop index on T (c)` (advanced) → simple `DropIndex` are on the + checklist. ✓ +- **Second advanced node verified, not assumed?** `decide` is read + (advanced.chain(simple) + first-full-match loop) and a dispatch parse + test is on the checklist (`create table`/`create index`/`drop + table`/`drop index` each to the right command in advanced mode). ✓ +- **Round-trip + rebuild for the new flag?** Explicit checklist items: + YAML `unique: true` save/load identity, older-file default, rebuild + re-emits `CREATE UNIQUE INDEX`, uniqueness actually enforced after + rebuild. Two DDL generators stay in sync (pattern 1): `do_add_index` + and `rebuild_from_text` both gain the `UNIQUE` emit. ✓ +- **Undo parity?** One step per statement (`snapshot_then`); skip is + journalled-not-snapshotted (4c pattern). Undo tests for both create and + drop. ✓ +- **Anything silently dropped?** Simple-mode `add unique index` is + *deferred by user decision*, recorded in the ADR-0025 Amendment — not + silently. The 4i list grows by one (help/usage skeleton refresh for the + new index forms — but these nodes' keys land now, only the *unified + CREATE TABLE* skeleton refresh stays in 4i(a)). Completion merge for + the now-two-advanced-node `create`/`drop` widens 4i(d) — already + tracked, user-confirmed deferred. ✓ +- **`describe`/items-list of a unique index — RESOLVED (user chose "add + in 4d", 2026-05-25).** The structure view rendered every index as + `name (cols)` with no uniqueness marker — a pedagogy gap ("can't tell a + UNIQUE index from a plain one"). Now in scope: §4.7. Structure view is + cheap (`IndexInfo.unique` already populated); the items list needs the + `SchemaCache.table_indexes` value to carry the bit. Both on the + checklist. ✓ + +## 8. Out of 4d scope (tracked, not dropped) +- Simple-mode `add unique index` — deferred by user decision (ADR-0025 + Amendment). +- `ALTER TABLE` (4e–4h). +- 4i(a) CREATE TABLE help/usage skeleton refresh; 4i(b) `describe` of + **table-level constraints** (composite `UNIQUE` + table `CHECK`) — the + *unique-index* display moves **into 4d** (§4.7), but the table-level + constraint display stays 4i(b); 4i(d) shared-entry-word completion + merge (now widened by `create`/`drop` having two advanced nodes); + 4i(e) the simple/advanced completion-colour UX discussion. + +## 9. Implementation sequence (test-first) +1. **Model extension first (M1), isolated** — add `IndexSchema.unique` + + yaml round-trip + `read_schema_snapshot` + rebuild emit + `do_add_index` + `unique` param (default-`false` call site) + the dup-guard refinement. + Red tests: YAML round-trip of a unique index, rebuild survival, + uniqueness enforced, plain≠unique not-a-duplicate → green. **No SQL + surface yet** — this keeps the model change reviewable on its own and + green before any grammar lands. +2. **DROP INDEX (simpler grammar)** — Tier-1 parse + dispatch tests → + command/grammar/builder/REGISTRY/worker(`DropIndexOutcome`+skip)/ + runtime/event/app/catalog → Tier-3 (`tests/sql_drop_index.rs`) → green. +3. **CREATE INDEX** — smoke-probe the leading `Choice`; Tier-1 parse + + flag + dispatch tests (incl. `create index on T (c)` → `name: None`) + → command/grammar/builder/REGISTRY/worker(`CreateIndexOutcome`+skip)/ + runtime/event/app/catalog → Tier-3 (`tests/sql_create_index.rs`, + incl. unique create + round-trip + rebuild + IF NOT EXISTS skip + + duplicate-data refusal) → green. +4. **Unique-index display (§4.7)** — structure-view `[unique]` marker + (render test) + items-list marker (`SchemaCache.table_indexes` carries + the bit; ui test) → green. +5. **Full sweep** — `cargo test` (no regression from 1805) + `cargo + clippy --all-targets -- -D warnings`. +6. **Docs** — ADR-0035 Status note + §13 4d; **ADR-0025 Amendment** (the + UNIQUE-index supersession for advanced mode) + README index-upkeep; + `requirements.md` Q1/C3. Run `/runda` over this slice. Propose commit; + wait for approval. + +## 10. Exit gate +- All §3 items satisfied; four tiers green, zero skips; no regression from + the 1805 baseline; `/runda` / written-DA PASS; clippy clean; ADR-0025 + Amendment + ADR-0035 §13 4d + README + `requirements.md` updated + lockstep. diff --git a/docs/requirements.md b/docs/requirements.md index 3d31e53..fd7eec7 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -168,7 +168,10 @@ handoff-14 cleanup; 449 after B2/C2.) inbound view in the structure renderer; type compatibility validated at declaration via `Type::fk_target_type()`. Indexes done (ADR-0025) — `add index` / `drop index`, - rebuild-preserving, persisted in `project.yaml`. + rebuild-preserving, persisted in `project.yaml`; UNIQUE indexes + added on the advanced-mode SQL surface (`CREATE UNIQUE INDEX`, + ADR-0035 §4d / ADR-0025 Amendment 1; simple-mode `add unique index` + deferred). `NOT NULL` / `UNIQUE` / `CHECK` / `DEFAULT` done (ADR-0029) — a constraint suffix on `create table` / `add column`, plus `add constraint` / `drop constraint` on existing columns; @@ -224,9 +227,11 @@ handoff-14 cleanup; 449 after B2/C2.) keys (4b — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction; self-references and bare `REFERENCES ` supported), then `DROP TABLE [IF EXISTS]` - (4c — reuses `do_drop_table`; `IF EXISTS` is a no-op-with-note)). - Remaining DDL — indexes (4d), `ALTER TABLE` (4e–4h) — is phased per - ADR-0035 §13.)* + (4c — reuses `do_drop_table`; `IF EXISTS` is a no-op-with-note), then + `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` (4d — reuse + `do_add_index`/`do_drop_index`; `CREATE UNIQUE INDEX` admitted in + advanced mode via the `IndexSchema.unique` flag, ADR-0025 Amendment 1)). + Remaining DDL — `ALTER TABLE` (4e–4h) — is phased per ADR-0035 §13.)* - [ ] **Q2** Non-standard syntax rejected with a clear message pointing at the supported subset. *(Design done — ADR-0030 §8: out-of-subset statements are diff --git a/src/app.rs b/src/app.rs index a20adaf..5604c3b 100644 --- a/src/app.rs +++ b/src/app.rs @@ -469,6 +469,24 @@ impl App { )); Vec::new() } + AppEvent::DslDropIndexSkipped { command } => { + // No-op (DROP INDEX IF EXISTS on an absent index, + // ADR-0035 §4d): just the skip note. `target_table()` + // returns the index name for `SqlDropIndex`. + self.note_system(crate::t!( + "ddl.drop_index_skipped_absent", + name = command.target_table() + )); + Vec::new() + } + AppEvent::DslCreateIndexSkipped { command: _, name } => { + // No-op (CREATE INDEX IF NOT EXISTS on an existing index + // name, ADR-0035 §4d): the skip note carries the resolved + // index name (the unnamed form's auto-name isn't on the + // command). No structure shown. + self.note_system(crate::t!("ddl.create_index_skipped_exists", name = name)); + Vec::new() + } AppEvent::DslDataSucceeded { command, data } => { self.handle_dsl_query_success(&command, &data); Vec::new() @@ -1603,6 +1621,12 @@ impl App { RelationshipSelector::Named { .. } => (Operation::DropRelationship, None, None), }, C::AddIndex { table, .. } => (Operation::AddIndex, Some(table.as_str()), None), + // SQL `CREATE [UNIQUE] INDEX` shares the add-index operation + // (it reuses `do_add_index`); route engine/validation errors + // through it with the parsed table. + C::SqlCreateIndex { table, .. } => { + (Operation::AddIndex, Some(table.as_str()), None) + } C::AddConstraint { table, column, .. } => ( Operation::AddConstraint, Some(table.as_str()), @@ -1619,6 +1643,9 @@ impl App { } IndexSelector::Named { .. } => (Operation::DropIndex, None, None), }, + // The SQL `DROP INDEX` is name-only (the table is resolved by + // the executor), like the named DSL drop. + C::SqlDropIndex { .. } => (Operation::DropIndex, None, None), C::Insert { table, .. } => (Operation::Insert, Some(table.as_str()), None), C::Update { table, .. } => (Operation::Update, Some(table.as_str()), None), C::Delete { table, .. } => (Operation::Delete, Some(table.as_str()), None), diff --git a/src/completion.rs b/src/completion.rs index 9ee3664..d2d85d4 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -48,9 +48,19 @@ pub struct SchemaCache { /// case-insensitive in `columns_for_table` so the walker /// can resolve `Customers` regardless of how it was typed. pub table_columns: std::collections::HashMap>, - /// Per-table user index names (ADR-0025). Keyed by table - /// name; drives the nested tables/indexes items panel (S2). - pub table_indexes: std::collections::HashMap>, + /// Per-table user indexes (ADR-0025). Keyed by table name; drives + /// the nested tables/indexes items panel (S2). Each entry carries + /// the index's uniqueness so the panel can mark a UNIQUE index + /// (ADR-0035 §4d). + pub table_indexes: std::collections::HashMap>, +} + +/// One per-table index for the items panel (ADR-0025 / ADR-0035 §4d): +/// its name and whether it is a UNIQUE index. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IndexEntry { + pub name: String, + pub unique: bool, } /// One column's user-facing type info, scoped to a table diff --git a/src/db.rs b/src/db.rs index 8dbc814..1f6c658 100644 --- a/src/db.rs +++ b/src/db.rs @@ -486,6 +486,28 @@ enum Request { source: Option, reply: oneshot::Sender>, }, + /// Advanced-mode SQL `DROP INDEX [IF EXISTS] ` (ADR-0035 §4d). + /// Executes through `do_drop_index`; `if_exists` turns an absent + /// index into a no-op (`DropIndexOutcome::Skipped`, no snapshot). + SqlDropIndex { + name: String, + if_exists: bool, + source: Option, + reply: oneshot::Sender>, + }, + /// Advanced-mode SQL `CREATE [UNIQUE] INDEX [IF NOT EXISTS]` + /// (ADR-0035 §4d). Executes through `do_add_index` (with `unique`); + /// `if_not_exists` turns an existing index name into a no-op + /// (`CreateIndexOutcome::Skipped`, no snapshot). + SqlCreateIndex { + name: Option, + table: String, + columns: Vec, + unique: bool, + if_not_exists: bool, + source: Option, + reply: oneshot::Sender>, + }, AddColumn { table: String, column: ColumnSpec, @@ -894,6 +916,53 @@ impl Database { recv.await.map_err(|_| DbError::WorkerGone)? } + /// Advanced-mode SQL `DROP INDEX [IF EXISTS]` (ADR-0035 §4d). + /// Returns whether the index was dropped (with the affected table's + /// structure) or skipped (the `IF EXISTS` no-op on an absent index). + pub async fn sql_drop_index( + &self, + name: String, + if_exists: bool, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::SqlDropIndex { + name, + if_exists, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + + /// Advanced-mode SQL `CREATE [UNIQUE] INDEX [IF NOT EXISTS]` + /// (ADR-0035 §4d). Returns whether the index was created (with the + /// affected table's structure) or skipped (the `IF NOT EXISTS` no-op + /// on an existing index name, carrying the resolved name). + pub async fn sql_create_index( + &self, + name: Option, + table: String, + columns: Vec, + unique: bool, + if_not_exists: bool, + source: Option, + ) -> Result { + let (reply, recv) = oneshot::channel(); + self.send(Request::SqlCreateIndex { + name, + table, + columns, + unique, + if_not_exists, + source, + reply, + }) + .await?; + recv.await.map_err(|_| DbError::WorkerGone)? + } + pub async fn add_column( &self, table: String, @@ -1950,6 +2019,10 @@ fn handle_request( name.as_deref(), &table, &columns, + // Simple-mode `add index` is always non-unique + // (ADR-0025); `add unique index` stays deferred. The SQL + // `CREATE UNIQUE INDEX` path passes `true` (ADR-0035 §4d). + false, )); } Request::DropIndex { @@ -1964,6 +2037,76 @@ fn handle_request( &selector, )); } + Request::SqlDropIndex { + name, + if_exists, + source, + reply, + } => { + // `IF EXISTS` on an absent index is a no-op: reply `Skipped` + // and take **no** snapshot (nothing to undo). The submitted + // line is still journalled (the 4c skip pattern, ADR-0034 / + // ADR-0035 §4). Existence uses the same user-index lookup as + // `do_drop_index` (`sql IS NOT NULL`). + if if_exists && !index_exists(conn, &name, true).unwrap_or(false) { + let result = (|| { + if let (Some(p), Some(text)) = (persistence, source.as_deref()) { + p.append_history(text).map_err(DbError::from_persistence)?; + } + Ok(DropIndexOutcome::Skipped) + })(); + let _ = reply.send(result); + } else { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_drop_index( + conn, + persistence, + source.as_deref(), + &IndexSelector::Named { name: name.clone() }, + ) + .map(DropIndexOutcome::Dropped) + }); + } + } + Request::SqlCreateIndex { + name, + table, + columns, + unique, + if_not_exists, + source, + reply, + } => { + // `IF NOT EXISTS` short-circuits only a *name* collision into + // a no-op (reply `Skipped`, no snapshot, line journalled — the + // 4c skip pattern). The name uses the broad lookup (any index + // of that name), matching `do_add_index`'s collision guard. + // A *different*-named but redundant-column-set create still + // hits `do_add_index`'s redundant-set refusal (ADR-0025). + let resolved = resolve_index_name(name.as_deref(), &table, &columns); + if if_not_exists && index_exists(conn, &resolved, false).unwrap_or(false) { + let result = (|| { + if let (Some(p), Some(text)) = (persistence, source.as_deref()) { + p.append_history(text).map_err(DbError::from_persistence)?; + } + Ok(CreateIndexOutcome::Skipped(resolved.clone())) + })(); + let _ = reply.send(result); + } else { + snapshot_then(snap, batch, conn, source.as_deref(), reply, || { + do_add_index( + conn, + persistence, + source.as_deref(), + name.as_deref(), + &table, + &columns, + unique, + ) + .map(CreateIndexOutcome::Created) + }); + } + } Request::AddConstraint { table, column, @@ -2415,6 +2558,7 @@ fn read_schema_snapshot(conn: &Connection) -> Result { name: idx.name, table: name.clone(), columns: idx.columns, + unique: idx.unique, }); } } @@ -2699,6 +2843,34 @@ pub enum DropOutcome { Skipped, } +/// The result of an advanced-mode SQL `DROP INDEX` (ADR-0035 §4d). +/// +/// Either the index was dropped — `Dropped` carries the affected +/// table's structure so the runtime auto-shows the now de-indexed table +/// (ADR-0014), unlike `DROP TABLE` whose table is gone — or `IF EXISTS` +/// matched no index and the statement was a no-op driving the "doesn't +/// exist — skipped" note (the index name comes from the command). +#[derive(Debug)] +pub enum DropIndexOutcome { + Dropped(TableDescription), + Skipped, +} + +/// The result of an advanced-mode SQL `CREATE [UNIQUE] INDEX` +/// (ADR-0035 §4d). +/// +/// Either the index was created — `Created` carries the affected table's +/// structure for the auto-show (ADR-0014) — or `IF NOT EXISTS` matched +/// an existing index name and the statement was a no-op. `Skipped` +/// carries the **resolved** index name (the auto-name is unknown to the +/// command for the unnamed form) to drive the "already exists — skipped" +/// note. +#[derive(Debug)] +pub enum CreateIndexOutcome { + Created(TableDescription), + Skipped(String), +} + #[allow(clippy::too_many_arguments)] fn do_create_table( conn: &Connection, @@ -5859,6 +6031,38 @@ fn do_drop_relationship( /// Refuses a redundant index on an already-indexed column set /// and a name collision. The index name is auto-generated as /// `
__idx` when not supplied. +/// Resolve an index name: the user-given name, or the ADR-0025 +/// auto-name `
__idx`. Shared by `do_add_index` and the +/// `CREATE INDEX IF NOT EXISTS` skip pre-check (ADR-0035 §4d) so both +/// compute the same name. +fn resolve_index_name(name: Option<&str>, table: &str, columns: &[String]) -> String { + name.map_or_else( + || format!("{table}_{}_idx", columns.join("_")), + ToString::to_string, + ) +} + +/// Whether an index named `name` exists (ADR-0035 §4d skip checks). +/// +/// `user_only = true` counts only explicit `CREATE INDEX` objects +/// (`sql IS NOT NULL`), matching `do_drop_index`'s named lookup — used +/// by the `DROP INDEX IF EXISTS` skip. `user_only = false` counts any +/// index of that name (incl. the automatic PK / UNIQUE-constraint +/// indexes), matching `do_add_index`'s name-collision guard — used by +/// the `CREATE INDEX IF NOT EXISTS` skip. +fn index_exists(conn: &Connection, name: &str, user_only: bool) -> Result { + let sql = if user_only { + "SELECT COUNT(*) FROM sqlite_master \ + WHERE type = 'index' AND name = ?1 AND sql IS NOT NULL;" + } else { + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'index' AND name = ?1;" + }; + let count: i64 = conn + .query_row(sql, [name], |row| row.get(0)) + .map_err(DbError::from_rusqlite)?; + Ok(count > 0) +} + fn do_add_index( conn: &Connection, persistence: Option<&Persistence>, @@ -5866,7 +6070,21 @@ fn do_add_index( name: Option<&str>, table: &str, columns: &[String], + unique: bool, ) -> Result { + // 0. Internal `__rdbms_*` tables are not user tables (they are + // filtered from `list_tables` and never offered in completion), so + // indexing one is refused as "no such table" — the same opacity + // the rest of the app presents. Guards BOTH the simple `add index` + // and the SQL `CREATE INDEX` surfaces, since both reach here + // (ADR-0025 / ADR-0035 §4d; the grammar's `reject_internal_table` + // only covers the typed SQL family, not the simple node). + if table.to_ascii_lowercase().starts_with("__rdbms_") { + return Err(DbError::Sqlite { + message: format!("no such table: {table}"), + kind: SqliteErrorKind::NoSuchTable, + }); + } // 1. Table must exist; gather its columns. let schema = read_schema(conn, table)?; // 2. Every indexed column must exist on the table. @@ -5878,11 +6096,17 @@ fn do_add_index( }); } } - // 3. Refuse a redundant index over an identical column set. + // 3. Refuse a redundant index over an identical column set *of the + // same kind*. A plain and a unique index over the same columns are + // NOT redundant (the unique one enforces a constraint the plain one + // does not), so the guard keys on `(columns, unique)` (ADR-0035 + // §4d). To hold both, the user must name them distinctly — the + // auto-name is identical, so the name guard (step 5) would + // otherwise collide. let existing = read_table_indexes(conn, table)?; if let Some(dup) = existing .iter() - .find(|i| i.columns.as_slice() == columns) + .find(|i| i.columns.as_slice() == columns && i.unique == unique) { return Err(DbError::Unsupported(format!( "the columns ({}) of `{table}` are already indexed by `{}`.", @@ -5891,10 +6115,7 @@ fn do_add_index( ))); } // 4. Resolve the index name (auto-generate when omitted). - let resolved = name.map_or_else( - || format!("{table}_{}_idx", columns.join("_")), - ToString::to_string, - ); + let resolved = resolve_index_name(name, table, columns); // 5. Refuse a name collision. let name_taken: i64 = conn .query_row( @@ -5919,13 +6140,14 @@ fn do_add_index( .map(|c| quote_ident(c)) .collect::>() .join(", "); + let unique_kw = if unique { "UNIQUE " } else { "" }; let ddl = format!( - "CREATE INDEX {idx} ON {tbl} ({cols});", + "CREATE {unique_kw}INDEX {idx} ON {tbl} ({cols});", idx = quote_ident(&resolved), tbl = quote_ident(table), cols = cols_csv, ); - debug!(ddl = %ddl, "add_index"); + debug!(ddl = %ddl, unique, "add_index"); tx.execute_batch(&ddl).map_err(DbError::from_rusqlite)?; let description = do_describe_table(conn, table)?; let changes = Changes { @@ -7818,8 +8040,12 @@ fn do_rebuild_from_text( .map(|c| quote_ident(c)) .collect::>() .join(", "); + // ADR-0035 §4d: a UNIQUE index round-trips its uniqueness, so + // re-emit `CREATE UNIQUE INDEX` — otherwise a rebuild would + // silently demote it to a plain index. + let unique_kw = if index.unique { "UNIQUE " } else { "" }; tx.execute_batch(&format!( - "CREATE INDEX {idx} ON {tbl} ({cols});", + "CREATE {unique_kw}INDEX {idx} ON {tbl} ({cols});", idx = quote_ident(&index.name), tbl = quote_ident(&index.table), )) diff --git a/src/dsl/command.rs b/src/dsl/command.rs index b56df31..92dbfb7 100644 --- a/src/dsl/command.rs +++ b/src/dsl/command.rs @@ -278,6 +278,29 @@ pub enum Command { DropIndex { selector: IndexSelector, }, + /// Advanced-mode SQL `DROP INDEX [IF EXISTS] ` (ADR-0035 §4, + /// sub-phase 4d). Name-only (SQL has no positional column form — that + /// is the simple `drop index on T(…)`). Executes through the same + /// `do_drop_index` machinery as [`Self::DropIndex`]; `if_exists` + /// turns an absent index into a no-op-with-note rather than an error. + SqlDropIndex { + name: String, + if_exists: bool, + }, + /// Advanced-mode SQL `CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] + /// ON
(, …)` (ADR-0035 §4d). Executes through the same + /// `do_add_index` machinery as [`Self::AddIndex`] (the columns/ + /// auto-name reuse), plus the `unique` flag (simple mode has no + /// `add unique index` — that stays deferred per ADR-0025). `name` is + /// `None` for the unnamed form (auto-named at execution); + /// `if_not_exists` makes an existing index name a no-op-with-note. + SqlCreateIndex { + name: Option, + table: String, + columns: Vec, + unique: bool, + if_not_exists: bool, + }, /// Add a column-level constraint to an existing column /// (ADR-0029 §2.2). Applied through the rebuild-table /// primitive after a §5 dry-run guards populated columns. @@ -710,6 +733,8 @@ impl Command { Self::DropRelationship { .. } => "drop relationship", Self::AddIndex { .. } => "add index", Self::DropIndex { .. } => "drop index", + Self::SqlDropIndex { .. } => "drop index", + Self::SqlCreateIndex { .. } => "create index", Self::AddConstraint { .. } => "add constraint", Self::DropConstraint { .. } => "drop constraint", Self::ShowTable { .. } => "show table", @@ -783,6 +808,11 @@ impl Command { // sensible fallback for logging. IndexSelector::Named { name } => name, }, + // The SQL drop is name-only; the index name identifies it + // until the executor resolves the table (mirrors the named + // `DropIndex` / `SqlDropTable` fallback). + Self::SqlDropIndex { name, .. } => name, + Self::SqlCreateIndex { table, .. } => table, // Replay isn't tied to a single table; the path is // the most identifying thing for log output. Self::Replay { path } => path, diff --git a/src/dsl/grammar/ddl.rs b/src/dsl/grammar/ddl.rs index 4f84398..fb7198f 100644 --- a/src/dsl/grammar/ddl.rs +++ b/src/dsl/grammar/ddl.rs @@ -203,6 +203,21 @@ static SQL_DROP_TABLE_SHAPE_NODES: &[Node] = &[ ]; const SQL_DROP_TABLE_SHAPE: Node = Node::Seq(SQL_DROP_TABLE_SHAPE_NODES); +// Advanced-mode SQL `DROP INDEX [IF EXISTS] [;]` (ADR-0035 §4, +// sub-phase 4d). Name-only — SQL has no positional `on T (cols)` drop +// form (that stays the simple `drop index on …`, which falls back to +// the simple `drop` node). Leads on the concrete `index` keyword; the +// `IF EXISTS` opt is mid-`Seq` (trap-safe, like SQL_DROP_TABLE). +// `INDEX_NAME_EXISTING` has `validator: None`, so `IF EXISTS ` +// still parses and reaches the skip path. +static SQL_DROP_INDEX_SHAPE_NODES: &[Node] = &[ + Node::Word(Word::keyword("index")), + SQL_DROP_IF_EXISTS_OPT, + INDEX_NAME_EXISTING, + Node::Optional(&Node::Punct(';')), +]; +const SQL_DROP_INDEX_SHAPE: Node = Node::Seq(SQL_DROP_INDEX_SHAPE_NODES); + // ================================================================= // drop_column — `drop column [from] [table] : ` // ================================================================= @@ -1726,6 +1741,106 @@ pub static SQL_DROP_TABLE: CommandNode = CommandNode { usage_ids: &["parse.usage.sql_drop_table"], }; +/// Build a `Command::SqlDropIndex` from the advanced-mode SQL +/// `DROP INDEX [IF EXISTS] ` shape (ADR-0035 §4, sub-phase 4d). +/// `if` appears only in the `IF EXISTS` prefix, so its presence is the +/// flag (mirroring `build_sql_drop_table`). +fn build_sql_drop_index(path: &MatchedPath, _source: &str) -> Result { + Ok(Command::SqlDropIndex { + name: require_ident(path, "index_name")?, + if_exists: path.contains_word("if"), + }) +} + +pub static SQL_DROP_INDEX: CommandNode = CommandNode { + entry: Word::keyword("drop"), + shape: SQL_DROP_INDEX_SHAPE, + ast_builder: build_sql_drop_index, + help_id: Some("ddl.sql_drop_index"), + usage_ids: &["parse.usage.sql_drop_index"], +}; + +// ================================================================= +// SQL `CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` +// (ADR-0035 §4d). Entry word `create` — `create`'s *second* advanced +// node (alongside SQL_CREATE_TABLE). +// ================================================================= + +// Leading `[UNIQUE]` prefix as a `Choice` whose every branch starts on a +// concrete keyword (`unique index` | `index`) — the trap-safe form (the +// §3 rule forbids a leading *Optional*, not a leading `Choice`). The +// builder reads `unique` presence via `contains_word("unique")`. +static SQL_CI_UNIQUE_INDEX_NODES: &[Node] = + &[Node::Word(Word::keyword("unique")), Node::Word(Word::keyword("index"))]; +const SQL_CI_UNIQUE_INDEX: Node = Node::Seq(SQL_CI_UNIQUE_INDEX_NODES); +static SQL_CI_LEAD_CHOICES: &[Node] = + &[SQL_CI_UNIQUE_INDEX, Node::Word(Word::keyword("index"))]; +const SQL_CI_LEAD: Node = Node::Choice(SQL_CI_LEAD_CHOICES); + +static SQL_CI_IF_NOT_EXISTS_NODES: &[Node] = &[ + Node::Word(Word::keyword("if")), + Node::Word(Word::keyword("not")), + Node::Word(Word::keyword("exists")), +]; +const SQL_CI_IF_NOT_EXISTS_OPT: Node = Node::Optional(&Node::Seq(SQL_CI_IF_NOT_EXISTS_NODES)); + +// The name/`on` selector. The **unnamed** (`on`-led) branch comes FIRST, +// relying on `Choice` backtracking — exactly the shipped `DI_SELECTOR` +// pattern (`DI_POSITIONAL` first). A bare `Optional()` would +// instead greedily consume the `on` keyword (`consume_ident` does not +// reject keywords), breaking the unnamed form. +static SQL_CI_UNNAMED_NODES: &[Node] = &[ + Node::Word(Word::keyword("on")), + TABLE_NAME_EXISTING, + Node::Punct('('), + INDEX_COLUMN_LIST, + Node::Punct(')'), +]; +const SQL_CI_UNNAMED: Node = Node::Seq(SQL_CI_UNNAMED_NODES); +static SQL_CI_NAMED_NODES: &[Node] = &[ + INDEX_NAME_NEW, + Node::Word(Word::keyword("on")), + TABLE_NAME_EXISTING, + Node::Punct('('), + INDEX_COLUMN_LIST, + Node::Punct(')'), +]; +const SQL_CI_NAMED: Node = Node::Seq(SQL_CI_NAMED_NODES); +static SQL_CI_SELECTOR_CHOICES: &[Node] = &[SQL_CI_UNNAMED, SQL_CI_NAMED]; +const SQL_CI_SELECTOR: Node = Node::Choice(SQL_CI_SELECTOR_CHOICES); + +static SQL_CREATE_INDEX_SHAPE_NODES: &[Node] = &[ + SQL_CI_LEAD, + SQL_CI_IF_NOT_EXISTS_OPT, + SQL_CI_SELECTOR, + Node::Optional(&Node::Punct(';')), +]; +const SQL_CREATE_INDEX_SHAPE: Node = Node::Seq(SQL_CREATE_INDEX_SHAPE_NODES); + +/// Build a `Command::SqlCreateIndex` from the advanced-mode SQL +/// `CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` shape +/// (ADR-0035 §4d). `unique`/`if_not_exists` are keyword-presence flags +/// (`unique` only in the lead; `if` only in `IF NOT EXISTS`); the name +/// is present iff the `SQL_CI_NAMED` branch matched. Columns / table +/// extraction mirrors the simple `add index` builder. +fn build_sql_create_index(path: &MatchedPath, _source: &str) -> Result { + Ok(Command::SqlCreateIndex { + name: ident(path, "index_name").map(str::to_string), + table: require_ident(path, "table_name")?, + columns: collect_idents(path, "column_name"), + unique: path.contains_word("unique"), + if_not_exists: path.contains_word("if"), + }) +} + +pub static SQL_CREATE_INDEX: CommandNode = CommandNode { + entry: Word::keyword("create"), + shape: SQL_CREATE_INDEX_SHAPE, + ast_builder: build_sql_create_index, + help_id: Some("ddl.sql_create_index"), + usage_ids: &["parse.usage.sql_create_index"], +}; + // ================================================================= // Tests — `create table` column constraints (ADR-0029 §2.1, §9) // ================================================================= @@ -1994,3 +2109,187 @@ mod sql_drop_table_tests { )); } } + +#[cfg(test)] +mod sql_drop_index_tests { + use crate::dsl::command::{Command, IndexSelector}; + use crate::dsl::parser::parse_command_in_mode; + use crate::mode::Mode; + + fn drop_index_fields(input: &str) -> (String, bool) { + match parse_command_in_mode(input, Mode::Advanced).expect("should parse") { + Command::SqlDropIndex { name, if_exists } => (name, if_exists), + other => panic!("expected SqlDropIndex, got {other:?}"), + } + } + + #[test] + fn drop_index_parses_as_sql_drop_index_in_advanced_mode() { + let (name, if_exists) = drop_index_fields("drop index Orders_CustId_idx"); + assert_eq!(name, "Orders_CustId_idx"); + assert!(!if_exists); + } + + #[test] + fn if_exists_sets_the_flag() { + let (name, if_exists) = drop_index_fields("drop index if exists ix"); + assert_eq!(name, "ix"); + assert!(if_exists); + // trailing semicolon tolerated + assert!(drop_index_fields("drop index if exists ix;").1); + } + + #[test] + fn drop_table_and_drop_index_each_dispatch_to_the_right_advanced_node() { + // `drop` now has *two* advanced nodes (SQL_DROP_TABLE + + // SQL_DROP_INDEX); the dispatcher must try both and pick the + // shape that matches (ADR-0035 §4d — the second-advanced-node + // case). + assert!(matches!( + parse_command_in_mode("drop table Orders", Mode::Advanced).expect("parses"), + Command::SqlDropTable { .. } + )); + assert!(matches!( + parse_command_in_mode("drop index ix", Mode::Advanced).expect("parses"), + Command::SqlDropIndex { .. } + )); + } + + #[test] + fn positional_drop_index_falls_back_to_the_simple_node_in_advanced_mode() { + // The SQL form is name-only; `drop index on T (cols)` is the + // simple positional form. The name-only SQL shape can't fully + // match it (trailing `(cols)`), so it falls back to the simple + // `drop` node's `DropIndex { Columns }` even in advanced mode. + match parse_command_in_mode("drop index on Orders (CustId)", Mode::Advanced) + .expect("parses") + { + Command::DropIndex { + selector: IndexSelector::Columns { table, columns }, + } => { + assert_eq!(table, "Orders"); + assert_eq!(columns, vec!["CustId".to_string()]); + } + other => panic!("expected positional DropIndex, got {other:?}"), + } + } + + #[test] + fn named_drop_index_in_simple_mode_is_the_dsl_command() { + // In simple mode the SQL node is gated; `drop index ix` is the + // simple `DropIndex { Named }`. + match parse_command_in_mode("drop index ix", Mode::Simple).expect("parses") { + Command::DropIndex { + selector: IndexSelector::Named { name }, + } => assert_eq!(name, "ix"), + other => panic!("expected named DropIndex, got {other:?}"), + } + } +} + +#[cfg(test)] +mod sql_create_index_tests { + use crate::dsl::command::Command; + use crate::dsl::parser::parse_command_in_mode; + use crate::mode::Mode; + + struct Ci { + name: Option, + table: String, + columns: Vec, + unique: bool, + if_not_exists: bool, + } + + fn ci(input: &str) -> Ci { + match parse_command_in_mode(input, Mode::Advanced).expect("should parse") { + Command::SqlCreateIndex { + name, + table, + columns, + unique, + if_not_exists, + } => Ci { name, table, columns, unique, if_not_exists }, + other => panic!("expected SqlCreateIndex, got {other:?}"), + } + } + + #[test] + fn named_create_index_parses() { + let c = ci("create index ix on Customers (email)"); + assert_eq!(c.name.as_deref(), Some("ix")); + assert_eq!(c.table, "Customers"); + assert_eq!(c.columns, vec!["email".to_string()]); + assert!(!c.unique); + assert!(!c.if_not_exists); + } + + #[test] + fn unnamed_create_index_leaves_name_none() { + // The unnamed form: the optional name must NOT swallow `on` + // (the `DI_SELECTOR`-style on-led-first selector handles it). + let c = ci("create index on Customers (email)"); + assert_eq!(c.name, None); + assert_eq!(c.table, "Customers"); + assert_eq!(c.columns, vec!["email".to_string()]); + } + + #[test] + fn unique_sets_the_flag() { + let c = ci("create unique index ux on Customers (email)"); + assert!(c.unique); + assert_eq!(c.name.as_deref(), Some("ux")); + // unnamed unique form too + let c2 = ci("create unique index on Customers (email)"); + assert!(c2.unique); + assert_eq!(c2.name, None); + } + + #[test] + fn if_not_exists_sets_the_flag() { + let c = ci("create index if not exists ix on Customers (email)"); + assert!(c.if_not_exists); + assert_eq!(c.name.as_deref(), Some("ix")); + // combined with unique + unnamed + trailing semicolon + let c2 = ci("create unique index if not exists on Customers (email);"); + assert!(c2.unique && c2.if_not_exists); + assert_eq!(c2.name, None); + } + + #[test] + fn multi_column_index_parses() { + let c = ci("create index on Orders (CustId, Date)"); + assert_eq!(c.columns, vec!["CustId".to_string(), "Date".to_string()]); + } + + #[test] + fn create_table_and_create_index_each_dispatch_to_the_right_advanced_node() { + // `create` now has *two* advanced nodes (SQL_CREATE_TABLE + + // SQL_CREATE_INDEX); the dispatcher must try both (ADR-0035 §4d). + assert!(matches!( + parse_command_in_mode("create table T (id int primary key)", Mode::Advanced) + .expect("parses"), + Command::SqlCreateTable { .. } + )); + assert!(matches!( + parse_command_in_mode("create index ix on T (id)", Mode::Advanced).expect("parses"), + Command::SqlCreateIndex { .. } + )); + assert!(matches!( + parse_command_in_mode("create unique index ux on T (id)", Mode::Advanced) + .expect("parses"), + Command::SqlCreateIndex { unique: true, .. } + )); + } + + #[test] + fn simple_create_table_dsl_still_parses_in_advanced_mode() { + // The `create table … with pk …` DSL form falls back to the + // simple node even with two advanced `create` nodes present. + assert!(matches!( + parse_command_in_mode("create table T with pk id(serial)", Mode::Advanced) + .expect("parses"), + Command::CreateTable { .. } + )); + } +} diff --git a/src/dsl/grammar/mod.rs b/src/dsl/grammar/mod.rs index 312ade4..a426814 100644 --- a/src/dsl/grammar/mod.rs +++ b/src/dsl/grammar/mod.rs @@ -585,16 +585,24 @@ pub static REGISTRY: &[(&CommandNode, CommandCategory)] = &[ (&data::SQL_UPDATE, CommandCategory::Advanced), (&data::SQL_DELETE, CommandCategory::Advanced), // Shared entry word `create` (ADR-0035 §2): the simple - // `ddl::CREATE` (above) and this advanced SQL node. The - // dispatcher tries SQL first in advanced mode and falls back to - // the `create table … with pk …` DSL node when the SQL shape - // does not match — the `insert` precedent. + // `ddl::CREATE` (above) and these advanced SQL nodes. The + // dispatcher tries the advanced candidates first in advanced mode + // and falls back to the `create table … with pk …` DSL node when no + // SQL shape matches — the `insert` precedent. 4d adds + // SQL_CREATE_INDEX, so `create` now has *two* advanced nodes; + // `decide` tries both (`create table …` → SQL_CREATE_TABLE, + // `create [unique] index …` → SQL_CREATE_INDEX). (&ddl::SQL_CREATE_TABLE, CommandCategory::Advanced), - // Shared `drop` entry word: `ddl::DROP` (simple) and this advanced - // SQL node. SQL-first in advanced mode; `drop table [if exists] T` - // matches here while `drop column`/`drop relationship`/`drop index` - // fall back to the simple `drop` node. + (&ddl::SQL_CREATE_INDEX, CommandCategory::Advanced), + // Shared `drop` entry word: `ddl::DROP` (simple) and these advanced + // SQL nodes. SQL-first in advanced mode; `drop table [if exists] T` + // → SQL_DROP_TABLE, `drop index [if exists] ` → SQL_DROP_INDEX + // (4d — `drop` now has *two* advanced nodes; the dispatcher's + // `decide` tries all advanced candidates). `drop column`/`drop + // relationship`/`drop index on T(…)` fall back to the simple `drop` + // node. (&ddl::SQL_DROP_TABLE, CommandCategory::Advanced), + (&ddl::SQL_DROP_INDEX, CommandCategory::Advanced), ]; /// Whether `entry` names an advanced-mode-only command (ADR-0030 diff --git a/src/event.rs b/src/event.rs index 9456783..fad3e02 100644 --- a/src/event.rs +++ b/src/event.rs @@ -41,6 +41,20 @@ pub enum AppEvent { DslDropSkipped { command: Command, }, + /// A SQL `DROP INDEX IF EXISTS` matched no index — a no-op + /// (ADR-0035 §4d). Renders an index-specific "doesn't exist — + /// skipped" note; no structure to show. + DslDropIndexSkipped { + command: Command, + }, + /// A SQL `CREATE INDEX IF NOT EXISTS` matched an existing index name + /// — a no-op (ADR-0035 §4d). `name` is the resolved index name (the + /// auto-name is not on the command). Renders "already exists — + /// skipped"; no structure to show. + DslCreateIndexSkipped { + command: Command, + name: String, + }, /// A `show data` query succeeded. DslDataSucceeded { command: Command, data: DataResult }, /// An `explain …` command succeeded (ADR-0028). `plan` diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index 1d96dc1..0209d87 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -173,9 +173,14 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("help.ddl.create", &[]), ("help.ddl.sql_create_table", &[]), ("help.ddl.sql_drop_table", &[]), + ("help.ddl.sql_create_index", &[]), + ("help.ddl.sql_drop_index", &[]), // Advanced-mode SQL CREATE TABLE / DROP TABLE no-op notes (ADR-0035 §4). ("ddl.create_skipped_exists", &["name"]), ("ddl.drop_skipped_absent", &["name"]), + // Advanced-mode SQL CREATE INDEX / DROP INDEX no-op notes (ADR-0035 §4d). + ("ddl.create_index_skipped_exists", &["name"]), + ("ddl.drop_index_skipped_absent", &["name"]), ("help.ddl.drop", &[]), ("help.ddl.add", &[]), ("help.ddl.rename", &[]), @@ -248,6 +253,8 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("parse.usage.create_table", &[]), ("parse.usage.sql_create_table", &[]), ("parse.usage.sql_drop_table", &[]), + ("parse.usage.sql_create_index", &[]), + ("parse.usage.sql_drop_index", &[]), ("parse.usage.delete", &[]), ("parse.usage.drop_column", &[]), ("parse.usage.drop_constraint", &[]), diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index ffeb989..8d2b3a4 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -265,6 +265,11 @@ help: [, primary key (, ...)]) — create a table (advanced SQL) sql_drop_table: |- drop table [if exists] — remove a table (advanced SQL) + sql_create_index: |- + create [unique] index [if not exists] [] on (, ...) + — create an index (advanced SQL) + sql_drop_index: |- + drop index [if exists] — remove an index (advanced SQL) drop: |- drop table — remove a table drop column [from] [table] : [--cascade] — remove a column @@ -382,6 +387,12 @@ ddl: # `drop table if exists ` where the table is absent: a no-op that # succeeds with this note instead of a "doesn't exist" error. drop_skipped_absent: "table '{name}' doesn't exist — skipped (no changes made)" + # `create [unique] index if not exists …` where the index name + # already exists: a no-op that succeeds with this note (ADR-0035 §4d). + create_index_skipped_exists: "index '{name}' already exists — skipped (no changes made)" + # `drop index if exists ` where the index is absent: a no-op that + # succeeds with this note instead of a "doesn't exist" error. + drop_index_skipped_absent: "index '{name}' doesn't exist — skipped (no changes made)" parse: # Wrapper around chumsky's structural error message. The @@ -452,6 +463,8 @@ parse: create_table: "create table with pk [()[, ...]]" sql_create_table: "create table [if not exists] ( [not null] [unique] [primary key], ... [, primary key (, ...)])" sql_drop_table: "drop table [if exists] " + sql_create_index: "create [unique] index [if not exists] [] on
([, ...])" + sql_drop_index: "drop index [if exists] " drop_table: "drop table " drop_column: "drop column [from] [table]
: " drop_relationship: |- diff --git a/src/output_render.rs b/src/output_render.rs index 4f0b82d..c42e512 100644 --- a/src/output_render.rs +++ b/src/output_render.rs @@ -137,12 +137,15 @@ pub fn render_structure(desc: &TableDescription) -> Vec { } // Indexes section (ADR-0025), shown only when the table - // carries at least one user-created index. + // carries at least one user-created index. A UNIQUE index is + // marked `[unique]` so a learner can tell a uniqueness-enforcing + // index from a performance-only one (ADR-0035 §4d). if !desc.indexes.is_empty() { out.push("Indexes:".to_string()); for index in &desc.indexes { + let unique = if index.unique { " [unique]" } else { "" }; out.push(format!( - " {} ({})", + " {} ({}){unique}", index.name, index.columns.join(", "), )); @@ -797,6 +800,30 @@ mod tests { let out = render_structure(&desc).join("\n"); assert!(out.contains("Indexes:"), "got:\n{out}"); assert!(out.contains("idx_email (Email)"), "got:\n{out}"); + // A plain index carries no uniqueness marker. + assert!(!out.contains("[unique]"), "plain index unmarked; got:\n{out}"); + } + + #[test] + fn render_structure_marks_a_unique_index() { + // ADR-0035 §4d: a UNIQUE index is marked `[unique]` so a learner + // can tell it from a performance-only index. + let desc = TableDescription { + name: "Customers".to_string(), + columns: vec![ + col("id", Type::Serial, true, false), + col("Email", Type::Text, false, false), + ], + outbound_relationships: Vec::new(), + inbound_relationships: Vec::new(), + indexes: vec![IndexInfo { + name: "uidx_email".to_string(), + columns: vec!["Email".to_string()], + unique: true, + }], + }; + let out = render_structure(&desc).join("\n"); + assert!(out.contains("uidx_email (Email) [unique]"), "got:\n{out}"); } #[test] diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index a62f119..c1faeb3 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -186,6 +186,14 @@ pub struct IndexSchema { pub table: String, /// The indexed columns, in index order. pub columns: Vec, + /// Whether this is a `UNIQUE` index (ADR-0035 §4d — advanced-mode + /// `CREATE UNIQUE INDEX`). The engine reports it via + /// `pragma_index_list`'s `unique` column, so it is read back rather + /// than stored in any `__rdbms_*` table; it is carried here so it + /// round-trips through `project.yaml` and survives `rebuild`. + /// Defaults to `false` when missing in older project files (the YAML + /// field is optional on read); `version` stays `1`. + pub unique: bool, } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/persistence/yaml.rs b/src/persistence/yaml.rs index 3a87e43..f46bc51 100644 --- a/src/persistence/yaml.rs +++ b/src/persistence/yaml.rs @@ -74,6 +74,12 @@ fn write_index(out: &mut String, index: &IndexSchema) { out.push_str("e_if_needed(col)); } let _ = writeln!(out, "]"); + // Emit `unique` only when true (ADR-0035 §4d), matching the + // column-`unique` convention — keeps pre-unique-index project files + // byte-stable on a no-op round-trip. + if index.unique { + let _ = writeln!(out, " unique: true"); + } } fn write_table(out: &mut String, table: &TableSchema) { @@ -300,6 +306,7 @@ pub(crate) fn parse_schema(body: &str) -> Result { name: i.name, table: i.table, columns: i.columns, + unique: i.unique, }) .collect(); Ok(SchemaSnapshot { @@ -434,6 +441,11 @@ struct RawIndex { name: String, table: String, columns: Vec, + /// `UNIQUE` index flag (ADR-0035 §4d). Optional on read — project + /// files written before unique indexes existed omit it and default + /// to `false`. + #[serde(default)] + unique: bool, } #[cfg(test)] @@ -479,6 +491,7 @@ mod tests { name: "Orders_CustId_idx".to_string(), table: "Orders".to_string(), columns: vec!["CustId".to_string()], + unique: false, }], } } @@ -556,6 +569,89 @@ mod tests { assert_eq!(parsed, original); } + #[test] + fn unique_index_round_trips_through_yaml() { + // ADR-0035 §4d: a UNIQUE index's uniqueness survives a serialize + // → parse cycle. A plain index emits no `unique` line; a unique + // index emits `unique: true`. + let snap = SchemaSnapshot { + created_at: "2026-05-25T00:00:00Z".to_string(), + tables: vec![TableSchema { + name: "Customers".to_string(), + primary_key: vec!["id".to_string()], + columns: vec![ + ColumnSchema { + name: "id".to_string(), + user_type: Type::Serial, + unique: false, + not_null: false, + default: None, + check: None, + }, + ColumnSchema { + name: "Email".to_string(), + user_type: Type::Text, + unique: false, + not_null: false, + default: None, + check: None, + }, + ], + unique_constraints: Vec::new(), + check_constraints: Vec::new(), + }], + relationships: Vec::new(), + indexes: vec![ + IndexSchema { + name: "Customers_Email_uidx".to_string(), + table: "Customers".to_string(), + columns: vec!["Email".to_string()], + unique: true, + }, + IndexSchema { + name: "Customers_id_idx".to_string(), + table: "Customers".to_string(), + columns: vec!["id".to_string()], + unique: false, + }, + ], + }; + let body = serialize_schema(&snap); + // The unique index emits the flag; the plain one does not. + assert!(body.contains("unique: true"), "yaml:\n{body}"); + assert_eq!( + body.matches("unique: true").count(), + 1, + "only the unique index carries the flag:\n{body}" + ); + let parsed = parse_schema(&body).expect("parse schema"); + assert_eq!(parsed, snap); + } + + #[test] + fn index_without_unique_field_defaults_to_false() { + // Older project files (written before unique indexes) omit the + // `unique` field; the `#[serde(default)]` makes it `false`. + let body = "\ +version: 1 +project: + created_at: 2026-05-25T00:00:00Z +tables: + - name: Customers + primary_key: [id] + columns: + - { name: id, type: serial } +relationships: [] +indexes: + - name: Customers_id_idx + table: Customers + columns: [id] +"; + let parsed = parse_schema(body).expect("parse schema"); + assert_eq!(parsed.indexes.len(), 1); + assert!(!parsed.indexes[0].unique); + } + #[test] fn column_constraints_round_trip_through_yaml() { // NOT NULL / UNIQUE / DEFAULT survive a serialize → diff --git a/src/runtime.rs b/src/runtime.rs index c7aab01..f5d0570 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -29,9 +29,9 @@ use crate::action::Action; use crate::app::App; use crate::cli::Args; use crate::db::{ - AddColumnResult, ChangeColumnTypeResult, CreateOutcome, DataResult, Database, DbError, - DeleteResult, DropColumnResult, DropOutcome, InsertResult, QueryPlan, TableDescription, - UpdateResult, + AddColumnResult, ChangeColumnTypeResult, CreateIndexOutcome, CreateOutcome, DataResult, + Database, DbError, DeleteResult, DropColumnResult, DropIndexOutcome, DropOutcome, InsertResult, + QueryPlan, TableDescription, UpdateResult, }; use crate::dsl::{Command, ColumnSpec}; use crate::dsl::walker::Severity; @@ -1029,11 +1029,18 @@ async fn build_schema_cache(database: &Database) -> crate::completion::SchemaCac // walker falls back to the schemaless value-literal list. for name in cache.tables.clone() { if let Ok(desc) = database.describe_table(name.clone(), None).await { - // Per-table index names for the items panel (S2, - // ADR-0025). Captured before `desc.columns` is + // Per-table indexes for the items panel (S2, ADR-0025). + // Carry uniqueness so the panel can mark a UNIQUE index + // (ADR-0035 §4d). Captured before `desc.columns` is // consumed below. - let index_names: Vec = - desc.indexes.iter().map(|i| i.name.clone()).collect(); + let index_entries: Vec = desc + .indexes + .iter() + .map(|i| crate::completion::IndexEntry { + name: i.name.clone(), + unique: i.unique, + }) + .collect(); let cols: Vec = desc .columns .into_iter() @@ -1055,7 +1062,7 @@ async fn build_schema_cache(database: &Database) -> crate::completion::SchemaCac }) .collect(); cache.table_columns.insert(name.clone(), cols); - cache.table_indexes.insert(name, index_names); + cache.table_indexes.insert(name, index_entries); } } cache @@ -1261,6 +1268,15 @@ fn spawn_dsl_dispatch( Ok(CommandOutcome::SchemaDropSkipped) => AppEvent::DslDropSkipped { command: command.clone(), }, + Ok(CommandOutcome::SchemaDropIndexSkipped) => AppEvent::DslDropIndexSkipped { + command: command.clone(), + }, + Ok(CommandOutcome::SchemaCreateIndexSkipped(name)) => { + AppEvent::DslCreateIndexSkipped { + command: command.clone(), + name, + } + } Ok(CommandOutcome::Query(data)) => AppEvent::DslDataSucceeded { command: command.clone(), data, @@ -1662,6 +1678,15 @@ enum CommandOutcome { /// (ADR-0035 §4, 4c). Carries no structure (there is none); the App /// renders the "doesn't exist — skipped" note from the command. SchemaDropSkipped, + /// A SQL `DROP INDEX IF EXISTS` that matched no index — a no-op + /// (ADR-0035 §4d). The App renders the "doesn't exist — skipped" + /// note from the command's index name. + SchemaDropIndexSkipped, + /// A SQL `CREATE INDEX IF NOT EXISTS` that matched an existing index + /// name — a no-op (ADR-0035 §4d). Carries the resolved index name + /// (the auto-name is unknown to the command) for the "already exists + /// — skipped" note. + SchemaCreateIndexSkipped(String), Query(DataResult), QueryPlan(QueryPlan), Insert(InsertResult), @@ -2048,6 +2073,28 @@ async fn execute_command_typed( .drop_index(selector, src) .await .map(|d| CommandOutcome::Schema(Some(d))), + Command::SqlDropIndex { name, if_exists } => database + .sql_drop_index(name, if_exists, src) + .await + .map(|outcome| match outcome { + // Auto-show the now de-indexed table (ADR-0014), unlike + // SQL DROP TABLE whose table is gone. + DropIndexOutcome::Dropped(d) => CommandOutcome::Schema(Some(d)), + DropIndexOutcome::Skipped => CommandOutcome::SchemaDropIndexSkipped, + }), + Command::SqlCreateIndex { + name, + table, + columns, + unique, + if_not_exists, + } => database + .sql_create_index(name, table, columns, unique, if_not_exists, src) + .await + .map(|outcome| match outcome { + CreateIndexOutcome::Created(d) => CommandOutcome::Schema(Some(d)), + CreateIndexOutcome::Skipped(n) => CommandOutcome::SchemaCreateIndexSkipped(n), + }), Command::AddConstraint { table, column, diff --git a/src/ui.rs b/src/ui.rs index 593da91..524c33c 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -514,8 +514,11 @@ fn render_items_panel(app: &App, theme: &Theme, frame: &mut Frame<'_>, area: Rec lines.push(Line::from(Span::styled(name.as_str(), style))); if let Some(indexes) = app.schema_cache.table_indexes.get(name) { for index in indexes { + // Mark a UNIQUE index so the panel distinguishes it from + // a performance-only index (ADR-0035 §4d). + let unique = if index.unique { " [unique]" } else { "" }; lines.push(Line::from(Span::styled( - format!(" {index}"), + format!(" {}{unique}", index.name), Style::default().fg(theme.muted), ))); } @@ -1280,17 +1283,23 @@ mod tests { #[test] fn items_panel_nests_indexes_under_their_table() { // S2 (ADR-0025): the items panel renders each table - // with its index names indented beneath it. + // with its index names indented beneath it. A UNIQUE index is + // marked `[unique]` (ADR-0035 §4d). + use crate::completion::IndexEntry; let mut app = App::new(); app.tables = vec!["Customers".to_string(), "Orders".to_string()]; app.schema_cache.table_indexes.insert( "Customers".to_string(), - vec!["idx_email".to_string()], + vec![ + IndexEntry { name: "idx_email".to_string(), unique: false }, + IndexEntry { name: "uidx_login".to_string(), unique: true }, + ], ); let theme = Theme::dark(); let out = render_to_string(&mut app, &theme, 80, 24); assert!(out.contains("Customers"), "table listed:\n{out}"); assert!(out.contains("Orders"), "table listed:\n{out}"); assert!(out.contains("idx_email"), "index nested in panel:\n{out}"); + assert!(out.contains("uidx_login [unique]"), "unique index marked:\n{out}"); } } diff --git a/tests/sql_create_index.rs b/tests/sql_create_index.rs new file mode 100644 index 0000000..1e24518 --- /dev/null +++ b/tests/sql_create_index.rs @@ -0,0 +1,360 @@ +//! Sub-phase 4d integration tests for advanced-mode SQL +//! `CREATE [UNIQUE] INDEX [IF NOT EXISTS]` (ADR-0035 §4d). +//! +//! `SqlCreateIndex` executes through the same `do_add_index` machinery +//! as the simple `add index`, plus the `unique` flag and the +//! `IF NOT EXISTS` no-op-with-note (`CreateIndexOutcome::Skipped`). +//! Parsing (text → `Command::SqlCreateIndex`) is covered by the +//! `sql_create_index_tests` in `src/dsl/grammar/ddl.rs`. + +use rdbms_playground::db::{CreateIndexOutcome, Database}; +use rdbms_playground::dsl::{ColumnSpec, Type, Value}; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open(undo: bool) -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let persistence = Persistence::new(project.path().to_path_buf()); + let db = Database::open_with_persistence_and_undo(project.db_path(), persistence, undo) + .expect("open db with persistence"); + (project, db, dir) +} + +/// Create `T (id int primary key, email text)`. +fn make_t(db: &Database, r: &tokio::runtime::Runtime) { + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ColumnSpec::new("id", Type::Int), ColumnSpec::new("email", Type::Text)], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table T (id int primary key, email text)".to_string()), + )) + .expect("create T"); +} + +fn insert_row(db: &Database, r: &tokio::runtime::Runtime, id: i64, email: &str) -> bool { + r.block_on(db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "email".to_string()]), + vec![Value::Number(id.to_string()), Value::Text(email.to_string())], + Some(format!("insert into T (id, email) values ({id}, '{email}')")), + )) + .is_ok() +} + +fn index(db: &Database, r: &tokio::runtime::Runtime, name: &str) -> Option<(Vec, bool)> { + r.block_on(db.describe_table("T".to_string(), None)) + .expect("describe") + .indexes + .into_iter() + .find(|i| i.name == name) + .map(|i| (i.columns, i.unique)) +} + +#[test] +fn create_plain_index() { + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + let out = r + .block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix on T (email)".to_string()), + )) + .expect("create index"); + assert!(matches!(out, CreateIndexOutcome::Created(_))); + assert_eq!(index(&db, &r, "ix"), Some((vec!["email".to_string()], false))); +} + +#[test] +fn create_unique_index_round_trips_and_survives_rebuild_and_enforces() { + let (p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ux".to_string()), + "T".to_string(), + vec!["email".to_string()], + true, + false, + Some("create unique index ux on T (email)".to_string()), + )) + .expect("create unique index"); + // Reported as unique. + assert_eq!(index(&db, &r, "ux"), Some((vec!["email".to_string()], true))); + // Persisted to project.yaml as a unique index. + let yaml = std::fs::read_to_string(p.path().join("project.yaml")).expect("read project.yaml"); + assert!(yaml.contains("unique: true"), "project.yaml:\n{yaml}"); + + // Uniqueness is enforced by the engine. + assert!(insert_row(&db, &r, 1, "a@x")); + assert!(!insert_row(&db, &r, 2, "a@x"), "duplicate email refused by the unique index"); + + // Rebuild from the text artifacts: the index comes back UNIQUE + // (the rebuild re-emits CREATE UNIQUE INDEX), not demoted to plain. + r.block_on(db.rebuild_from_text(p.path().to_path_buf(), Some("rebuild".to_string()))) + .expect("rebuild"); + assert_eq!( + index(&db, &r, "ux"), + Some((vec!["email".to_string()], true)), + "the unique flag survived rebuild" + ); + // Still enforced after rebuild. + assert!(!insert_row(&db, &r, 3, "a@x"), "uniqueness enforced after rebuild too"); +} + +#[test] +fn create_unique_index_on_duplicate_data_is_refused() { + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + assert!(insert_row(&db, &r, 1, "dup@x")); + assert!(insert_row(&db, &r, 2, "dup@x")); + // A unique index can't be created over columns that already hold + // duplicate values — the engine refuses at creation. + let res = r.block_on(db.sql_create_index( + Some("ux".to_string()), + "T".to_string(), + vec!["email".to_string()], + true, + false, + Some("create unique index ux on T (email)".to_string()), + )); + assert!(res.is_err(), "unique index over duplicate data is refused"); +} + +#[test] +fn if_not_exists_on_an_existing_name_is_a_noop_and_journalled() { + let (p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix on T (email)".to_string()), + )) + .expect("first create"); + // A second IF NOT EXISTS create of the same name is a no-op. + let line = "create index if not exists ix on T (email)"; + let out = r + .block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + true, + Some(line.to_string()), + )) + .expect("IF NOT EXISTS on an existing index name succeeds as a no-op"); + match out { + CreateIndexOutcome::Skipped(name) => assert_eq!(name, "ix"), + CreateIndexOutcome::Created(_) => panic!("expected Skipped, got Created"), + } + let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); + assert!(log.contains(line), "the skipped create should be journalled; log:\n{log}"); +} + +#[test] +fn unnamed_if_not_exists_skips_when_the_auto_named_index_exists() { + // The unnamed form resolves the auto-name `__idx`; the skip + // pre-check must resolve the SAME name (shared `resolve_index_name`). + // First an unnamed create (auto-named T_email_idx), then an unnamed + // IF NOT EXISTS create of the same columns → skip on the auto-name. + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + None, + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index on T (email)".to_string()), + )) + .expect("unnamed create"); + let out = r + .block_on(db.sql_create_index( + None, + "T".to_string(), + vec!["email".to_string()], + false, + true, + Some("create index if not exists on T (email)".to_string()), + )) + .expect("unnamed IF NOT EXISTS over the auto-named index is a no-op"); + match out { + CreateIndexOutcome::Skipped(name) => assert_eq!(name, "T_email_idx"), + CreateIndexOutcome::Created(_) => panic!("expected Skipped on the auto-name, got Created"), + } +} + +#[test] +fn if_not_exists_short_circuits_only_a_name_collision() { + // `IF NOT EXISTS` skips only when the *name* already exists. A + // *different*-named create over already-indexed columns is not a + // name collision, so it still hits the ADR-0025 redundant-set guard + // (the playground's pedagogical refusal, not raw-SQL semantics). + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix on T (email)".to_string()), + )) + .expect("first create"); + // Same columns, a *new* name, with IF NOT EXISTS → not a name + // collision, so the redundant-set refusal still fires. + let res = r.block_on(db.sql_create_index( + Some("ix2".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + true, + Some("create index if not exists ix2 on T (email)".to_string()), + )); + assert!( + res.is_err(), + "IF NOT EXISTS does not bypass the redundant-column-set guard for a new name" + ); +} + +#[test] +fn plain_duplicate_name_errors() { + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix on T (email)".to_string()), + )) + .expect("first create"); + // Same name again, *without* IF NOT EXISTS → error. + let res = r.block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["id".to_string()], + false, + false, + Some("create index ix on T (id)".to_string()), + )); + assert!(res.is_err(), "duplicate index name without IF NOT EXISTS errors"); +} + +#[test] +fn plain_and_unique_over_the_same_columns_are_not_duplicates() { + // The redundant-set guard keys on (columns, unique): a plain and a + // unique index over the same columns are distinct (different + // semantics). They need distinct explicit names (the auto-name would + // collide). + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ix_plain".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix_plain on T (email)".to_string()), + )) + .expect("plain"); + r.block_on(db.sql_create_index( + Some("ix_unique".to_string()), + "T".to_string(), + vec!["email".to_string()], + true, + false, + Some("create unique index ix_unique on T (email)".to_string()), + )) + .expect("unique over the same columns is allowed (distinct kind)"); + assert_eq!(index(&db, &r, "ix_plain").map(|(_, u)| u), Some(false)); + assert_eq!(index(&db, &r, "ix_unique").map(|(_, u)| u), Some(true)); + + // But an *exact* duplicate (same columns AND same uniqueness) is + // still refused. + let res = r.block_on(db.sql_create_index( + Some("ix_plain2".to_string()), + "T".to_string(), + vec!["email".to_string()], + false, + false, + Some("create index ix_plain2 on T (email)".to_string()), + )); + assert!(res.is_err(), "a second plain index over the same columns is redundant"); +} + +#[test] +fn create_index_on_an_internal_table_is_refused_on_both_surfaces() { + // Internal `__rdbms_*` tables are hidden from the user; indexing one + // is refused as "no such table" — via the SQL surface and the simple + // `add index` surface alike (the guard lives in the shared + // `do_add_index`, ADR-0035 §4d). + let (_p, db, _d) = open(false); + let r = rt(); + make_t(&db, &r); + // SQL CREATE INDEX on an internal table → error. + let sql = r.block_on(db.sql_create_index( + Some("bad".to_string()), + "__rdbms_playground_columns".to_string(), + vec!["table_name".to_string()], + false, + false, + Some("create index bad on __rdbms_playground_columns (table_name)".to_string()), + )); + assert!(sql.is_err(), "SQL CREATE INDEX on an internal table is refused"); + // Simple `add index` on an internal table → error (same guard). + let dsl = r.block_on(db.add_index( + Some("bad2".to_string()), + "__rdbms_playground_columns".to_string(), + vec!["table_name".to_string()], + Some("add index as bad2 on __rdbms_playground_columns (table_name)".to_string()), + )); + assert!(dsl.is_err(), "simple add index on an internal table is refused"); +} + +#[test] +fn create_index_is_one_undo_step() { + let (_p, db, _d) = open(true); // undo enabled + let r = rt(); + make_t(&db, &r); + r.block_on(db.sql_create_index( + Some("ix".to_string()), + "T".to_string(), + vec!["email".to_string()], + true, + false, + Some("create unique index ix on T (email)".to_string()), + )) + .expect("create index"); + assert!(index(&db, &r, "ix").is_some()); + // One undo removes the index. + assert!(r.block_on(db.undo()).expect("undo").is_some(), "the create was one undo step"); + assert!(index(&db, &r, "ix").is_none(), "undo removed the index"); +} diff --git a/tests/sql_drop_index.rs b/tests/sql_drop_index.rs new file mode 100644 index 0000000..fd4dd3a --- /dev/null +++ b/tests/sql_drop_index.rs @@ -0,0 +1,123 @@ +//! Sub-phase 4d integration tests for advanced-mode SQL +//! `DROP INDEX [IF EXISTS]` (ADR-0035 §4d). +//! +//! `SqlDropIndex` executes through the same `do_drop_index` machinery as +//! the simple `drop index `; the only new behaviour is `IF EXISTS` +//! as a no-op-with-note (`DropIndexOutcome::Skipped`). These drive the +//! worker directly; parsing (text → `Command::SqlDropIndex`) is covered +//! by the `sql_drop_index_tests` in `src/dsl/grammar/ddl.rs`. + +use rdbms_playground::db::{Database, DropIndexOutcome}; +use rdbms_playground::dsl::{ColumnSpec, Type}; +use rdbms_playground::persistence::Persistence; +use rdbms_playground::project; + +fn rt() -> tokio::runtime::Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("tokio rt") +} + +fn open(undo: bool) -> (project::Project, Database, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create tempdir"); + let project = + project::open_or_create(None, Some(dir.path())).expect("open or create project"); + let persistence = Persistence::new(project.path().to_path_buf()); + let db = Database::open_with_persistence_and_undo(project.db_path(), persistence, undo) + .expect("open db with persistence"); + (project, db, dir) +} + +/// Create `T (id int primary key, email text)` and an index on `email`. +fn make_t_with_index(db: &Database, r: &tokio::runtime::Runtime) -> String { + r.block_on(db.sql_create_table( + "T".to_string(), + vec![ColumnSpec::new("id", Type::Int), ColumnSpec::new("email", Type::Text)], + vec!["id".to_string()], + vec![], + vec![], + vec![], + false, + Some("create table T (id int primary key, email text)".to_string()), + )) + .expect("create T"); + let desc = r + .block_on(db.add_index( + Some("T_email_idx".to_string()), + "T".to_string(), + vec!["email".to_string()], + Some("add index as T_email_idx on T (email)".to_string()), + )) + .expect("add index"); + assert_eq!(desc.indexes.len(), 1, "index created"); + "T_email_idx".to_string() +} + +fn index_names(db: &Database, r: &tokio::runtime::Runtime) -> Vec { + r.block_on(db.describe_table("T".to_string(), None)) + .expect("describe") + .indexes + .into_iter() + .map(|i| i.name) + .collect() +} + +#[test] +fn drop_index_removes_an_existing_index_and_shows_the_table() { + let (_p, db, _d) = open(false); + let r = rt(); + let name = make_t_with_index(&db, &r); + let out = r + .block_on(db.sql_drop_index(name, false, Some("drop index T_email_idx".to_string()))) + .expect("drop index"); + // Dropped carries the de-indexed table's structure (auto-show). + match out { + DropIndexOutcome::Dropped(desc) => { + assert_eq!(desc.name, "T"); + assert!(desc.indexes.is_empty(), "the index is gone from the structure"); + } + DropIndexOutcome::Skipped => panic!("expected Dropped, got Skipped"), + } + assert!(index_names(&db, &r).is_empty()); +} + +#[test] +fn if_exists_on_an_absent_index_is_a_noop_and_journalled() { + let (p, db, _d) = open(false); + let r = rt(); + let line = "drop index if exists ghost_idx"; + let out = r + .block_on(db.sql_drop_index("ghost_idx".to_string(), true, Some(line.to_string()))) + .expect("IF EXISTS on an absent index succeeds as a no-op"); + assert!(matches!(out, DropIndexOutcome::Skipped)); + // The no-op is still journalled (ADR-0034), like the create-skip. + let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); + assert!(log.contains(line), "the skipped drop should be journalled; log:\n{log}"); +} + +#[test] +fn plain_drop_of_an_absent_index_errors() { + let (_p, db, _d) = open(false); + let r = rt(); + let res = r.block_on(db.sql_drop_index( + "ghost_idx".to_string(), + false, + Some("drop index ghost_idx".to_string()), + )); + assert!(res.is_err(), "plain DROP INDEX on an absent index errors (no IF EXISTS)"); +} + +#[test] +fn drop_index_is_one_undo_step_and_restores_the_index() { + let (_p, db, _d) = open(true); // undo enabled + let r = rt(); + let name = make_t_with_index(&db, &r); + r.block_on(db.sql_drop_index(name.clone(), false, Some("drop index T_email_idx".to_string()))) + .expect("drop index"); + assert!(index_names(&db, &r).is_empty()); + + // One undo brings the index back. + assert!(r.block_on(db.undo()).expect("undo").is_some(), "the drop was one undo step"); + assert_eq!(index_names(&db, &r), vec![name], "undo restored the index"); +} diff --git a/tests/typing_surface/mod.rs b/tests/typing_surface/mod.rs index a4d72cd..9bd989d 100644 --- a/tests/typing_surface/mod.rs +++ b/tests/typing_surface/mod.rs @@ -227,6 +227,8 @@ fn command_kind_label(cmd: &rdbms_playground::dsl::Command) -> String { DropRelationship { .. } => "DropRelationship".into(), AddIndex { .. } => "AddIndex".into(), DropIndex { .. } => "DropIndex".into(), + SqlDropIndex { .. } => "SqlDropIndex".into(), + SqlCreateIndex { .. } => "SqlCreateIndex".into(), AddConstraint { .. } => "AddConstraint".into(), DropConstraint { .. } => "DropConstraint".into(), ShowTable { .. } => "ShowTable".into(),