Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions docs/ENGINE_REFACTORING.md
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,100 @@ feature work**, and so we can tell the difference between "this is awkward" and
[P42](SQL_PARITY.md#p42) is the natural companion to step 2 — porting `MODE`'s
type handling from the dead implementation to the live one is most of that fix.

### R13 — Two expression evaluators, every boolean operator implemented twice
- **Status:** 🔴 OPEN — filed 2026-09-13 out of [P46](SQL_PARITY.md#p46).
**The active workstream:** parity fixes that touch expression evaluation land
as slices of this entry, not as patches.
- **Where:** `src/data/arithmetic_evaluator.rs` (`ArithmeticEvaluator`, value →
`DataValue`, 1.8k lines) and `src/data/recursive_where_evaluator.rs`
(`RecursiveWhereEvaluator`, predicate → `Trilean`, 1.6k lines). Callers in
`query_engine.rs`, `group_by_expressions.rs` and `hash_join.rs`.
- **Observed:** the engine began with one evaluator per clause and never
converged. Both walk the same `SqlExpression` tree and each has its own arms
for the same operators:

| | `ArithmeticEvaluator` | `RecursiveWhereEvaluator` |
|---|---|---|
| Serves | SELECT, GROUP BY, HAVING, JOIN conditions, generator args — and, since P46, WHERE *operands* | WHERE only (one construction site, `query_engine.rs`) |
| Own arms for | `IN`, `NOT IN`, `BETWEEN`, `NOT`, `AND`/`OR`, `LIKE`, `IS NULL`, `CASE`, method calls | the same set again, plus `CASE`-as-predicate |
| Logic | two-valued: comparisons/`IN`/`BETWEEN` answer `false`, `AND`/`OR` go through `to_bool` (NULL → false) | three-valued ([R10](#r10)) |
| Method calls | map `.Contains()` etc. onto registry functions | hand-rolled `contains`/`startswith`/`endswith`/`trim*`/`length` |
| Case-insensitive | hard-coded `false` in its `compare_with_op` calls | honoured |

And three different "is this true?" collapses: `Trilean::is_true`
(WHERE), `ArithmeticEvaluator::to_bool` (inside the value evaluator), and
`group_by_expressions::is_truthy` (HAVING). JOIN compares with bare
`compare_with_op`, which answers `NULL = NULL` as equal.
- **Impact: one predicate means different things depending on the clause it is
written in.** Probed 2026-09-13 on `null_edges.csv`, none of it filed yet:

| Shape | Where | Ours | SQL / DuckDB |
|---|---|---|---|
| `NOT (score > bonus)`, bonus all NULL | WHERE | no rows ✅ | no rows |
| `NOT (MAX(score) > MIN(bonus))` | HAVING | **all 5 groups** 🚫 | none |
| `score IN (50, NULL)` for a NULL score | SELECT | **true** 🚫 | NULL |
| `a.label = b.label` self-join, 5 NULL labels | JOIN ON | **32 rows** 🚫 (NULLs pair up, 7 + 5×5) | 7 |
| `id < partner_id` for a NULL partner | SELECT | **false** 🚫 | NULL — [P48](SQL_PARITY.md#p48) |

[P46](SQL_PARITY.md#p46) was the same split from the other side: WHERE lagging
on operand resolution the value evaluator already had. So every operator fix
is two fixes, and the history says they drift — R10 fixed NULL logic in one
evaluator and none of the others. This is the finding that makes future
semantics work ad hoc by construction.
- **Also:** evaluator construction is scattered — 16 construction sites outside
the evaluator's own file, each `ArithmeticEvaluator::new` rebuilding the
function registry and both aggregate registries. P46 showed the per-row form
of this costing 9.8 s over 100k rows.
- **Target shape — one decision tree.** A single expression evaluator: every
expression evaluates to a `DataValue`, with `Null` standing for UNKNOWN when
the expression is a predicate. The R10 truth tables become its `AND`/`OR`/`NOT`
arms. Every filtering site — WHERE, HAVING, JOIN ON, QUALIFY, CASE WHEN —
evaluates the same way and collapses exactly once, at its own boundary, with
one sanctioned function (`Trilean::from` a value, then `is_true`).
`RecursiveWhereEvaluator` shrinks to that adapter plus the LIKE regex cache,
then goes. This is the evaluation counterpart of [R2](#r2): R2 gave traversal
one shape; R13 gives *meaning* one place.
- **Slices, in the R10 pattern — no-op slices kept apart from the one that
changes answers:**
1. **Pin the divergences, no engine change.** A per-operator matrix run through
both evaluators — `=`, `<>`, `IN`, `NOT IN`, `BETWEEN`, `LIKE`, `NOT`,
`AND`/`OR`, `CASE WHEN` — each with NULL on either side, asserting the two
agree, with today's disagreements marked. Plus corpus cases for the
clause-level shapes above (HAVING, SELECT, JOIN ON), filed as P-findings.
Acceptance: parity buckets move only by the new cases.
2. **Make `ArithmeticEvaluator` three-valued.** The one semantic slice:
comparisons, `IN`/`NOT IN`, `BETWEEN`, `LIKE` yield NULL for a NULL operand;
`AND`/`OR`/`NOT` follow the truth tables; `to_bool` stops mapping NULL to
false for predicates. Closes [P48](SQL_PARITY.md#p48) and the HAVING / SELECT
rows above. Check `CASE WHEN` (UNKNOWN takes ELSE) and every `to_bool` /
`is_truthy` caller. Acceptance: exactly the slice-1 cases flip, nothing else.
3. **One construction path.** Registries built once and shared (`Arc`), context
— case sensitivity, date notation, table aliases — passed in rather than
defaulted per site. No-op; acceptance is parity *exactly* unchanged, and it
fixes the case-insensitivity gap as a side effect only if slice 1 pinned it.
4. **Retire WHERE's duplicate arms, one operator per commit.** Each arm of
`RecursiveWhereEvaluator` delegates to the value evaluator and collapses;
the hand-written arm is deleted. After slice 2 the rules already agree, so
each commit is a provable no-op — parity exactly unchanged, and the slice-1
matrix still green.
5. **Method calls:** WHERE's hand-rolled `Contains`/`StartsWith`/`Trim`… move
onto the registry mapping the value evaluator already uses.
6. **Collapse sites converge:** HAVING's `is_truthy` and JOIN's bare comparison
go through the same collapse. JOIN NULL keys need care on the *hash* path
too, not only the nested loop. [R8](#r8) stage 2 (the legacy WHERE stack) is
a natural lull task alongside.
- **Explicitly not in scope:** window evaluation (`BatchWindowEvaluator`),
aggregate registries ([R12](#r12)), correlated scoping ([R7](#r7) /
[P3](SQL_PARITY.md#p3), [P49](SQL_PARITY.md#p49)). And [P15](SQL_PARITY.md#p15)
stays a lifter fix — a single evaluator must still refuse a raw window
function in a filter.
- **Guard rails:** the FORMAL examples capture our own output, so slice 2 may
churn expectations — review each diff as a potential [P21](SQL_PARITY.md#p21)-
style capture of a wrong answer, never re-capture blind. Watch the per-row
cost when WHERE arms start delegating (slice 4): the P46 benchmarks
(`price > quantity`, `price + 0 > quantity` over `trades_100000.csv`, inline
QUALIFY) are the baseline.

---

## Sequencing
Expand All @@ -494,6 +588,7 @@ R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a
R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129)
R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run
R12 aggregate registries ─ independent; step 1 is a provable no-op, do it before the next aggregate fix
R13 one evaluator ─── ACTIVE from 2026-09-13; slice 1 (pin) → 2 (3VL, = P48) → 3 (construction) → 4–6 (retire WHERE arms)
```

**A note on ordering, from the P18/P19 work being next.** The WHERE evaluator
Expand Down Expand Up @@ -529,3 +624,5 @@ AGREE count — which makes it safe to land well before the semantics change.
| 2026-08-30 | P34 fixed: `ORDER BY "col.with.dot"` no longer strips a quoted identifier at the dot. R11 filed — ORDER BY still resolves columns with its own copy of `resolve_column_index` rather than the canonical one | — |
| 2026-09-04 | P40 filed from field use: a generator's args are evaluated against DUAL (`statement_executor.rs` has no `from_function` case), so no column reference resolves in `FROM SPLIT(col, …)`. Cross-linked here — R1's missing table-function variant is the reason generators sit on the legacy path | — |
| 2026-09-06 | R12 filed by parity P41: two aggregate registries, nine functions implemented twice with the newer one shadowing the older. P41's fix was written against the dead copy first and changed nothing — the entry records the disjointness assertion as step 1 | — |
| 2026-09-13 | P46 fixed: all WHERE operands resolve through one path, one `ArithmeticEvaluator` per evaluation. Parity 157 → **168 AGREE**; P49 filed | #80 |
| 2026-09-13 | R13 filed and made the active workstream: two evaluators with divergent NULL semantics, three truth collapses. Probing found four live clause-dependent divergences (HAVING, SELECT `IN`, JOIN NULL keys, P48) | — |
10 changes: 8 additions & 2 deletions docs/SQL_PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,9 @@ Suggested fix order, by silent blast radius:
| ~~9d~~ | ~~[P37](#p37) window in `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-05** — corpus count unchanged, and that is the finding: the case is `OURS_ONLY` before *and* after, so the harness cannot see this fix or a future regression of it (first entry of that kind — the regression test is a Rust module). The filed root cause was wrong: `ExpressionLifter` *does* lift from `WHERE`. The real defect was one arm in the WHERE evaluator answering FALSE for any bare value used as a predicate — `WHERE true` returned zero rows too. It did **not** close [P15](#p15), which needs the opposite change |
| ~~9e~~ | ~~[P41](#p41) `MODE` tie-break is random per run~~ | ✅ **Fixed 2026-09-06** — 152 → **156 AGREE** (four new cases). Small, as predicted, but not where it was filed: the named `ModeState` was a *shadowed* implementation and fixing it moved nothing. Reference does specify a rule and it is **first-occurrence**, not the "smallest value wins" this row proposed. Unblocked both example files, now FORMAL. Spun off [P42](#p42), [P43](#p43), [R12](ENGINE_REFACTORING.md#r12) |
| ~~9g~~ | ~~[P46](#p46) column-vs-column `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-13** — 157 → **168 AGREE**. Wider than filed: *every* non-literal right-hand operand read as NULL (comparisons, `IN` items, `BETWEEN` bounds), and a literal on the left errored (`WHERE 1=0`). One resolver for all operands closed it. Did **not** need [P48](#p48) alongside, as this row predicted — the fix kept the comparison in the WHERE evaluator. Spun off [P49](#p49) (correlated outer refs bind to the inner table), accepted knowingly |
| **NEXT** | [P47](#p47) `MIN`/`MAX` ranked by type, [P48](#p48) NULL comparison projects `false` | Both silent, both cheap. P47 hands back a number wrong by a factor of half a million from an ordinary `GROUP BY`, and its correct comparator is already in the tree. P48 is now projection-only and independent of P46 |
| then | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken. Was row 9b, then NEXT until the 2026-09-12 findings displaced it |
| **NEXT** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | Not a finding but the reason several are cheap to fix only once. WHERE and the value evaluator implement every boolean operator separately with different NULL rules, so the same predicate answers differently in WHERE, HAVING, SELECT and JOIN ON. **Working rule:** a parity fix that touches expression evaluation lands as an R13 slice, not a patch. [P48](#p48) *is* R13 slice 2 |
| then | [P47](#p47) `MIN`/`MAX` ranked by type | Silent and cheap, and outside the evaluator (aggregate registries — confirm the live one first, [R12](ENGINE_REFACTORING.md#r12)), so it can land alongside R13 without breaking the working rule |
| then | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken. Was row 9b, then NEXT until the 2026-09-12 findings displaced it. **Check each against the R13 working rule first** — P20 (`\|\|` with NULL) is an operator in the value evaluator |
| 9f | [P42](#p42) `MODE` is numeric-only | Companion to [R12](ENGINE_REFACTORING.md#r12), and cheap if taken with it: the shadowed implementation already handles non-numerics and preserves type, so the fix is largely to stop the live path throwing away what it knows. Also buys the corpus its clearest tie-break case |
| 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32), [P38](#p38) | Hard errors — visible, so less urgent than any of the above |
| 10b | [P39](#p39) `x/0` errors, voiding the whole statement | Hard error like row 10, but the only one whose blast radius is the *query* rather than the cell. Settle the four inconsistent call sites as one decision; it currently has no live probe (see the entry) |
Expand Down Expand Up @@ -2286,6 +2287,11 @@ out of date.
leaked into `WHERE` (`NOT (id + 0 < partner_id)` returned 8 rows, DuckDB 3).
What remains is projection only. Check `CASE`, `AND`/`OR` and `to_bool` still
handle a NULL comparison result when taking it.
- **Update 2026-09-13 — taken as [R13](ENGINE_REFACTORING.md#r13) slice 2,
not as a spot fix.** Making only the comparison arm yield NULL would leave
`IN`, `BETWEEN`, `AND`/`OR` and HAVING's `is_truthy` two-valued — the drift R13
exists to end. It lands with the rest of the value evaluator's three-valued
conversion, after R13 slice 1 pins every operator.
- **Related:** [P18](#p18), [P19](#p19), [P20](#p20) (`||` treating NULL as an
empty string — the same coercion instinct in the string operators),
[P46](#p46).
Expand Down
Loading