From d0c8a793567c874b132df65b38acdc3de02070d0 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 18 Jul 2026 13:16:30 +0100 Subject: [PATCH] docs: add ENGINE_REFACTORING.md, an R-numbered structural debt log Companion to SQL_PARITY.md. The two tracks answer different questions: P-numbers ask "do we return the right answer?", R-numbers ask "can we keep changing the engine safely?". The engine grew organically, which is why it does as much as it does. The cost is that some structure was never designed, only accreted, and that bill arrives when we try to go further -- richer functions, deeper nesting, correlated scoping. This log exists to make that debt legible so it can be paid incrementally during feature work, and to distinguish "awkward" from "blocking". It is explicitly not a rewrite proposal: principle 1 is that every entry reduces to independently shippable steps. Seeded with seven findings established while scoping parity P3, each with evidence rather than opinion: - R1 two FROM representations (partial; migration deferred as larger than P3 itself, and blocked on real AST work in TableSource) - R2 446 hand-rolled SqlExpression matches across 40 files (partial; walk.rs landed, transformer migration outstanding) - R3 catch-all arms swallow new variants -- only 5 match sites in the whole tree are compiler-guarded, verified by probe - R4 transformer fixtures use ASTs the parser never produces, which is precisely why R1's desync survived a green suite - R5 dead code accumulates undetected - R6 CorrelatedSubqueryAnalyzer unwired, its tests hollow - R7 subqueries are substituted not evaluated -- the structural root cause under parity P3 Adds a sequencing diagram making the load-bearing dependency explicit: the advanced work is gated on R2/R3, because every new expression variant currently costs a 40-file hand audit with silent failure as the default outcome. Cross-links P3 to R7/R2/R6, and surfaces both books of work in CLAUDE.md so future sessions find them before touching the engine. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 8 ++ docs/ENGINE_REFACTORING.md | 236 +++++++++++++++++++++++++++++++++++++ docs/SQL_PARITY.md | 9 ++ 3 files changed, 253 insertions(+) create mode 100644 docs/ENGINE_REFACTORING.md diff --git a/CLAUDE.md b/CLAUDE.md index 44533ce2..9c41bd4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -380,6 +380,14 @@ The script parser is basic - it chunks on `GO` statements, so proper formatting ## 🔗 Quick Links +### Books of Work (read before starting engine changes) +- **`docs/SQL_PARITY.md`** — P-numbered log of divergences vs DuckDB (*do we + return the right answer?*). Backed by `tests/comparison/` and a CI gate. +- **`docs/ENGINE_REFACTORING.md`** — R-numbered log of structural debt (*can we + keep changing the engine safely?*). Records why groundwork is sequenced the + way it is, and which refactors are deliberately deferred. + +### Code - Function Registry: `src/sql/functions/mod.rs` - Parser: `src/sql/recursive_parser.rs` - Expression Evaluator: `src/data/arithmetic_evaluator.rs` diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md new file mode 100644 index 00000000..1f8d6dc9 --- /dev/null +++ b/docs/ENGINE_REFACTORING.md @@ -0,0 +1,236 @@ +# Engine Refactoring — Book of Work + +The durable decision log for **structural** debt in the query engine, the +counterpart to [`SQL_PARITY.md`](SQL_PARITY.md). + +The two tracks answer different questions: + +| | Question | Driven by | +|---|---|---| +| `SQL_PARITY.md` (P-numbers) | *Do we return the right answer?* | Differential testing vs DuckDB | +| **This file (R-numbers)** | *Can we keep changing the engine safely?* | Findings from doing the work | + +A parity gap is a wrong result. An **R-finding** is a place where the engine's +shape makes the *next* change disproportionately expensive or risky — duplicated +branch logic, two representations of one fact, a test fixture that can't catch +the bug it's meant to guard. + +## Why this file exists + +The engine grew organically and on demand, which is why it does as much as it +does. The cost is that some structure was never designed, only accreted — and +that cost is now being paid at the point where we want to go further (richer +function support, deeper nesting, correlated scoping). + +The purpose of this log is explicitly **not** to justify a rewrite. It is to +make the debt legible so it can be paid **incrementally, in the course of +feature work**, and so we can tell the difference between "this is awkward" and +"this is blocking". + +## Principles + +1. **No wholesale rewrites.** No multi-month refactor projects. Every entry here + must be reducible to steps that ship independently. +2. **Groundwork is justified by the feature it unblocks**, and the link is + recorded. Refactoring for its own sake doesn't earn a slot. +3. **Each step must be independently verifiable** — green tests, parity contract + intact, and where possible a test that fails without the change. +4. **Prefer compiler-enforced invariants over conventions.** A rule the compiler + checks survives; a rule in a comment does not. +5. **Additive first.** Introduce the better mechanism, migrate call sites in + separate commits, delete the old path last. + +## Status legend + +| Status | Meaning | +|---|---| +| 🔴 OPEN | confirmed weakness, not yet addressed | +| 🟡 IN PROGRESS | mechanism landed, migration outstanding | +| 🟢 DONE | resolved | +| ⚪ ACCEPTED | known, deliberately not changing — rationale recorded | + +--- + +## Open findings + +### R1 — Two representations of the FROM clause +- **Status:** 🟡 IN PROGRESS — sync helpers landed (PR #30); full migration deferred +- **Where:** `ast.rs` `SelectStatement.from_source` vs the deprecated + `from_table` / `from_subquery` / `from_function` / `from_alias` +- **Observed:** The parser populates **both**; the executor + (`query_engine.rs:1217`) reads `from_source` first and falls back to the + legacy fields. Four transformers rewrote only one of the two, leaving the + stale copy as the one that actually executed. `from_source` is a + *second-class projection* derived from the legacy fields + (`recursive_parser.rs:1119-1136`), not the source of truth. +- **Impact:** ~56 live use sites across 19 files. Any code reasoning about the + FROM clause has to know which representation is authoritative in that context + — and the answer differs by call site. +- **Done so far:** Added `impl SelectStatement` (there was none) with + `set_from_table` / `set_from_subquery` / `map_from_subquery`, and routed the + four desync sites through them, so the representations can only move together. +- **Deferred, deliberately:** The actual migration off the legacy fields is + **blocked on AST work, not mechanical churn**: + - `TableSource` has **no table-function variant** — `recursive_parser.rs:1132` + explicitly sets `from_source = None` when `from_function` is set, so a + `from_source`-only reader silently loses `READ_CSV()`, `RANGE()`, etc. + - `TableSource::Table(String)` **cannot carry an alias**; `FROM trades t` + stores `t` only in `from_alias`. + + Fixing both breaks `TableSource`, which `JoinClause.table` also uses — so + joins get dragged in. Assessed as **larger than P3 itself**. Revisit as its + own project. +- **Related:** [P3](SQL_PARITY.md) needs `from_source` to be authoritative for + the scope work. + +### R2 — Branch logic over `SqlExpression` is copy-pasted everywhere +- **Status:** 🟡 IN PROGRESS — helpers landed (PR #31); transformer migration outstanding +- **Where:** `src/query_plan/*.rs`, `src/data/*.rs`, `src/analysis/*.rs` +- **Observed:** No traversal abstraction existed. **446 + `SqlExpression::` patterns across 40 files**, every consumer + hand-rolling its own match over 24 variants. `ilike_to_like_transformer` and + `cte_hoister` have `BinaryOp` / `FunctionCall` / `CaseExpression` arms that are + byte-for-byte identical apart from the method name — each wrapping *one* real + rule in ~50 lines of boilerplate. +- **Impact:** This is the finding that makes everything else expensive. Adding a + variant (e.g. `Exists` for P3) means auditing every copy by hand. It is also + the direct blocker on richer expression features generally — each new + expression form multiplies across ~40 files. +- **Done so far:** `src/sql/parser/walk.rs` — `map_children` (owned rewrite) and + `visit_children` / `visit_all` (borrowing collector), exhaustive, no catch-all. + Two design rules worth preserving: + - **Direct children only, callers drive recursion** — this is what collapses a + transformer to its one real rule plus a delegate. + - **Scope boundary = walker boundary** — subquery statements are not descended + into. Auto-descending would break the alias expanders, which get outer-alias + scoping right today only *by omission*. +- **Next:** Migrate the ~10 `query_plan` transformers, one commit each. Treat + `having_alias_transformer` as a behaviour **fix**, not a refactor (see R3). + Note `WindowSpec::order_by` is now descended into, so each migration needs a + per-transformer behaviour check rather than a blanket "pure refactor" claim. + +### R3 — Catch-all match arms silently swallow new variants +- **Status:** 🔴 OPEN +- **Observed:** Almost every hand-rolled walker ends in `_ => {}` or + `other => other`. Verified by temporarily adding an `Exists` variant to + `SqlExpression`: only **five** match sites in the entire codebase failed to + compile — + + ``` + src/sql/parser/formatter.rs:139 + src/sql/parser/formatter.rs:668 + src/sql/recursive_parser.rs:569 + src/sql/parser/walk.rs:63 (added by R2) + src/sql/parser/walk.rs:214 (added by R2) + ``` + + Every other consumer compiled clean and would have silently ignored it. +- **Impact:** A new expression variant is *silently* dropped by the formatter, + the lifters, and the aggregate detector. The failure mode is a wrong answer or + a no-op, not a build error. +- **Confirmed live bugs from this pattern** (both pre-existing, neither yet fixed): + - `expression_lifter::extract_column_references` never handles `MethodCall`, + `ChainedMethodCall`, `Unnest`, `InSubquery`, or the tuple subquery variants. + - `having_alias_transformer::collect_aggregates_in_having` handles only + `FunctionCall`, `BinaryOp`, `Not` — **an aggregate inside a `CASE` in a + `HAVING` clause is never found.** +- **Decision:** Fix by attrition through the R2 migration; each transformer that + moves onto `walk` loses its catch-all. Worth a follow-up to confirm the two + bugs above with failing tests before their transformers migrate, so the fix is + demonstrated rather than assumed. + +### R4 — Transformer test fixtures use ASTs the parser never produces +- **Status:** 🔴 OPEN +- **Observed:** Hand-built `SelectStatement` fixtures in `cte_hoister`, + `into_clause_remover` and others set `from_source: None` while setting + `from_table: Some(...)`. The parser always populates both. +- **Impact:** **This is why R1's desync survived.** The transformers were only + ever exercised on inputs that couldn't exhibit the bug. A green suite meant + nothing here. +- **Decision:** Prefer parsing real SQL in transformer tests over hand-building + ASTs. Where a fixture is genuinely needed, derive it from a parse rather than + writing the struct literal. The two regression tests added in PR #30 follow + this pattern. +- **Note:** A `SelectStatement::from_sql(&str)` test helper would make the right + thing the easy thing; the struct literals are ~25 lines each, which is most of + why people reach for `Default`-ish shortcuts. + +### R5 — Dead code accumulates undetected +- **Status:** 🔴 OPEN +- **Observed:** `column_dependency_lifter.rs` sat in the tree fully dead — + absent from `query_plan/mod.rs`, referenced nowhere, and no longer compiling + against the current AST (its `SelectStatement` literal was missing six + fields). Deleted in PR #30. `cte_hoister::hoist_from_condition` is still dead + (confirmed pre-existing). +- **Impact:** Modest in isolation, but dead code inflates every audit — the + deleted file alone accounted for 9 deprecated-field sites and 14 + `SqlExpression` match arms in the R2/R3 survey. +- **Decision:** Not worth a dedicated project. Cheapest fix is to stop tolerating + the warnings: the tree currently emits ~368 clippy warnings and ~67 + `#[allow(deprecated)]` annotations, which is enough noise to hide a real + signal. Consider a `[lints]` table in `Cargo.toml` once the count is down. + +### R6 — `CorrelatedSubqueryAnalyzer` is unwired and untested +- **Status:** 🔴 OPEN +- **Where:** `src/query_plan/correlated_subquery_analyzer.rs` +- **Observed:** It already has the scope stack that P3 needs (`scope_stack: + Vec>`, push/pop, outer-frame lookup) and a stubbed + `Exists { negated }` variant marked *"not yet parsed, but for future"*. But it + is not wired into the execution path, and its tests are hollow — + `test_non_correlated_scalar_subquery` builds a statement containing **no + subquery at all** and asserts the count is zero, so correlation detection is + effectively untested. +- **Also:** `collect_references_from_expr` records only `table_prefix`, not + column names, and misses several expression variants (an instance of R3). +- **Decision:** Fix as part of P3 rather than standalone — it is the natural + detection stage. Reuse, don't rewrite: the structure is right, the wiring and + tests are missing. + +### R7 — Subqueries are substituted, not evaluated +- **Status:** 🔴 OPEN — the structural root cause behind parity [P3](SQL_PARITY.md) +- **Where:** `src/data/subquery_executor.rs` +- **Observed:** Subquery execution is a whole-statement **AST rewrite pass** that + runs once, up front, replacing each subquery node with literal values before + any row is touched. There is no outer-row context anywhere in the evaluation + path. Two aggravating details: + - The result cache keys on the AST debug string alone + (`format!("scalar:{:?}", query)`), which is actively wrong for correlation — + identical AST across outer rows, different required results. + - `arithmetic_evaluator.rs:250` falls back to dropping a column's qualifier + when it doesn't resolve, so an unresolved *outer* reference silently binds to + an inner column instead of erroring. This turns a missing feature into a + **silent wrong answer**. +- **Impact:** Blocks all five correlated-subquery parity cases, and more + generally blocks any nested-scope feature. +- **Decision:** Fix via P3. Keep the substitution path for uncorrelated + subqueries (three corpus cases pass *because* of it) and fork correlated nodes + to per-row evaluation — not a replacement. + +--- + +## Sequencing + +The dependency that matters: **the advanced work is gated on R2/R3.** Richer +expression and function support means new `SqlExpression` variants, and today +each one costs a 40-file hand audit with silent failure as the default outcome. + +``` +R2 walkers ──┬─→ R3 catch-alls retired + │ + └─→ P3 Exists variant + scope spine ──→ correlated subqueries + ↑ + R6 analyzer wiring + ↑ + R7 per-row evaluation + +R1 FROM migration ── independent; deferred (larger than P3) +R4 fixtures ──────── adopt opportunistically, per transformer touched +R5 dead code ─────── opportunistic +``` + +## Log + +| Date | Change | PR | +|---|---|---| +| 2026-07-18 | R1 partial: FROM sync helpers; four desync sites fixed; dead lifter deleted | #30 | +| 2026-07-18 | R2 partial: `walk.rs` traversal helpers landed (additive) | #31 | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 3d62a566..b1e62231 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -11,6 +11,9 @@ and either **fix** divergences or record **why we won't**. says *what* diverges right now; this file says *what we're doing about it and why*. - **CI gate:** `runner.py --check` runs in `.github/workflows/test-complete.yml` (job *SQL Parity*) and fails the build on any drift from the contract below. +- **Companion log:** [`ENGINE_REFACTORING.md`](ENGINE_REFACTORING.md) (R-numbers) + tracks *structural* debt rather than wrong answers. Where a parity gap turns + out to have a shape problem underneath it, the P-entry links to the R-entry. ### Regression contract @@ -119,6 +122,12 @@ annotation be removed. - **Notes:** Uncorrelated scalar / `IN` / derived-table subqueries already AGREE; the gap is specifically the outer-row binding. Highest-leverage fix here — one root cause unlocks five cases and the broader nested-scoping goal. +- **Structural root cause:** [R7](ENGINE_REFACTORING.md) — subqueries are + *substituted* by an up-front AST rewrite pass, never *evaluated* per row, so + there is no outer row to correlate against. Groundwork tracked as + [R2](ENGINE_REFACTORING.md) (traversal helpers, needed before the `EXISTS` + variant can be added safely) and [R6](ENGINE_REFACTORING.md) (the existing + correlation analyzer is unwired and untested). ### P4 — Self-join of the base table fails to resolve - **Status:** 🟢 FIXED (2026-07-11)