grammar+db: 3f — SQL DELETE + cascade summary (ADR-0033 §1/§7)
New src/dsl/grammar/sql_delete.rs (FROM <table> [WHERE] [;]), Command::SqlDelete, Request::RunSqlDelete, do_sql_delete worker. do_sql_delete mirrors the DSL do_delete: detect FK cascade by before/after child row-count diffing, re-persist target + every cascade-affected child, history-on-success inside the tx. Reuses CommandOutcome::Delete -> handle_dsl_delete_success, so the per-relationship cascade summary formatter is shared, not duplicated. ADR-0033 Amendment 2: supersedes §7's WHERE-injected pre-count. Its premise (DSL handler builds pre-counts from the typed Expr) was wrong — do_delete uses count-diff. The pre-count would also have broken the §2 parity promise by reporting SET NULL the DSL path doesn't. Count- diff gives exact parity, no WHERE-byte extraction, and withdraws R2. SET NULL reporting deferred for both paths (user-confirmed). Tests: +6 grammar unit, +12 integration (cascade parity with DSL, both R2 subquery cases, before-execute order, no-WHERE, FK-rejection rollback, childless-parent, two-child cascade). 1542 pass / 0 fail / 1 ignored. Clippy clean. Dev sql_delete entry word removed in 3j.
This commit is contained in:
claude@clouddev1
@@ -1176,6 +1176,100 @@ Concretely:
|
||||
shared-entry problem (`create`, …): tag the SQL DDL nodes
|
||||
`Advanced`; the dispatcher handles the rest.
|
||||
|
||||
## Amendment 2 — Cascade summary on SQL DELETE: count-diff parity supersedes WHERE-injected pre-count (2026-05-22)
|
||||
|
||||
This amendment **supersedes §7's "Predicate extraction" subsection
|
||||
and the pre-count mechanism in §7 steps 1–3**, and **withdraws Open
|
||||
Implementation Risk R2**. It was written during sub-phase 3f, after
|
||||
tracing §7's prescribed mechanism against the actual DSL `do_delete`
|
||||
handler, and is recorded with explicit user approval before any 3f
|
||||
code landed.
|
||||
|
||||
### The finding — §7's premise about the DSL handler is incorrect
|
||||
|
||||
§7 prescribes a per-child **pre-execution pre-count** of the form
|
||||
`SELECT count(*) FROM <child> WHERE <fk_col> IN (SELECT <pk_col> FROM
|
||||
<target_table> WHERE <user_where_predicate>)`, with the
|
||||
`<user_where_predicate>` re-injected from the SQL DELETE's
|
||||
WHERE-clause source bytes. Its stated justification:
|
||||
|
||||
> "The DSL handler reuses the typed `Expr` AST to construct the
|
||||
> pre-count subqueries."
|
||||
|
||||
Tracing `do_delete` in `src/db.rs` shows this premise is factually
|
||||
wrong. The DSL handler does **not** construct pre-count subqueries
|
||||
from the typed `Expr`. It detects cascade effects by **before/after
|
||||
row-count diffing**: read the inbound relationships, count each child
|
||||
table's rows *before* the DELETE, execute the DELETE inside a
|
||||
transaction, count each child *after*, and report the positive
|
||||
difference as a `CascadeEffect`. No pre-count query and no
|
||||
`Expr`-derived subquery exist anywhere in the path.
|
||||
|
||||
### Two consequences of the correct mechanism
|
||||
|
||||
1. **Count-diff reports `ON DELETE CASCADE` row removals only.** `ON
|
||||
DELETE SET NULL` does not change a child's row count, so the DSL
|
||||
path does not report it (its own code comment notes this).
|
||||
Reporting SET NULL would require value-level diffing.
|
||||
|
||||
2. **§7's pre-count is in tension with the parity promise.** §2
|
||||
promises "a DSL-style … DELETE in Advanced mode produces identical
|
||||
observable behaviour whether it routes through the SQL or DSL
|
||||
path", and the plan's 3f exit gate requires the SQL cascade summary
|
||||
to *match* the DSL `do_delete` output. A pre-count that counted
|
||||
`ON DELETE CASCADE` *and* `SET NULL` (as §7 step 1 says) would
|
||||
report **more** than the DSL path — breaking that parity. To match
|
||||
the DSL path it would have to drop SET NULL anyway, leaving
|
||||
WHERE-byte extraction and the R2 N+1-subquery pathology as pure
|
||||
cost with no benefit over count-diff.
|
||||
|
||||
### The replacement — count-diff parity
|
||||
|
||||
`do_sql_delete` mirrors `do_delete` exactly: read inbound
|
||||
relationships, snapshot child row counts, run the **verbatim**
|
||||
validated DELETE SQL inside a transaction via
|
||||
`execute_with_fk_enrichment`, diff the child counts, build the same
|
||||
`Vec<CascadeEffect>`, and re-persist the target plus every
|
||||
cascade-affected child before commit. Because both handlers return
|
||||
the same `DeleteResult` and both route through
|
||||
`CommandOutcome::Delete` → `handle_dsl_delete_success`, the
|
||||
per-relationship summary formatter is **already shared at the render
|
||||
layer** — no formatter refactor and no duplicate path.
|
||||
|
||||
This dominates the §7 mechanism on every axis the 3f exit gate
|
||||
measures:
|
||||
|
||||
- **Exact parity** with the DSL path — identical detection mechanism,
|
||||
not merely a matching formatter.
|
||||
- **No WHERE-clause byte extraction.** The verbatim DELETE (including
|
||||
any subquery in its WHERE) executes once; the engine cascades; the
|
||||
diff observes the result. The R2 invariant case (`DELETE FROM a
|
||||
WHERE id IN (SELECT … )`) is correct by construction and carries no
|
||||
N+1 cost. **R2 is withdrawn.**
|
||||
- **Shared render** with no duplicate formatter, satisfying §7's
|
||||
shared-formatter promise structurally.
|
||||
|
||||
### SET NULL reporting — deferred for both paths (user-confirmed)
|
||||
|
||||
3f keeps strict DSL parity: `ON DELETE CASCADE` row removals are
|
||||
reported; `SET NULL` is not, on either path. Adding SET NULL
|
||||
reporting (value-level diffing) is a tracked future enhancement to be
|
||||
applied to **both** `do_delete` and `do_sql_delete` together so
|
||||
parity is preserved. The user confirmed this deferral.
|
||||
|
||||
### Consequences of the amendment
|
||||
|
||||
- §7's "Predicate extraction" subsection and pre-count steps 1–3 are
|
||||
superseded by count-diff; the per-relationship summary and
|
||||
shared-formatter outcomes are unchanged.
|
||||
- **R2 (cascade-summary predicate extraction) is withdrawn** — the
|
||||
mechanism it budgeted for is no longer used.
|
||||
- `Command::SqlDelete` carries `{ sql, target_table }` only — no
|
||||
WHERE-clause field is needed (the worker never inspects the
|
||||
predicate).
|
||||
- The handoff-31 §4 "WHERE-byte-extraction is tractable for DELETE"
|
||||
heads-up is moot — no extraction happens.
|
||||
|
||||
## See also
|
||||
|
||||
- ADR-0005 — the ten-type vocabulary INSERT works with.
|
||||
|
||||
+1
-1
@@ -38,4 +38,4 @@ This directory contains the project's ADRs, recorded per
|
||||
- [ADR-0030 — Advanced mode: the standard-SQL surface](0030-advanced-mode-sql-surface.md) — **Accepted**, SQL added as grammar *within the unified grammar tree* (ADR-0024), not a separate batch parser — so SQL gets the same completion / highlighting / hints / parse-errors as the DSL; mode gates the SQL forms; DDL routes through the typed `Command` executor (metadata + type vocabulary preserved), DML and `SELECT` execute as validated SQL; engine-neutral posture, the DSL→SQL teaching echo; supersedes ADR-0001's `sqlparser-rs` reservation; phased plan (`Q1` / `Q2` / `Q4`)
|
||||
- [ADR-0031 — The SQL expression grammar](0031-sql-expression-grammar.md) — **Accepted**, the stratified SQL expression grammar fragment commissioned by ADR-0030 §3: a single precedence ladder (`OR`/`AND`/`NOT`, the comparison/`LIKE`/`IN`/`BETWEEN`/`IS NULL` predicate set, arithmetic incl. `||`, function calls, `CASE`) — the superset of ADR-0026's DSL `WHERE` grammar, authored as a parallel fragment so simple mode is untouched; pure validation, builds **no** AST (consumers run/store SQL as text per ADR-0030 §4/§6); reuses ADR-0026's `Subgrammar` recursion + depth cap unchanged; subquery expressions and qualified column refs deferred to ADR-0030 Phase 2
|
||||
- [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) — **Proposed**, 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), with the WHERE-clause source bytes re-injected into per-child pre-count subqueries; 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
|
||||
- [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Proposed**, 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)
|
||||
|
||||
Reference in New Issue
Block a user