refactor(r13): WHERE comparisons delegate to the value evaluator; closes P54 (R13 slice 4, part 2)

data/big_ids.csv

@@ -0,0 +1,4 @@
+id,name
+9007199254740993,alpha
+9007199254740992,bravo
+12,charlie

docs/ENGINE_REFACTORING.md

@@ -473,7 +473,8 @@ feature work**, and so we can tell the difference between "this is awkward" and
 ### R13 — Two expression evaluators, every boolean operator implemented twice
 - **Status:** 🟡 IN PROGRESS — filed 2026-09-13 out of [P46](SQL_PARITY.md#p46);
   **slices 1 and 2 done 2026-09-13, slice 3 (with 3b) done 2026-09-14**, slice 4
-  in progress (`BETWEEN`, `IN`, `NOT IN` delegated 2026-09-15).
+  in progress (`BETWEEN`, `IN`, `NOT IN` delegated 2026-09-15; comparisons
+  2026-09-18, closing P54).
   **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 →
@@ -639,6 +640,40 @@ feature work**, and so we can tell the difference between "this is awkward" and
     `price > 100` 2.83 → 2.42 s, `LIKE` 2.87 → 2.43 s, all now at load time.
     Still outstanding: window evaluation logs `info!` timings per row (out of
     R13's scope).
+  - *Part 1 was not quite a no-op.* Delegating IN and BETWEEN also moved them
+    onto the value evaluator's literal reader, which fixed [P54](SQL_PARITY.md#p54)
+    for those two operators. The parity report could not see it — no case held
+    an integer above 2^53 — so "byte-identical" proved less than it claimed. A
+    delegation changes the *readers* as well as the rules; pin the readers.
+- **Slice 4, second part (2026-09-18) — the comparison operators.** `=`, `<>`,
+  `!=`, `<`, `<=`, `>`, `>=` delegate, except with a legacy method call on the
+  left. Three commits:
+  - *Pin the readers.* A third matrix (`EXPECTED_OPERANDS`, DuckDB-generated)
+    over shapes where the readers could disagree. Probing found most agree —
+    dates held as strings compare as dates either way — and three that do not:
+    **P54** (WHERE parsed every number literal through `f64`, so
+    `WHERE id = 9007199254740993` returned the row with id …992), `'5.0' = 5.0`
+    (WHERE read `5.0` as `Integer 5` and compared as text), and **P53**, wrong in
+    both evaluators (`'5.0' = 5` compares as text; low priority, see entry).
+    `check_matrix` now takes its table.
+  - *`DATETIME()` in local time.* WHERE read today from `Local`, the value
+    evaluator from `Utc` — they disagree while the dates differ (00:00–01:00
+    under BST). WHERE was right (DuckDB's `current_date` is session-local), so
+    the value evaluator changed *before* delegation, which would otherwise have
+    moved WHERE onto UTC. The one step not pinned first: it only diverges while
+    the clock says so.
+  - *Delegate.* Exactly the four WHERE pins flipped, the P53 pair held, and
+    parity moved by exactly one case (P54, 175 → **176 AGREE** / 206).
+    `evaluate_binary_op` now gets the node itself, so neither the delegation nor
+    the P37 value-as-predicate branch clones it per row.
+  - *Remaining in slice 4:* `LIKE` (WHERE compiles through `EvaluationContext`'s
+    regex cache; the value evaluator has its own matcher, and they disagree on a
+    non-string operand — `5.0 LIKE '5%'` is FALSE in WHERE, TRUE in the value
+    evaluator, and an error in DuckDB), `IS NULL` / `IS NOT NULL` (small), and
+    then `AND` / `OR` / `NOT` / `CASE`, which wait on slice 5 for their
+    method-call children. WHERE's own readers (`number_literal_value`, the
+    `DATETIME` arms) are now only reached through a legacy left-side method
+    call or LIKE.
 - **Slices, in the R10 pattern — no-op slices kept apart from the one that
   changes answers:**
   1. ✅ **Pin the divergences, no engine change.** *(Done 2026-09-13.)* A per-operator matrix run through
@@ -704,7 +739,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; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 IN PROGRESS (BETWEEN/IN done) → 5–6
+R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 IN PROGRESS (BETWEEN/IN, comparisons done; LIKE, IS NULL next) → 5–6
 ```
 
 **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator
@@ -746,3 +781,4 @@ AGREE count — which makes it safe to land well before the semantics change.
 | 2026-09-13 | R13 slice 2: value evaluator three-valued; `Trilean::from_value`/`to_value`; `to_bool` removed. Matrix value column 4/36 → 36/36 in one change. Closes P48, P50 — 168 → **174 AGREE** | — |
 | 2026-09-14 | R13 slice 3: registries built once per process; one `ArithmeticEvaluator` constructor, dead date-notation plumbing deleted — parity byte-identical, high-cardinality GROUP BY ~2× faster. 3b: case-insensitive mode pinned (11 matrix + 5 clause cases) then handed in at every construction site; all flipped, parity byte-identical | — |
 | 2026-09-15 | R13 slice 4 (first part): WHERE's `BETWEEN`, `IN`, `NOT IN` delegate to the value evaluator. Latent alias-resolution fault pinned and fixed first; per-node TRACE logging found to be most of the value evaluator's cost and removed (SELECT/GROUP BY expressions ~2 s faster over 100k rows). Parity byte-identical at every commit | — |
+| 2026-09-18 | R13 slice 4 (second part): WHERE comparisons delegate. Operand readers pinned first (third matrix); P53 and P54 filed; value evaluator's `DATETIME()` moved to local time before delegating. Closes P54 — 175 → **176 AGREE**. Part 1 found to have fixed P54 for IN/BETWEEN unnoticed | — |

docs/SQL_PARITY.md

@@ -65,9 +65,10 @@ session apiece to fix properly, so **discovery is paused and the effort moves to
 picking them off**. Widen the corpus again when the open list is short, or
 opportunistically when a fix needs a case that doesn't exist yet.
 
-Corpus coverage today: tiers 01–10, **204 cases** (174 AGREE / 15 DIFFER /
-12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-13, after R13 slice 2 closed
-[P48](#p48) and [P50](#p50); [P51](#p51), [P52](#p52) open). The largest single movement so far remains the 2026-09-05
+Corpus coverage today: tiers 01–10, **206 cases** (176 AGREE / 15 DIFFER /
+12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-18, after R13 slice 4's
+comparison delegation closed [P54](#p54); [P51](#p51), [P52](#p52) open,
+[P53](#p53) open at low priority). The largest single movement so far remains the 2026-09-05
 NULL-ordering slice, which closed [P13](#p13) stage 2 and [P17](#p17) together —
 eleven cases in one change. **Tier 10 (aggregate & NULL edges) is still
 deliberately partial** — it holds the P14, P18–P20 and P41 cases and their
@@ -92,7 +93,7 @@ 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** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | **Slices 1–2 done 2026-09-13** (value evaluator three-valued, 168 → 174 AGREE, closed P48/P50); **slice 3 done 2026-09-14** (one construction path; case-insensitive mode now honoured outside WHERE, parity unmoved); slice 4 (WHERE arms delegate, one operator per commit) next. 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. [P52](#p52) (NULL join keys) is slice 6 |
+| **NEXT** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | **Slices 1–2 done 2026-09-13** (value evaluator three-valued, 168 → 174 AGREE, closed P48/P50); **slice 3 done 2026-09-14** (one construction path; case-insensitive mode now honoured outside WHERE, parity unmoved); **slice 4 in progress** (WHERE arms delegate, one operator per commit: BETWEEN/IN 2026-09-15, comparisons 2026-09-18 closing [P54](#p54), 175 → 176 AGREE; LIKE and IS NULL next). 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. [P52](#p52) (NULL join keys) is slice 6 |
 | 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 |
@@ -2459,6 +2460,56 @@ out of date.
   testing for NULL at the predicate layer rather than in the comparator),
   [P48](#p48).
 
+### P53 — A number held as text compares to an integer as text
+- **Status:** 🟡 OPEN, low priority — pinned in the evaluator matrix only.
+- **Corpus:** none; see *Why no corpus case*.
+- **Observed:** with `s` a VARCHAR holding `'5.0'`, `s = 5` is FALSE in both
+  evaluators; DuckDB casts the string to the number and answers TRUE. `s = 5.0`
+  is TRUE in the value evaluator (a `Float` literal compares numerically) — and
+  was FALSE in WHERE, which read `5.0` as `Integer 5`; see [P54](#p54).
+- **Pinned:** `tests/evaluator_matrix_tests.rs`, `EXPECTED_OPERANDS`, recorded
+  against both evaluators in `KNOWN_OPERANDS`.
+- **Why no corpus case:** the shape is hard to reach from a file. DuckDB's CSV
+  sniffer types a column of `'5.0'`, `'5'` as a number, as does our loader, so
+  the comparison is numeric on both sides. A column that stays text holds
+  something non-numeric, and there DuckDB's cast raises an error on that row
+  instead of answering. So what diverges in practice is "we answer FALSE where
+  DuckDB errors" — not worth a fix ahead of anything in the fix-order table.
+- **Found:** 2026-09-18, pinning the operand readers for R13 slice 4.
+
+### P54 — An integer literal beyond 2^53 is rounded in `WHERE`
+- **Status:** ✅ FIXED 2026-09-18 by [R13](ENGINE_REFACTORING.md#r13) slice 4:
+  WHERE's comparison arms delegate to the value evaluator, whose literal reader
+  was already right. 175 → **176 AGREE**; the four matrix entries went FIXED
+  and nothing else moved.
+- **Corpus:** `02_where.toml :: where_big_int_literal_equals` (AGREE); control
+  `where_big_int_literal_in_list`.
+- **Harness blind spot:** the harness compares numbers as floats, so both ids
+  render as `9007199254740992.0` — only the `name` column showed the wrong row.
+  A case that selects only a large integer cannot tell two of them apart; select
+  something else from the row.
+- **Still reads through f64:** a *legacy method call on the left* of a WHERE
+  comparison (`name.Length() = 9007199254740993`) keeps WHERE's own reader until
+  slice 5. Not a realistic shape; noted so the reader's last use is visible.
+- **Observed:** `SELECT id, name FROM big_ids WHERE id = 9007199254740993`
+  returns **the row with id 9007199254740992** — not zero rows, the neighbour.
+  `SELECT id = 9007199254740993` on that row projects `false`, so one query
+  contradicts itself.
+- **Cause:** WHERE's operand reader (`number_literal_value` in
+  `recursive_where_evaluator.rs`) parses every number literal as `f64` and
+  turns whole values back into `Integer`. An `f64` holds every integer only up
+  to 2^53, so a larger literal lands on the nearest representable value. The
+  value evaluator parses `i64` first and is right.
+- **Why it matters:** 64-bit identifiers (snowflake-style ids, hashes, epoch
+  nanoseconds) sit well above 2^53 — filtering an extract by one silently
+  returns a different record.
+- **Already fixed for `IN` and `BETWEEN`**, unnoticed: R13 slice 4 part 1
+  (2026-09-15) delegated those arms to the value evaluator, which reads
+  literals correctly. That slice was checked with a byte-identical parity
+  report, and no case held an integer this large — the control case exists so
+  the next such change is seen.
+- **Found:** 2026-09-18, pinning the operand readers for R13 slice 4.
+
 ---
 
 ## Deferred / won't fix (intentional)

src/data/arithmetic_evaluator.rs

@@ -1581,10 +1581,14 @@ impl<'a> ArithmeticEvaluator<'a> {
         minute: Option<u32>,
         second: Option<u32>,
     ) -> Result<DataValue> {
-        use chrono::{TimeZone, Utc};
-
-        // Get today's date in UTC
-        let today = Utc::now().date_naive();
+        use chrono::{Local, TimeZone, Utc};
+
+        // Today in the local time zone, as DuckDB's `current_date` and WHERE's
+        // operand reader both read it. This used to be the UTC date, so for the
+        // hours where the two differ (00:00-01:00 under BST) `SELECT d >=
+        // DATETIME()` and `WHERE d >= DATETIME()` saw different days. The `Utc`
+        // below only formats the naive value; it converts nothing.
+        let today = Local::now().date_naive();
 
         // Create datetime with provided time components or defaults
         let hour = hour.unwrap_or(0);

src/data/recursive_where_evaluator.rs

@@ -494,7 +494,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> {
 
         let result = match expr {
             SqlExpression::BinaryOp { left, op, right } => {
-                self.evaluate_binary_op(left, op, right, row_index)
+                self.evaluate_binary_op(expr, left, op, right, row_index)
             }
             SqlExpression::InList {
                 expr: probe,
@@ -561,8 +561,12 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> {
         result
     }
 
+    /// `expr` is the `BinaryOp` node itself, and `left` / `op` / `right` its
+    /// parts: the arms that hand the whole predicate to the value evaluator
+    /// need the node, and rebuilding it would clone the subtree on every row.
     fn evaluate_binary_op(
         &mut self,
+        expr: &SqlExpression,
         left: &SqlExpression,
         op: &str,
         right: &SqlExpression,
@@ -599,12 +603,18 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> {
         // predicate -- the P37 rule, rather than a comparison under an
         // operator `compare_with_op` would silently answer false for.
         if !is_predicate_operator(&op_upper) {
-            let value_expr = SqlExpression::BinaryOp {
-                left: Box::new(left.clone()),
-                op: op.to_string(),
-                right: Box::new(right.clone()),
-            };
-            return self.evaluate_value_as_predicate(&value_expr, row_index);
+            return self.evaluate_value_as_predicate(expr, row_index);
+        }
+
+        // A comparison is the value evaluator's (R13 slice 4): its operand
+        // readers and three-valued `compare_trilean` are the one
+        // implementation. Two shapes stay below for now - a legacy method call
+        // on the left (`Length()`, `IndexOf()`, `Trim*()`), whose registry
+        // twins are slice 5's to check, and the operators with their own arms.
+        if is_comparison_operator(&op_upper) && !matches!(left, SqlExpression::MethodCall { .. }) {
+            reject_unlifted_window(left)?;
+            reject_unlifted_window(right)?;
+            return self.evaluate_delegated(expr, row_index);
         }
 
         // Both operands are resolved to values here and compared below, so the
@@ -1152,6 +1162,12 @@ fn reject_unlifted_window(expr: &SqlExpression) -> Result<()> {
     }
 }
 
+/// The comparison operators the value evaluator implements, and so the ones a
+/// WHERE comparison delegates. Expects the operator already upper-cased.
+fn is_comparison_operator(op_upper: &str) -> bool {
+    matches!(op_upper, "=" | "!=" | "<>" | "<" | "<=" | ">" | ">=")
+}
+
 /// Operators whose result is a truth value, handled by `evaluate_binary_op`'s
 /// comparison arms. Expects the operator already upper-cased.
 fn is_predicate_operator(op_upper: &str) -> bool {

tests/comparison/corpus/02_where.toml

@@ -273,3 +273,27 @@ sql = "SELECT id FROM null_edges WHERE 1 = 1 AND team = 'beta' ORDER BY id"
 # The `WHERE 1=1 AND ...` idiom that generated SQL leans on. Constant-false is
 # the more natural probe, but an empty result cannot be compared by the harness
 # today (our empty JSON carries no column list), so the true form pins it.
+
+# --- P54: an integer literal beyond 2^53 ---
+
+[[case]]
+id = "where_big_int_literal_equals"
+data = "big_ids.csv"
+sql = "SELECT id, name FROM big_ids WHERE id = 9007199254740993"
+# P54, fixed by R13 slice 4 (WHERE comparisons delegate). WHERE read a number
+# literal through f64, which cannot hold every integer above 2^53, so
+# 9007199254740993 became 9007199254740992 and the filter returned *the
+# neighbouring row* (bravo) instead of alpha. A 64-bit id is the realistic
+# shape. SELECT's evaluator parses i64 first and was right all along.
+#
+# Keep `name` in the SELECT list: the harness compares numbers as floats, so
+# both ids render as 9007199254740992.0 and an id-only query AGREEs whichever
+# row comes back.
+
+[[case]]
+id = "where_big_int_literal_in_list"
+data = "big_ids.csv"
+sql = "SELECT id, name FROM big_ids WHERE id IN (9007199254740993, 12) ORDER BY id"
+# The control for the case above. Already AGREE: R13 slice 4 part 1 delegated
+# IN to the value evaluator, which fixed it for IN and BETWEEN without anyone
+# noticing - no case held an integer this large.