TimelordUK · TimelordUK · Jul 18, 2026 · Jul 18, 2026
diff --git 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
@@ -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::<Variant>` 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<HashSet<String>>`, 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
@@ -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)