Skip to content

fix(parity): resolve every WHERE operand through one path (P46) - #80

Merged
TimelordUK merged 3 commits into
mainfrom
fix/p46-where-rhs-operand
Sep 13, 2026
Merged

TimelordUK merged 3 commits into
mainfrom
fix/p46-where-rhs-operand

Conversation

@TimelordUK

Copy link
Copy Markdown
Owner

Closes P46, which turned out wider than it was filed, plus the WHERE 1=0 rejection seen in the countries demo.

What was wrong

RecursiveWhereEvaluator read right-hand operands with a literal-only reader that ended _ => Ok(Null). Any column, CASE or expression on the right of a comparison, in an IN list or as a BETWEEN bound read as NULL, so the predicate was UNKNOWN on every row.

Shape Was DuckDB
id < partner_id, team = team 0 rows 1, 4, 7, 9 / all non-NULL
NOT (id < partner_id) 0 rows 2, 5, 8
score < CASE WHEN … END 0 rows 1, 2, 5, 8, 9
partner_id BETWEEN id AND 10 0 rows 1, 4, 7
partner_id IN (id + 2, 5) 4 only 1, 4, 9
NOT (id + 0 < partner_id) 8 rows (too many) 2, 5, 8
3 < id, WHERE 1=0 "Expected column name" error ✅

The last two rows are two more holes in the same function. An arithmetic operand sent the whole comparison to the ArithmeticEvaluator, which answers false for NULL, so P48 leaked in under NOT. And the left operand had to be a column. JOIN … ON and HAVING were probed and are clean.

The fix

Every operand now resolves through evaluate_operand_value. Columns and literals are read directly; anything else goes to the ArithmeticEvaluator. Every comparison stays in compare_trilean, so R10's NULL → UNKNOWN rule holds whatever shape the operands take. extract_value and ExprValue are deleted. As a result P48 no longer needs to land with this, and is projection-only.

Two traps, handled before committing:

  • Speed. ArithmeticEvaluator::new builds every registry, and the old code built one per row. An arithmetic operand over 100k rows took 9.8 s; an inline window over 2k rows took 9.5 s (quadratic). One shared evaluator per WHERE evaluation brings those to 2.9 s and 60 ms.
  • A false P15 fix. Once operands could be any expression, a raw window function evaluated and qualify_row_number went AGREE. But the window was computed over the rows WHERE is still filtering, so QUALIFY plus WHERE returned 1 row where DuckDB returns 3. A raw window operand is now refused, the rule evaluate_value_as_predicate already applied. qualify_inline_with_where (GAP) guards it.

Accepted trade: P49

With right-hand columns resolved, correlated outer references (x.region = s.region) bind to the inner table's column through the resolver's existing unqualified fallback. Two correlated scalar-subquery cases go from GAP to DIFFER, and in_subquery_correlated agrees by coincidence. Filed as P49 with the options considered. The proper fix comes with P3.

Corpus and tests

  • Commit 1 adds six cases that all DIFFER on the unfixed binary. Commit 2 flips them and adds where_literal_on_left, where_constant_true and qualify_inline_with_where.
  • Parity: 157 → 168 AGREE over 197 cases, and --check passes.
  • operand_resolution_tests: six per-row Trilean assertions, including NOT-over-NULL and the refused window operand.
  • Rust, Python and example suites green. The one SMOKE failure (expander_rewriters query 7, P26 shape) fails identically on main.

Docs

P46 closed with the full diagnosis. P49 filed. P48 decoupled. P15 now warns against fixing it inside the WHERE evaluator. R3 gains the catch-all row. The fix-order table puts P47 next.

This work also showed WHERE and the value evaluator each implement every boolean operator, with different NULL semantics. That becomes R13 in a follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW

TimelordUK and others added 3 commits September 13, 2026 08:52
Re-probing P46 from a second angle showed the cause is wider than
column-vs-column: every right-hand operand of a WHERE predicate (the
comparison RHS, IN-list items, BETWEEN bounds) went through a literal-only
reader that answered NULL for anything else. Six cases, all DIFFER against
the current binary, including the NOT (expr < col) shape where P48 leaks
into WHERE and returns too many rows.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW
The WHERE evaluator read right-hand operands with a literal-only reader that
ended `_ => Ok(Null)`: a column, CASE or expression on the right of a
comparison, in an IN list or as a BETWEEN bound read as NULL, so the predicate
was UNKNOWN on every row and returned zero rows. A literal on the LEFT was a
hard error (`WHERE 1=0`), and an arithmetic operand handed the whole comparison
to the ArithmeticEvaluator, which answers false for NULL - so NOT admitted the
NULL rows.

All operands now go through `evaluate_operand_value` (columns and literals
direct, anything else via the ArithmeticEvaluator) and every comparison stays
in `compare_trilean`, keeping the NULL -> UNKNOWN rule. `extract_value` and
`ExprValue` are gone.

Two traps handled on the way:
- one ArithmeticEvaluator is now shared per evaluation; building one per row
  cost 9.8s on 100k rows and made an inline window quadratic (2k rows: 9.5s)
- a raw window-function operand is refused, as evaluate_value_as_predicate
  already did: evaluating it ranks the rows WHERE is still filtering, so
  QUALIFY + WHERE silently returned the wrong rows (P15 stays GAP, guarded
  by qualify_inline_with_where)

Parity 157 -> 168 AGREE over 197 cases. Three correlated-subquery cases move
because outer references now bind to the inner table (P49), accepted and
re-bucketed with comments.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW
P46 was wider than filed - every non-literal right-hand WHERE operand read as
NULL - and the entry now records the re-probe, the fix, and the two traps it
walked into. P49 logs the correlated-subquery trade accepted with it. P48 no
longer needs to land with P46 and is projection-only. P15 gains a warning not
to fix it inside the WHERE evaluator. R3 gains the catch-all row.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW
@TimelordUK
TimelordUK merged commit 69c544a into main Sep 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant