From f69f1011673fd3569a129a587f38307aaaf8ec47 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Fri, 18 Sep 2026 18:48:48 +0100 Subject: [PATCH 1/4] test(r13): pin the operand readers before comparisons delegate (R13 slice 4) WHERE and the value evaluator each turn a literal into a value with their own reader. A third matrix, generated by DuckDB, runs the comparison shapes where the two readers can disagree through both evaluators: - P54 (filed): WHERE reads a number literal through f64, so an integer beyond 2^53 is rounded onto its neighbour. `WHERE id = 9007199254740993` returns the row with id ...992. Three WHERE entries recorded. Corpus case where_big_int_literal_equals pins it end to end (DIFFER). - `s = 5.0` against the string '5.0': WHERE reads the literal as Integer 5 and compares as text. One WHERE entry. - P53 (filed, low priority): `'5.0' = 5` compares as text in both evaluators; DuckDB casts. Recorded for both; no corpus case (explained in the entry). IN and BETWEEN already answer correctly: delegating them in part 1 fixed P54 for those operators without anyone noticing. Control case where_big_int_literal_in_list AGREEs. check_matrix now takes its table. Parity 175 AGREE / 206, contract holds. Co-Authored-By: Claude Opus 5 (1M context) --- data/big_ids.csv | 4 + docs/SQL_PARITY.md | 41 +++++++++ tests/comparison/corpus/02_where.toml | 20 ++++ tests/evaluator_matrix_tests.rs | 128 ++++++++++++++++++++++++-- 4 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 data/big_ids.csv diff --git a/data/big_ids.csv b/data/big_ids.csv new file mode 100644 index 00000000..4008606c --- /dev/null +++ b/data/big_ids.csv @@ -0,0 +1,4 @@ +id,name +9007199254740993,alpha +9007199254740992,bravo +12,charlie diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 269e47e4..ee54922c 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -2459,6 +2459,47 @@ 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:** 🔴 OPEN — silent, wrong row. [R13](ENGINE_REFACTORING.md#r13) + slice 4 (comparison arms delegate) closes it. +- **Corpus:** `02_where.toml :: where_big_int_literal_equals` + (`expect = "DIFFER"`); control `where_big_int_literal_in_list` AGREEs. +- **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) diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index d1028f58..3cb9b81b 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -273,3 +273,23 @@ 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" +expect = "DIFFER" +# P54. 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. + +[[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. diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index fc804bfd..abe32613 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -60,8 +60,6 @@ fn table() -> DataTable { t } -const ROWS: usize = 5; - /// `(predicate, SQL answer per row)`, generated by DuckDB from the same table: /// `SELECT CASE WHEN (p) IS NULL THEN 'U' WHEN (p) THEN 'T' ELSE 'F' END`. const EXPECTED: &[(&str, &str)] = &[ @@ -161,6 +159,102 @@ const KNOWN: &[Known] = &[]; // every construction site, and all 11 went FIXED in one change. const KNOWN_CASE_INSENSITIVE: &[Known] = &[]; +/// The operand readers (R13 slice 4). The two evaluators turn a literal into a +/// value with their own code, so a predicate over the same column can compare +/// against two different values. These rows exercise the readers rather than +/// NULL logic: an integer beyond 2^53, where an `f64` round trip changes the +/// value, and numbers held as strings. +/// +/// | row | big | s | n | +/// |-----|------------------|-------|------| +/// | 0 | 9007199254740993 | '5.0' | 5.0 | +/// | 1 | 9007199254740992 | '5' | NULL | +/// | 2 | NULL | NULL | 7.5 | +fn operand_table() -> DataTable { + let mut t = DataTable::new("t"); + t.add_column(DataColumn::new("big")); + t.add_column(DataColumn::new("s")); + t.add_column(DataColumn::new("n")); + let int = |v: Option| v.map_or(DataValue::Null, DataValue::Integer); + let text = |v: Option<&str>| v.map_or(DataValue::Null, |s| DataValue::String(s.to_string())); + let float = |v: Option| v.map_or(DataValue::Null, DataValue::Float); + for (big, s, n) in [ + (Some(9_007_199_254_740_993), Some("5.0"), Some(5.0)), + (Some(9_007_199_254_740_992), Some("5"), None), + (None, None, Some(7.5)), + ] { + t.add_row(DataRow::new(vec![int(big), text(s), float(n)])) + .unwrap(); + } + t +} + +/// Generated by DuckDB from the same table (`big BIGINT, s VARCHAR, +/// n DOUBLE`), as for `EXPECTED`. +const EXPECTED_OPERANDS: &[(&str, &str)] = &[ + // An integer literal beyond 2^53, both sides of each comparison. + ("big = 9007199254740993", "TFU"), + ("big <> 9007199254740993", "FTU"), + ("big = 9007199254740992", "FTU"), + ("9007199254740993 = big", "TFU"), + ("big > 9007199254740992", "TFU"), + ("big IN (9007199254740993, 1)", "TFU"), + ("big BETWEEN 9007199254740993 AND 9007199254740994", "TFU"), + // A number held as a string, against an integer and a decimal literal. + ("s = 5.0", "TTU"), + ("s = 5", "TTU"), + // A float column against integer and decimal literals. + ("n = 5", "TUF"), + ("n = 7.5", "FUT"), + ("n > 7", "FUT"), +]; + +const WHERE_ROUNDS_BIG_LITERALS: &str = + "P54: WHERE reads a number literal through f64, so an integer \ + beyond 2^53 is rounded onto its neighbour; the value evaluator parses i64 first"; +const STRING_VS_INTEGER: &str = "P53: '5.0' = 5 compares as text in both evaluators; DuckDB casts \ + the string to the number"; + +const KNOWN_OPERANDS: &[Known] = &[ + Known { + evaluator: Evaluator::Where, + predicate: "big = 9007199254740993", + observed: "FTU", + why: WHERE_ROUNDS_BIG_LITERALS, + }, + Known { + evaluator: Evaluator::Where, + predicate: "big <> 9007199254740993", + observed: "TFU", + why: WHERE_ROUNDS_BIG_LITERALS, + }, + Known { + evaluator: Evaluator::Where, + predicate: "9007199254740993 = big", + observed: "FTU", + why: WHERE_ROUNDS_BIG_LITERALS, + }, + Known { + evaluator: Evaluator::Where, + predicate: "s = 5.0", + observed: "FTU", + why: "WHERE reads the literal 5.0 as Integer 5, and '5.0' does not equal 5 as text \ + (P53); the value evaluator keeps it a Float and compares numerically", + }, + Known { + evaluator: Evaluator::Where, + predicate: "s = 5", + observed: "FTU", + why: STRING_VS_INTEGER, + }, + Known { + evaluator: Evaluator::Value, + predicate: "s = 5", + observed: "FTU", + why: STRING_VS_INTEGER, + }, +]; + fn trilean_char(t: Trilean) -> char { match t { Trilean::True => 'T', @@ -176,7 +270,7 @@ fn observe_where(table: &DataTable, predicate: &str, case_insensitive: bool) -> .unwrap_or_else(|e| panic!("parse failed for {sql}: {e}")); let where_clause = statement.where_clause.expect("expected a WHERE clause"); let mut evaluator = RecursiveWhereEvaluator::with_case_insensitive(table, case_insensitive); - (0..ROWS) + (0..table.row_count()) .map(|row| match evaluator.evaluate(&where_clause, row) { Ok(t) => trilean_char(t), Err(_) => 'E', @@ -196,7 +290,7 @@ fn observe_value(table: &DataTable, predicate: &str, case_insensitive: bool) -> other => panic!("expected one expression select item for {sql}, got {other:?}"), }; let mut evaluator = ArithmeticEvaluator::new(table).with_case_insensitive(case_insensitive); - (0..ROWS) + (0..table.row_count()) .map(|row| match evaluator.evaluate(&expr, row) { Ok(DataValue::Boolean(true)) => 'T', Ok(DataValue::Boolean(false)) => 'F', @@ -209,22 +303,36 @@ fn observe_value(table: &DataTable, predicate: &str, case_insensitive: bool) -> #[test] fn every_predicate_through_both_evaluators() { - check_matrix(EXPECTED, KNOWN, false); + check_matrix(&table(), EXPECTED, KNOWN, false); } #[test] fn every_predicate_through_both_evaluators_case_insensitive() { - check_matrix(EXPECTED_CASE_INSENSITIVE, KNOWN_CASE_INSENSITIVE, true); + check_matrix( + &table(), + EXPECTED_CASE_INSENSITIVE, + KNOWN_CASE_INSENSITIVE, + true, + ); +} + +#[test] +fn every_operand_reading_through_both_evaluators() { + check_matrix(&operand_table(), EXPECTED_OPERANDS, KNOWN_OPERANDS, false); } -fn check_matrix(expected_answers: &[(&str, &str)], known: &[Known], case_insensitive: bool) { - let t = table(); +fn check_matrix( + t: &DataTable, + expected_answers: &[(&str, &str)], + known: &[Known], + case_insensitive: bool, +) { let mut problems = Vec::new(); let mut matrix = Vec::new(); for &(predicate, expected) in expected_answers { - let where_got = observe_where(&t, predicate, case_insensitive); - let value_got = observe_value(&t, predicate, case_insensitive); + let where_got = observe_where(t, predicate, case_insensitive); + let value_got = observe_value(t, predicate, case_insensitive); matrix.push(format!( "{predicate:<44} expected {expected} where {where_got} value {value_got}" )); From 8889e6f89b70fd80eb646f5360351a633ecfae5e Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Fri, 18 Sep 2026 18:52:06 +0100 Subject: [PATCH 2/4] fix(r13): DATETIME() is today in local time in the value evaluator too (R13 slice 4) WHERE's operand reader took today's date from `Local`, the value evaluator from `Utc`. They differ for the hours where the local and UTC dates differ - 00:00-01:00 under BST - so `SELECT d >= DATETIME()` and `WHERE d >= DATETIME()` compared against different days. DuckDB's `current_date` uses the session time zone, local by default, so WHERE had it right. Lands before the comparison arms delegate: otherwise delegating would have moved WHERE onto the UTC date. Not pinned first, unlike the rest of the slice: the divergence exists only while the two dates differ, so a test could not fail on demand without injecting a clock. Parity report byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/arithmetic_evaluator.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 87e9de98..43791c78 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -1581,10 +1581,14 @@ impl<'a> ArithmeticEvaluator<'a> { minute: Option, second: Option, ) -> Result { - 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); From 67f9068f86aee5a844ffdd3456f53496872b4f3d Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Fri, 18 Sep 2026 18:58:15 +0100 Subject: [PATCH 3/4] refactor(r13): WHERE's comparisons delegate to the value evaluator (R13 slice 4) `=`, `<>`, `!=`, `<`, `<=`, `>`, `>=` in WHERE now evaluate through `ArithmeticEvaluator` and cross back in `evaluate_delegated`. The raw window check stays on WHERE's side. Two shapes keep WHERE's path for now: a legacy method call on the left (`Length()`, `IndexOf()`, `Trim*()`), whose registry twins are slice 5's, and the operators with their own arms (LIKE, IS NULL). `evaluate_binary_op` now receives the node as well as its parts, so neither the delegation nor the existing P37 value-as-predicate branch rebuilds (and clones) the expression per row. Closes P54: the four operand-matrix entries recorded by the pin commit went FIXED and nothing else moved; `where_big_int_literal_equals` DIFFER -> AGREE, parity 175 -> 176 AGREE over 206. Over trades_100000.csv, `WHERE price + 0 > quantity` 3.0 s -> 2.5 s (load time); other WHERE shapes unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- docs/SQL_PARITY.md | 17 ++++++++++---- src/data/recursive_where_evaluator.rs | 30 +++++++++++++++++++------ tests/comparison/corpus/02_where.toml | 14 +++++++----- tests/evaluator_matrix_tests.rs | 32 ++++----------------------- 4 files changed, 49 insertions(+), 44 deletions(-) diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index ee54922c..d58f7ec9 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -2477,10 +2477,19 @@ out of date. - **Found:** 2026-09-18, pinning the operand readers for R13 slice 4. ### P54 — An integer literal beyond 2^53 is rounded in `WHERE` -- **Status:** 🔴 OPEN — silent, wrong row. [R13](ENGINE_REFACTORING.md#r13) - slice 4 (comparison arms delegate) closes it. -- **Corpus:** `02_where.toml :: where_big_int_literal_equals` - (`expect = "DIFFER"`); control `where_big_int_literal_in_list` AGREEs. +- **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 diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index 793e889a..137f25d2 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/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 { diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index 3cb9b81b..b62be2c1 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -280,11 +280,15 @@ sql = "SELECT id FROM null_edges WHERE 1 = 1 AND team = 'beta' ORDER BY id" id = "where_big_int_literal_equals" data = "big_ids.csv" sql = "SELECT id, name FROM big_ids WHERE id = 9007199254740993" -expect = "DIFFER" -# P54. 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. +# 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" diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index abe32613..eecd4fc6 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -209,38 +209,14 @@ const EXPECTED_OPERANDS: &[(&str, &str)] = &[ ("n > 7", "FUT"), ]; -const WHERE_ROUNDS_BIG_LITERALS: &str = - "P54: WHERE reads a number literal through f64, so an integer \ - beyond 2^53 is rounded onto its neighbour; the value evaluator parses i64 first"; const STRING_VS_INTEGER: &str = "P53: '5.0' = 5 compares as text in both evaluators; DuckDB casts \ the string to the number"; +// R13 slice 4 recorded four WHERE entries here - three from P54 (an integer +// literal beyond 2^53 rounded through f64) and `s = 5.0` (5.0 read as Integer +// 5). All four went FIXED when WHERE's comparisons started delegating to the +// value evaluator, whose reader was already right. P53 is in both evaluators. const KNOWN_OPERANDS: &[Known] = &[ - Known { - evaluator: Evaluator::Where, - predicate: "big = 9007199254740993", - observed: "FTU", - why: WHERE_ROUNDS_BIG_LITERALS, - }, - Known { - evaluator: Evaluator::Where, - predicate: "big <> 9007199254740993", - observed: "TFU", - why: WHERE_ROUNDS_BIG_LITERALS, - }, - Known { - evaluator: Evaluator::Where, - predicate: "9007199254740993 = big", - observed: "FTU", - why: WHERE_ROUNDS_BIG_LITERALS, - }, - Known { - evaluator: Evaluator::Where, - predicate: "s = 5.0", - observed: "FTU", - why: "WHERE reads the literal 5.0 as Integer 5, and '5.0' does not equal 5 as text \ - (P53); the value evaluator keeps it a Float and compares numerically", - }, Known { evaluator: Evaluator::Where, predicate: "s = 5", From d3f659ae503688c08212159471c998a707f13083 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Fri, 18 Sep 2026 18:58:53 +0100 Subject: [PATCH 4/4] docs(r13): record slice 4 second part (comparisons, P53/P54) Co-Authored-By: Claude Opus 5 (1M context) --- docs/ENGINE_REFACTORING.md | 40 ++++++++++++++++++++++++++++++++++++-- docs/SQL_PARITY.md | 9 +++++---- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 368aa207..ee59baa5 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/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 | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index d58f7ec9..d95887b6 100644 --- a/docs/SQL_PARITY.md +++ b/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 |