From e9e52696fe66065cc9d210ae0026deb1b3ca6f3a Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Mon, 14 Sep 2026 20:18:48 +0100 Subject: [PATCH 1/5] refactor(r13): build evaluator registries once and share them (R13 slice 3) ArithmeticEvaluator construction rebuilt the function registry, both aggregate registries and the window registry every time. The registries are immutable after construction, so build them once per process in a OnceLock and hand each evaluator Arc clones. No-op: parity output byte-identical to main (174 AGREE / 204). Per-group construction in GROUP BY was the visible cost: over trades_100000.csv, net of ~3 s load, a high-cardinality GROUP BY drops 3.4 s -> 1.9 s and GROUP BY + HAVING 2.5 s -> 0.8 s. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha --- src/data/arithmetic_evaluator.rs | 52 +++++++++++++++++--------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 24492166..c031ac27 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -12,10 +12,30 @@ use crate::sql::window_context::WindowContext; use crate::sql::window_functions::{ExpressionEvaluator, WindowFunctionRegistry}; use anyhow::{anyhow, Result}; use std::collections::{HashMap, HashSet}; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use std::time::Instant; use tracing::{debug, info}; +/// The registries every evaluator reads. None of them change after +/// construction, so the process builds them once and every evaluator shares +/// them, rather than each construction site rebuilding all four (R13 slice 3). +struct EvaluatorRegistries { + function: Arc, + aggregate: Arc, + new_aggregate: Arc, + window: Arc, +} + +fn shared_registries() -> &'static EvaluatorRegistries { + static REGISTRIES: OnceLock = OnceLock::new(); + REGISTRIES.get_or_init(|| EvaluatorRegistries { + function: Arc::new(FunctionRegistry::new()), + aggregate: Arc::new(AggregateRegistry::new()), + new_aggregate: Arc::new(AggregateFunctionRegistry::new()), + window: Arc::new(WindowFunctionRegistry::new()), + }) +} + /// Evaluates SQL expressions to compute `DataValues` (for SELECT clauses) /// This is different from `RecursiveWhereEvaluator` which returns boolean pub struct ArithmeticEvaluator<'a> { @@ -33,28 +53,19 @@ pub struct ArithmeticEvaluator<'a> { impl<'a> ArithmeticEvaluator<'a> { #[must_use] pub fn new(table: &'a DataTable) -> Self { - Self { - table, - _date_notation: get_date_notation(), - function_registry: Arc::new(FunctionRegistry::new()), - aggregate_registry: Arc::new(AggregateRegistry::new()), - new_aggregate_registry: Arc::new(AggregateFunctionRegistry::new()), - window_function_registry: Arc::new(WindowFunctionRegistry::new()), - visible_rows: None, - window_contexts: HashMap::new(), - table_aliases: HashMap::new(), - } + Self::with_date_notation(table, get_date_notation()) } #[must_use] pub fn with_date_notation(table: &'a DataTable, date_notation: String) -> Self { + let registries = shared_registries(); Self { table, _date_notation: date_notation, - function_registry: Arc::new(FunctionRegistry::new()), - aggregate_registry: Arc::new(AggregateRegistry::new()), - new_aggregate_registry: Arc::new(AggregateFunctionRegistry::new()), - window_function_registry: Arc::new(WindowFunctionRegistry::new()), + function_registry: Arc::clone(®istries.function), + aggregate_registry: Arc::clone(®istries.aggregate), + new_aggregate_registry: Arc::clone(®istries.new_aggregate), + window_function_registry: Arc::clone(®istries.window), visible_rows: None, window_contexts: HashMap::new(), table_aliases: HashMap::new(), @@ -82,15 +93,8 @@ impl<'a> ArithmeticEvaluator<'a> { function_registry: Arc, ) -> Self { Self { - table, - _date_notation: date_notation, function_registry, - aggregate_registry: Arc::new(AggregateRegistry::new()), - new_aggregate_registry: Arc::new(AggregateFunctionRegistry::new()), - window_function_registry: Arc::new(WindowFunctionRegistry::new()), - visible_rows: None, - window_contexts: HashMap::new(), - table_aliases: HashMap::new(), + ..Self::with_date_notation(table, date_notation) } } From 56e41ffb4864f1cb5ee0a0cb79b7b92bd72188cb Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Mon, 14 Sep 2026 20:27:07 +0100 Subject: [PATCH 2/5] refactor(r13): one ArithmeticEvaluator constructor (R13 slice 3) ArithmeticEvaluator had three constructors, new, with_date_notation and with_date_notation_and_registry, and the only thing separating them was a date notation string stored in a field nothing ever read. Dates parse through the global notation. The registry variant had no callers. Collapse to ArithmeticEvaluator::new(table) plus the existing with_visible_rows / with_table_aliases builders, delete the dead field, the now-unused date_notation parameter on apply_group_by_expressions and QueryEngine's own unread date_notation field. Also hoist the evaluator out of the per-row loop in estimate_group_cardinality, matching the key loop in group_by_expressions. No-op: parity output byte-identical to main (174 AGREE / 204); cargo test green, evaluator matrix included. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha --- src/data/arithmetic_evaluator.rs | 20 -------------------- src/data/group_by_expressions.rs | 17 ++++------------- src/data/query_engine.rs | 31 +++++++------------------------ 3 files changed, 11 insertions(+), 57 deletions(-) diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index c031ac27..3fa8d3ea 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -1,4 +1,3 @@ -use crate::config::global::get_date_notation; use crate::data::data_view::DataView; use crate::data::datatable::{DataTable, DataValue}; use crate::data::trilean::Trilean; @@ -40,7 +39,6 @@ fn shared_registries() -> &'static EvaluatorRegistries { /// This is different from `RecursiveWhereEvaluator` which returns boolean pub struct ArithmeticEvaluator<'a> { table: &'a DataTable, - _date_notation: String, function_registry: Arc, aggregate_registry: Arc, // Old registry (being phased out) new_aggregate_registry: Arc, // New registry @@ -53,15 +51,9 @@ pub struct ArithmeticEvaluator<'a> { impl<'a> ArithmeticEvaluator<'a> { #[must_use] pub fn new(table: &'a DataTable) -> Self { - Self::with_date_notation(table, get_date_notation()) - } - - #[must_use] - pub fn with_date_notation(table: &'a DataTable, date_notation: String) -> Self { let registries = shared_registries(); Self { table, - _date_notation: date_notation, function_registry: Arc::clone(®istries.function), aggregate_registry: Arc::clone(®istries.aggregate), new_aggregate_registry: Arc::clone(®istries.new_aggregate), @@ -86,18 +78,6 @@ impl<'a> ArithmeticEvaluator<'a> { self } - #[must_use] - pub fn with_date_notation_and_registry( - table: &'a DataTable, - date_notation: String, - function_registry: Arc, - ) -> Self { - Self { - function_registry, - ..Self::with_date_notation(table, date_notation) - } - } - /// Find a column name similar to the given name using edit distance fn find_similar_column(&self, name: &str) -> Option { let columns = self.table.column_names(); diff --git a/src/data/group_by_expressions.rs b/src/data/group_by_expressions.rs index 89fda2b4..c76f2f09 100644 --- a/src/data/group_by_expressions.rs +++ b/src/data/group_by_expressions.rs @@ -50,7 +50,6 @@ pub trait GroupByExpressions { select_items: &[SelectItem], having: Option<&SqlExpression>, _case_insensitive: bool, - date_notation: String, ) -> Result<(DataView, GroupByPhaseInfo)>; } @@ -133,7 +132,6 @@ impl GroupByExpressions for QueryEngine { select_items: &[SelectItem], having: Option<&SqlExpression>, _case_insensitive: bool, - date_notation: String, ) -> Result<(DataView, GroupByPhaseInfo)> { use std::time::Instant; let start = Instant::now(); @@ -282,11 +280,8 @@ impl GroupByExpressions for QueryEngine { let agg_start = Instant::now(); for (expr, _col_name) in &aggregate_columns { let group_rows = group_view.get_visible_rows(); - let mut evaluator = ArithmeticEvaluator::with_date_notation( - group_view.source(), - date_notation.clone(), - ) - .with_visible_rows(group_rows.clone()); + let mut evaluator = ArithmeticEvaluator::new(group_view.source()) + .with_visible_rows(group_rows.clone()); let value = if group_view.row_count() > 0 && !group_rows.is_empty() { evaluator @@ -305,10 +300,7 @@ impl GroupByExpressions for QueryEngine { for (expr, _alias) in &derived_grouped_exprs { let group_rows = group_view.get_visible_rows(); let value = if !group_rows.is_empty() { - let mut evaluator = ArithmeticEvaluator::with_date_notation( - group_view.source(), - date_notation.clone(), - ); + let mut evaluator = ArithmeticEvaluator::new(group_view.source()); evaluator .evaluate(expr, group_rows[0]) .unwrap_or(DataValue::Null) @@ -341,8 +333,7 @@ impl GroupByExpressions for QueryEngine { .map_err(|e| anyhow!("Failed to create temp table for HAVING: {}", e))?; // Evaluate HAVING expression - let mut evaluator = - ArithmeticEvaluator::with_date_notation(&temp_table, date_notation.clone()); + let mut evaluator = ArithmeticEvaluator::new(&temp_table); let having_result = evaluator.evaluate(having_expr, 0)?; // Skip this group if HAVING condition is not met diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 1c158712..97e6cebb 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -7,7 +7,6 @@ use std::time::{Duration, Instant}; use tracing::{debug, info}; use crate::config::config::BehaviorConfig; -use crate::config::global::get_date_notation; use crate::data::arithmetic_evaluator::ArithmeticEvaluator; use crate::data::data_view::DataView; use crate::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; @@ -246,7 +245,6 @@ fn edit_distance(a: &str, b: &str) -> usize { #[derive(Clone)] pub struct QueryEngine { case_insensitive: bool, - date_notation: String, _behavior_config: Option, } @@ -261,7 +259,6 @@ impl QueryEngine { pub fn new() -> Self { Self { case_insensitive: false, - date_notation: get_date_notation(), _behavior_config: None, } } @@ -269,11 +266,8 @@ impl QueryEngine { #[must_use] pub fn with_behavior_config(config: BehaviorConfig) -> Self { let case_insensitive = config.case_insensitive_default; - // Use get_date_notation() to respect environment variable override - let date_notation = get_date_notation(); Self { case_insensitive, - date_notation, _behavior_config: Some(config), } } @@ -282,7 +276,6 @@ impl QueryEngine { pub fn with_date_notation(_date_notation: String) -> Self { Self { case_insensitive: false, - date_notation: get_date_notation(), // Always use the global function _behavior_config: None, } } @@ -291,7 +284,6 @@ impl QueryEngine { pub fn with_case_insensitive(case_insensitive: bool) -> Self { Self { case_insensitive, - date_notation: get_date_notation(), _behavior_config: None, } } @@ -299,11 +291,10 @@ impl QueryEngine { #[must_use] pub fn with_case_insensitive_and_date_notation( case_insensitive: bool, - _date_notation: String, // Keep parameter for compatibility but use get_date_notation() + _date_notation: String, // Kept for compatibility; date parsing reads the global notation ) -> Self { Self { case_insensitive, - date_notation: get_date_notation(), // Always use the global function _behavior_config: None, } } @@ -1302,10 +1293,7 @@ impl QueryEngine { if let Some(generator) = registry.get(name) { // Evaluate arguments - let mut evaluator = ArithmeticEvaluator::with_date_notation( - &table, - self.date_notation.clone(), - ); + let mut evaluator = ArithmeticEvaluator::new(&table); let dummy_row = 0; let mut evaluated_args = Vec::new(); @@ -2320,9 +2308,8 @@ impl QueryEngine { // partition over the FILTERED set. Without this the evaluator sees the whole // source table and a WHERE clause has no effect on any window (P21): partition // counts, rank slots and frames all include rows the query excluded. - let mut evaluator = - ArithmeticEvaluator::with_date_notation(source_table, self.date_notation.clone()) - .with_visible_rows(view.visible_row_indices().to_vec()); + let mut evaluator = ArithmeticEvaluator::new(source_table) + .with_visible_rows(view.visible_row_indices().to_vec()); // Populate table aliases from exec_context if available if let Some(exec_ctx) = exec_context { @@ -2754,8 +2741,7 @@ impl QueryEngine { } // Process each input row - let mut evaluator = - ArithmeticEvaluator::with_date_notation(source_table, self.date_notation.clone()); + let mut evaluator = ArithmeticEvaluator::new(source_table); for &row_idx in visible_rows { // First pass: identify UNNEST expressions and collect their expansion arrays @@ -2932,9 +2918,7 @@ impl QueryEngine { // Create evaluator with visible rows from the view (for filtered aggregates) let visible_rows = view.visible_row_indices().to_vec(); - let mut evaluator = - ArithmeticEvaluator::with_date_notation(source_table, self.date_notation.clone()) - .with_visible_rows(visible_rows); + let mut evaluator = ArithmeticEvaluator::new(source_table).with_visible_rows(visible_rows); // Evaluate each aggregate expression once (they handle all rows internally) let mut row_values = Vec::new(); @@ -3300,7 +3284,6 @@ impl QueryEngine { select_items, having, self.case_insensitive, - self.date_notation.clone(), )?; // Add detailed phase information to the execution plan @@ -3351,6 +3334,7 @@ impl QueryEngine { // Sample first 1000 rows or 10% of data, whichever is smaller let sample_size = min(1000, row_count / 10).max(100); let mut seen = FxHashSet::default(); + let mut evaluator = ArithmeticEvaluator::new(view.source()); let visible_rows = view.get_visible_rows(); for (i, &row_idx) in visible_rows.iter().enumerate() { @@ -3361,7 +3345,6 @@ impl QueryEngine { // Evaluate GROUP BY expressions for this row let mut key_values = Vec::new(); for expr in group_by_exprs { - let mut evaluator = ArithmeticEvaluator::new(view.source()); let value = evaluator.evaluate(expr, row_idx).unwrap_or(DataValue::Null); key_values.push(value); } From cc37217dcba892cde32e00630c44b5810b5a6736 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Mon, 14 Sep 2026 20:31:22 +0100 Subject: [PATCH 3/5] test(r13): pin case-insensitive divergence between evaluators (R13 slice 3b) Under the engine's case-insensitive mode the WHERE evaluator folds case, but the value evaluator has no such mode, so the same predicate answers differently in SELECT, HAVING, aggregate arguments and CASE WHEN - and in WHERE too when the predicate sits under a CASE, which WHERE hands to an inner value evaluator. Extend the evaluator matrix with a case-insensitive run over the string column (10 predicates, expectations from DuckDB with NOCASE collation / ILIKE) and add clause-level cases through QueryEngine::with_case_insensitive(true). Today's answers are recorded as KNOWN, so the fix has to flip exactly these. Test-only; no engine change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha --- tests/evaluator_matrix_tests.rs | 202 ++++++++++++++++++++++++++++++-- 1 file changed, 193 insertions(+), 9 deletions(-) diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index 66b71608..79419052 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -23,10 +23,12 @@ use sql_cli::data::arithmetic_evaluator::ArithmeticEvaluator; use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use sql_cli::data::query_engine::QueryEngine; use sql_cli::data::recursive_where_evaluator::RecursiveWhereEvaluator; use sql_cli::data::trilean::Trilean; use sql_cli::sql::parser::ast::SelectItem; use sql_cli::sql::recursive_parser::Parser; +use std::sync::Arc; /// Rows chosen so every binary shape sees a non-NULL pair both ways round, NULL /// on the left, NULL on the right, and NULL on both. @@ -110,6 +112,23 @@ const EXPECTED: &[(&str, &str)] = &[ ("CASE WHEN a < b THEN true ELSE false END", "TFFFF"), ]; +/// The same shapes under the engine's case-insensitive mode +/// (`--case-insensitive`), over the string column. DuckDB has no such mode, so +/// these were generated with each string comparison collated `NOCASE` (and +/// `LIKE` as `ILIKE`), e.g. `s COLLATE NOCASE < 'B'`. +const EXPECTED_CASE_INSENSITIVE: &[(&str, &str)] = &[ + ("s = 'ABC'", "TFUTU"), + ("s <> 'ABC'", "FTUFU"), + ("s < 'B'", "TFUTU"), + ("s IN ('ABC', 'Q')", "TFUTU"), + ("s NOT IN ('XYZ')", "TFUTU"), + ("s BETWEEN 'ABC' AND 'M'", "TFUTU"), + ("s LIKE 'A%'", "TFUTU"), + ("NOT (s = 'ABC')", "FTUFU"), + ("CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", "TFFTF"), + ("UPPER(s) = 'abc'", "TFUTU"), +]; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum Evaluator { /// `RecursiveWhereEvaluator`, as `WHERE `. @@ -135,6 +154,79 @@ struct Known { // here, with its finding, rather than left failing. const KNOWN: &[Known] = &[]; +// Recorded by R13 slice 3b (2026-09-14). The WHERE evaluator honours the +// engine's case-insensitive mode; the value evaluator has no such mode and +// always compares case-sensitively, so under `--case-insensitive` a SELECT, +// HAVING or CASE WHEN answers differently from the same predicate in WHERE. +const KNOWN_CASE_INSENSITIVE: &[Known] = &[ + Known { + evaluator: Evaluator::Value, + predicate: "s = 'ABC'", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s <> 'ABC'", + observed: "TTUTU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s < 'B'", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s IN ('ABC', 'Q')", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s NOT IN ('XYZ')", + observed: "TTUTU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s BETWEEN 'ABC' AND 'M'", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "s LIKE 'A%'", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "NOT (s = 'ABC')", + observed: "TTUTU", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Where, + predicate: "CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", + observed: "FFFFF", + why: "WHERE hands CASE to an inner value evaluator built without the mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", + observed: "FFFFF", + why: "value evaluator has no case-insensitive mode", + }, + Known { + evaluator: Evaluator::Value, + predicate: "UPPER(s) = 'abc'", + observed: "FFUFU", + why: "value evaluator has no case-insensitive mode", + }, +]; + fn trilean_char(t: Trilean) -> char { match t { Trilean::True => 'T', @@ -143,13 +235,13 @@ fn trilean_char(t: Trilean) -> char { } } -fn observe_where(table: &DataTable, predicate: &str) -> String { +fn observe_where(table: &DataTable, predicate: &str, case_insensitive: bool) -> String { let sql = format!("SELECT * FROM t WHERE {predicate}"); let statement = Parser::new(&sql) .parse() .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::new(table); + let mut evaluator = RecursiveWhereEvaluator::with_case_insensitive(table, case_insensitive); (0..ROWS) .map(|row| match evaluator.evaluate(&where_clause, row) { Ok(t) => trilean_char(t), @@ -158,7 +250,7 @@ fn observe_where(table: &DataTable, predicate: &str) -> String { .collect() } -fn observe_value(table: &DataTable, predicate: &str) -> String { +fn observe_value(table: &DataTable, predicate: &str, case_insensitive: bool) -> String { // Parenthesised, because a bare `SELECT a AND b` does not parse (logged as // a parity gap of its own); the parentheses do not change the expression. let sql = format!("SELECT ({predicate}) AS v FROM t"); @@ -169,6 +261,8 @@ fn observe_value(table: &DataTable, predicate: &str) -> String { Some(SelectItem::Expression { expr, .. }) => expr.clone(), other => panic!("expected one expression select item for {sql}, got {other:?}"), }; + // No case-insensitive mode to ask for: see KNOWN_CASE_INSENSITIVE. + let _ = case_insensitive; let mut evaluator = ArithmeticEvaluator::new(table); (0..ROWS) .map(|row| match evaluator.evaluate(&expr, row) { @@ -183,13 +277,22 @@ fn observe_value(table: &DataTable, predicate: &str) -> String { #[test] fn every_predicate_through_both_evaluators() { + check_matrix(EXPECTED, KNOWN, false); +} + +#[test] +fn every_predicate_through_both_evaluators_case_insensitive() { + check_matrix(EXPECTED_CASE_INSENSITIVE, KNOWN_CASE_INSENSITIVE, true); +} + +fn check_matrix(expected_answers: &[(&str, &str)], known: &[Known], case_insensitive: bool) { let t = table(); let mut problems = Vec::new(); let mut matrix = Vec::new(); - for &(predicate, expected) in EXPECTED { - let where_got = observe_where(&t, predicate); - let value_got = observe_value(&t, predicate); + for &(predicate, expected) in expected_answers { + 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}" )); @@ -198,7 +301,7 @@ fn every_predicate_through_both_evaluators() { (Evaluator::Where, &where_got), (Evaluator::Value, &value_got), ] { - let known = KNOWN + let known = known .iter() .find(|k| k.evaluator == evaluator && k.predicate == predicate); match known { @@ -222,8 +325,8 @@ fn every_predicate_through_both_evaluators() { } // Every KNOWN entry must name a predicate that is still in the matrix. - for k in KNOWN { - if !EXPECTED.iter().any(|(p, _)| *p == k.predicate) { + for k in known { + if !expected_answers.iter().any(|(p, _)| *p == k.predicate) { problems.push(format!( "ORPHAN {:?} `{}` is in KNOWN but not in EXPECTED", k.evaluator, k.predicate @@ -238,3 +341,84 @@ fn every_predicate_through_both_evaluators() { matrix.join("\n") ); } + +/// Clause-level forms of the case-insensitive rows, run through +/// `QueryEngine::with_case_insensitive(true)`. The matrix above builds each +/// evaluator directly; these catch the engine's own construction sites (SELECT, +/// HAVING, aggregate arguments, WHERE operands), which is where the mode has to +/// be handed in. `(sql, DuckDB answer with s collated NOCASE, what we return +/// today if it differs)`. Rows are `|`-separated, cells `,`-separated. +const CLAUSES_CASE_INSENSITIVE: &[(&str, &str, Option<&str>)] = &[ + ( + "SELECT (s = 'ABC') AS v FROM t", + "true|false|NULL|true|NULL", + Some("false|false|NULL|false|NULL"), + ), + ( + "SELECT s, COUNT(*) AS n FROM t GROUP BY s HAVING s = 'ABC'", + "abc,2", + Some(""), + ), + ( + // Not COUNT(*): an empty ungrouped aggregate returns no row (P14). + "SELECT a FROM t WHERE CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", + "1|1", + Some(""), + ), + ( + "SELECT SUM(CASE WHEN s = 'ABC' THEN 1 ELSE 0 END) AS n FROM t", + "2", + Some("0"), + ), + ( + "SELECT a, SUM(CASE WHEN s = 'ABC' THEN 1 ELSE 0 END) AS n FROM t \ + WHERE a IS NOT NULL GROUP BY a ORDER BY a", + "1,2|2,0", + Some("1,0|2,0"), + ), +]; + +fn render(value: &DataValue) -> String { + match value { + DataValue::Null => "NULL".to_string(), + DataValue::Boolean(b) => b.to_string(), + DataValue::Integer(i) => i.to_string(), + DataValue::Float(f) => f.to_string(), + DataValue::String(s) => s.clone(), + other => format!("{other:?}"), + } +} + +#[test] +fn clause_level_case_insensitive() { + let engine = QueryEngine::with_case_insensitive(true); + let t = Arc::new(table()); + let mut problems = Vec::new(); + + for &(sql, expected, observed) in CLAUSES_CASE_INSENSITIVE { + let got = match engine.execute(Arc::clone(&t), sql) { + Ok(view) => (0..view.row_count()) + .map(|i| { + let row = view.get_row(i).expect("row in range"); + row.values.iter().map(render).collect::>().join(",") + }) + .collect::>() + .join("|"), + Err(e) => format!("ERROR: {e}"), + }; + match observed { + None if got != expected => problems.push(format!( + "UNRECORDED `{sql}`: expected {expected}, got {got}" + )), + Some(_) if got == expected => problems.push(format!( + "FIXED `{sql}` now agrees ({expected}); drop its observed answer" + )), + Some(o) if got != o => problems.push(format!( + "CHANGED `{sql}`: recorded {o}, now {got} (expected {expected})" + )), + _ => {} + } + } + + assert!(problems.is_empty(), "\n{}\n", problems.join("\n")); +} From 9fc3a7304288b0404da9df05522727aa481ef2e0 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Mon, 14 Sep 2026 20:37:54 +0100 Subject: [PATCH 4/5] fix(r13): honour case-insensitive mode in the value evaluator (R13 slice 3b) Hand the engine's case-insensitive mode to every ArithmeticEvaluator construction site through a new with_case_insensitive builder, and use it in comparisons, IN, BETWEEN and LIKE. Under --case-insensitive a predicate in SELECT, HAVING, aggregate arguments or CASE WHEN now answers as it does in WHERE; so does WHERE itself when the predicate sits under a CASE, which it evaluates through its inner value evaluator. Flips exactly what the previous commit pinned: all 11 matrix entries and all 5 clause-level cases. Parity report byte-identical (the harness runs case-sensitive); cargo test, Python and FORMAL examples green. Not covered, recorded in R13: simple CASE x WHEN (own values_equal) and method calls like .Contains() still compare case-sensitively outside WHERE. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha --- src/data/arithmetic_evaluator.rs | 36 +++++++--- src/data/group_by_expressions.rs | 14 ++-- src/data/hash_join.rs | 18 +++-- src/data/query_engine.rs | 20 ++++-- src/data/recursive_where_evaluator.rs | 6 +- tests/evaluator_matrix_tests.rs | 95 ++++----------------------- 6 files changed, 82 insertions(+), 107 deletions(-) diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 3fa8d3ea..77f068cf 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -46,6 +46,7 @@ pub struct ArithmeticEvaluator<'a> { visible_rows: Option>, // For aggregate functions on filtered views window_contexts: HashMap>, // Cache window contexts by hash table_aliases: HashMap, // Map alias -> table name for qualified columns + case_insensitive: bool, // The engine's string-comparison mode, as WHERE honours it } impl<'a> ArithmeticEvaluator<'a> { @@ -61,6 +62,7 @@ impl<'a> ArithmeticEvaluator<'a> { visible_rows: None, window_contexts: HashMap::new(), table_aliases: HashMap::new(), + case_insensitive: false, } } @@ -71,6 +73,14 @@ impl<'a> ArithmeticEvaluator<'a> { self } + /// Compare strings case-insensitively, as the engine's case-insensitive + /// mode does for WHERE: comparisons, IN, BETWEEN and LIKE. + #[must_use] + pub fn with_case_insensitive(mut self, case_insensitive: bool) -> Self { + self.case_insensitive = case_insensitive; + self + } + /// Set table aliases for qualified column resolution #[must_use] pub fn with_table_aliases(mut self, aliases: HashMap) -> Self { @@ -150,8 +160,8 @@ impl<'a> ArithmeticEvaluator<'a> { let val = self.evaluate(expr, row_index)?; let lo = self.evaluate(lower, row_index)?; let hi = self.evaluate(upper, row_index)?; - let ge = compare_trilean(&val, &lo, ">="); - let le = compare_trilean(&val, &hi, "<="); + let ge = compare_trilean(&val, &lo, ">=", self.case_insensitive); + let le = compare_trilean(&val, &hi, "<=", self.case_insensitive); Ok(ge.and(le).to_value()) } // Logical negation as a value-producing expression. Reached, e.g., by @@ -369,7 +379,7 @@ impl<'a> ArithmeticEvaluator<'a> { // the clause that filters on the result collapses it (R13). See // tests/evaluator_matrix_tests.rs for the per-operator contract. ">" | "<" | ">=" | "<=" | "=" | "!=" | "<>" => { - Ok(compare_trilean(&left_val, &right_val, op).to_value()) + Ok(compare_trilean(&left_val, &right_val, op, self.case_insensitive).to_value()) } // IS NULL / IS NOT NULL are the sanctioned NULL tests, so two-valued. "IS NULL" => Ok(DataValue::Boolean(matches!(left_val, DataValue::Null))), @@ -381,8 +391,12 @@ impl<'a> ArithmeticEvaluator<'a> { if matches!(left_val, DataValue::Null) || matches!(right_val, DataValue::Null) { return Ok(DataValue::Null); } - let text = self.value_to_string(&left_val); - let pattern = self.value_to_string(&right_val); + let mut text = self.value_to_string(&left_val); + let mut pattern = self.value_to_string(&right_val); + if self.case_insensitive { + text = text.to_lowercase(); + pattern = pattern.to_lowercase(); + } let matches = self.sql_like_match(&text, &pattern); Ok(DataValue::Boolean(matches)) } @@ -1529,7 +1543,7 @@ impl<'a> ArithmeticEvaluator<'a> { let mut result = Trilean::False; for v in values { let item = self.evaluate(v, row_index)?; - result = result.or(compare_trilean(&val, &item, "=")); + result = result.or(compare_trilean(&val, &item, "=", self.case_insensitive)); if result.is_true() { break; } @@ -1649,12 +1663,16 @@ impl<'a> ArithmeticEvaluator<'a> { /// for predicates is what made `SELECT x = NULL` TRUE for NULL rows (P48, the /// value-evaluator twin of P18). Same rule as the WHERE evaluator's /// `compare_trilean`, which R13 slice 4 will make the only copy. -fn compare_trilean(left: &DataValue, right: &DataValue, op: &str) -> Trilean { +fn compare_trilean( + left: &DataValue, + right: &DataValue, + op: &str, + case_insensitive: bool, +) -> Trilean { if matches!(left, DataValue::Null) || matches!(right, DataValue::Null) { return Trilean::Unknown; } - // Case sensitivity is not yet threaded into this evaluator - R13 slice 3. - Trilean::from_bool(compare_with_op(left, right, op, false)) + Trilean::from_bool(compare_with_op(left, right, op, case_insensitive)) } /// The truth value of an operand of AND / OR / NOT. A value with no truth value diff --git a/src/data/group_by_expressions.rs b/src/data/group_by_expressions.rs index c76f2f09..90146681 100644 --- a/src/data/group_by_expressions.rs +++ b/src/data/group_by_expressions.rs @@ -49,7 +49,7 @@ pub trait GroupByExpressions { group_by_exprs: &[SqlExpression], select_items: &[SelectItem], having: Option<&SqlExpression>, - _case_insensitive: bool, + case_insensitive: bool, ) -> Result<(DataView, GroupByPhaseInfo)>; } @@ -81,7 +81,8 @@ impl GroupByExpressions for QueryEngine { debug!("GROUP BY Phase 2 starting: processing {} rows", total_rows); // OPTIMIZATION: Create evaluator once outside the loop! - let mut evaluator = ArithmeticEvaluator::new(view.source()); + let mut evaluator = ArithmeticEvaluator::new(view.source()) + .with_case_insensitive(self.is_case_insensitive()); // OPTIMIZATION: Pre-allocate key_values vector with the right capacity let mut key_values = Vec::with_capacity(group_by_exprs.len()); @@ -131,7 +132,7 @@ impl GroupByExpressions for QueryEngine { group_by_exprs: &[SqlExpression], select_items: &[SelectItem], having: Option<&SqlExpression>, - _case_insensitive: bool, + case_insensitive: bool, ) -> Result<(DataView, GroupByPhaseInfo)> { use std::time::Instant; let start = Instant::now(); @@ -281,6 +282,7 @@ impl GroupByExpressions for QueryEngine { for (expr, _col_name) in &aggregate_columns { let group_rows = group_view.get_visible_rows(); let mut evaluator = ArithmeticEvaluator::new(group_view.source()) + .with_case_insensitive(case_insensitive) .with_visible_rows(group_rows.clone()); let value = if group_view.row_count() > 0 && !group_rows.is_empty() { @@ -300,7 +302,8 @@ impl GroupByExpressions for QueryEngine { for (expr, _alias) in &derived_grouped_exprs { let group_rows = group_view.get_visible_rows(); let value = if !group_rows.is_empty() { - let mut evaluator = ArithmeticEvaluator::new(group_view.source()); + let mut evaluator = ArithmeticEvaluator::new(group_view.source()) + .with_case_insensitive(case_insensitive); evaluator .evaluate(expr, group_rows[0]) .unwrap_or(DataValue::Null) @@ -333,7 +336,8 @@ impl GroupByExpressions for QueryEngine { .map_err(|e| anyhow!("Failed to create temp table for HAVING: {}", e))?; // Evaluate HAVING expression - let mut evaluator = ArithmeticEvaluator::new(&temp_table); + let mut evaluator = + ArithmeticEvaluator::new(&temp_table).with_case_insensitive(case_insensitive); let having_result = evaluator.evaluate(having_expr, 0)?; // Skip this group if HAVING condition is not met diff --git a/src/data/hash_join.rs b/src/data/hash_join.rs index da5d2b5f..787f0ebe 100644 --- a/src/data/hash_join.rs +++ b/src/data/hash_join.rs @@ -1089,8 +1089,10 @@ impl HashJoinExecutor { } // Create evaluators for both sides - let mut left_evaluator = ArithmeticEvaluator::new(&left_table); - let mut right_evaluator = ArithmeticEvaluator::new(&right_table); + let mut left_evaluator = + ArithmeticEvaluator::new(&left_table).with_case_insensitive(self.case_insensitive); + let mut right_evaluator = + ArithmeticEvaluator::new(&right_table).with_case_insensitive(self.case_insensitive); // Nested loop join with multiple conditions let mut match_count = 0; @@ -1242,8 +1244,10 @@ impl HashJoinExecutor { } // Create evaluators for both sides - let mut left_evaluator = ArithmeticEvaluator::new(&left_table); - let mut right_evaluator = ArithmeticEvaluator::new(&right_table); + let mut left_evaluator = + ArithmeticEvaluator::new(&left_table).with_case_insensitive(self.case_insensitive); + let mut right_evaluator = + ArithmeticEvaluator::new(&right_table).with_case_insensitive(self.case_insensitive); // Nested loop join with multiple conditions let mut match_count = 0; @@ -1429,8 +1433,10 @@ impl HashJoinExecutor { // Create evaluators for both sides. The join-alias (joined) table is the // "right" evaluator, so alias-qualified operands route correctly (P7). - let mut from_evaluator = ArithmeticEvaluator::new(&from_table); - let mut joined_evaluator = ArithmeticEvaluator::new(&joined_table); + let mut from_evaluator = + ArithmeticEvaluator::new(&from_table).with_case_insensitive(self.case_insensitive); + let mut joined_evaluator = + ArithmeticEvaluator::new(&joined_table).with_case_insensitive(self.case_insensitive); // Outer loop over the joined table so every joined row is kept in order. let mut match_count = 0; diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 97e6cebb..f389d2d0 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -299,6 +299,12 @@ impl QueryEngine { } } + /// Whether string comparisons ignore case (`--case-insensitive`). + #[must_use] + pub fn is_case_insensitive(&self) -> bool { + self.case_insensitive + } + /// Find a column name similar to the given name using edit distance fn find_similar_column(&self, table: &DataTable, name: &str) -> Option { let columns = table.column_names(); @@ -1293,7 +1299,8 @@ impl QueryEngine { if let Some(generator) = registry.get(name) { // Evaluate arguments - let mut evaluator = ArithmeticEvaluator::new(&table); + let mut evaluator = ArithmeticEvaluator::new(&table) + .with_case_insensitive(self.case_insensitive); let dummy_row = 0; let mut evaluated_args = Vec::new(); @@ -2309,6 +2316,7 @@ impl QueryEngine { // source table and a WHERE clause has no effect on any window (P21): partition // counts, rank slots and frames all include rows the query excluded. let mut evaluator = ArithmeticEvaluator::new(source_table) + .with_case_insensitive(self.case_insensitive) .with_visible_rows(view.visible_row_indices().to_vec()); // Populate table aliases from exec_context if available @@ -2741,7 +2749,8 @@ impl QueryEngine { } // Process each input row - let mut evaluator = ArithmeticEvaluator::new(source_table); + let mut evaluator = + ArithmeticEvaluator::new(source_table).with_case_insensitive(self.case_insensitive); for &row_idx in visible_rows { // First pass: identify UNNEST expressions and collect their expansion arrays @@ -2918,7 +2927,9 @@ impl QueryEngine { // Create evaluator with visible rows from the view (for filtered aggregates) let visible_rows = view.visible_row_indices().to_vec(); - let mut evaluator = ArithmeticEvaluator::new(source_table).with_visible_rows(visible_rows); + let mut evaluator = ArithmeticEvaluator::new(source_table) + .with_case_insensitive(self.case_insensitive) + .with_visible_rows(visible_rows); // Evaluate each aggregate expression once (they handle all rows internally) let mut row_values = Vec::new(); @@ -3334,7 +3345,8 @@ impl QueryEngine { // Sample first 1000 rows or 10% of data, whichever is smaller let sample_size = min(1000, row_count / 10).max(100); let mut seen = FxHashSet::default(); - let mut evaluator = ArithmeticEvaluator::new(view.source()); + let mut evaluator = + ArithmeticEvaluator::new(view.source()).with_case_insensitive(self.case_insensitive); let visible_rows = view.get_visible_rows(); for (i, &row_idx) in visible_rows.iter().enumerate() { diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index 09871e11..34bf016e 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -817,8 +817,10 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { /// `ArithmeticEvaluator` (see the `arithmetic` field for why it is shared). fn evaluate_arithmetic(&self, expr: &SqlExpression, row_index: usize) -> Result { let mut slot = self.arithmetic.borrow_mut(); - slot.get_or_insert_with(|| ArithmeticEvaluator::new(self.table)) - .evaluate(expr, row_index) + slot.get_or_insert_with(|| { + ArithmeticEvaluator::new(self.table).with_case_insensitive(self.case_insensitive) + }) + .evaluate(expr, row_index) } fn evaluate_in_list( diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index 79419052..fc804bfd 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -154,78 +154,12 @@ struct Known { // here, with its finding, rather than left failing. const KNOWN: &[Known] = &[]; -// Recorded by R13 slice 3b (2026-09-14). The WHERE evaluator honours the -// engine's case-insensitive mode; the value evaluator has no such mode and -// always compares case-sensitively, so under `--case-insensitive` a SELECT, -// HAVING or CASE WHEN answers differently from the same predicate in WHERE. -const KNOWN_CASE_INSENSITIVE: &[Known] = &[ - Known { - evaluator: Evaluator::Value, - predicate: "s = 'ABC'", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s <> 'ABC'", - observed: "TTUTU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s < 'B'", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s IN ('ABC', 'Q')", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s NOT IN ('XYZ')", - observed: "TTUTU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s BETWEEN 'ABC' AND 'M'", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "s LIKE 'A%'", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "NOT (s = 'ABC')", - observed: "TTUTU", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Where, - predicate: "CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", - observed: "FFFFF", - why: "WHERE hands CASE to an inner value evaluator built without the mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", - observed: "FFFFF", - why: "value evaluator has no case-insensitive mode", - }, - Known { - evaluator: Evaluator::Value, - predicate: "UPPER(s) = 'abc'", - observed: "FFUFU", - why: "value evaluator has no case-insensitive mode", - }, -]; +// Empty since R13 slice 3b (2026-09-14). Slice 3b recorded 11 divergences: +// the value evaluator had no case-insensitive mode, so every string predicate +// here compared case-sensitively outside WHERE - and inside WHERE under a CASE, +// which WHERE hands to an inner value evaluator. The mode is now handed in at +// every construction site, and all 11 went FIXED in one change. +const KNOWN_CASE_INSENSITIVE: &[Known] = &[]; fn trilean_char(t: Trilean) -> char { match t { @@ -261,9 +195,7 @@ fn observe_value(table: &DataTable, predicate: &str, case_insensitive: bool) -> Some(SelectItem::Expression { expr, .. }) => expr.clone(), other => panic!("expected one expression select item for {sql}, got {other:?}"), }; - // No case-insensitive mode to ask for: see KNOWN_CASE_INSENSITIVE. - let _ = case_insensitive; - let mut evaluator = ArithmeticEvaluator::new(table); + let mut evaluator = ArithmeticEvaluator::new(table).with_case_insensitive(case_insensitive); (0..ROWS) .map(|row| match evaluator.evaluate(&expr, row) { Ok(DataValue::Boolean(true)) => 'T', @@ -347,34 +279,35 @@ fn check_matrix(expected_answers: &[(&str, &str)], known: &[Known], case_insensi /// evaluator directly; these catch the engine's own construction sites (SELECT, /// HAVING, aggregate arguments, WHERE operands), which is where the mode has to /// be handed in. `(sql, DuckDB answer with s collated NOCASE, what we return -/// today if it differs)`. Rows are `|`-separated, cells `,`-separated. +/// if it differs)` - all agree since slice 3b. Rows are `|`-separated, cells +/// `,`-separated. const CLAUSES_CASE_INSENSITIVE: &[(&str, &str, Option<&str>)] = &[ ( "SELECT (s = 'ABC') AS v FROM t", "true|false|NULL|true|NULL", - Some("false|false|NULL|false|NULL"), + None, ), ( "SELECT s, COUNT(*) AS n FROM t GROUP BY s HAVING s = 'ABC'", "abc,2", - Some(""), + None, ), ( // Not COUNT(*): an empty ungrouped aggregate returns no row (P14). "SELECT a FROM t WHERE CASE WHEN s = 'ABC' THEN 1 ELSE 0 END = 1", "1|1", - Some(""), + None, ), ( "SELECT SUM(CASE WHEN s = 'ABC' THEN 1 ELSE 0 END) AS n FROM t", "2", - Some("0"), + None, ), ( "SELECT a, SUM(CASE WHEN s = 'ABC' THEN 1 ELSE 0 END) AS n FROM t \ WHERE a IS NOT NULL GROUP BY a ORDER BY a", "1,2|2,0", - Some("1,0|2,0"), + None, ), ]; From f59592307f4d43b7655c45ce6226fd30b010a9bd Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Mon, 14 Sep 2026 20:37:55 +0100 Subject: [PATCH 5/5] docs(r13): record slice 3 and 3b Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha --- docs/ENGINE_REFACTORING.md | 39 +++++++++++++++++++++++++++++++++++--- docs/SQL_PARITY.md | 2 +- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 79fe77d6..a6f6d075 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -472,7 +472,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 next. + **slices 1 and 2 done 2026-09-13, slice 3 (with 3b) done 2026-09-14**, slice 4 + next. **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 → @@ -565,6 +566,37 @@ feature work**, and so we can tell the difference between "this is awkward" and (both NULL → not-true, already correct), so nothing downstream had to change. **Two evaluators now agree on all 36 predicates, but there are still two** — slices 3–6 are what make that agreement structural rather than coincidental. +- **Slice 3 result (2026-09-14).** Two no-op commits, then a pinned semantic + one (3b), each checked against a byte-identical parity report. + - *Registries built once.* The function, both aggregate and the window + registries are immutable after construction, so a process-wide `OnceLock` + builds them and every `ArithmeticEvaluator` takes `Arc` clones. GROUP BY was + paying for it per group and per aggregate: over `trades_100000.csv`, net of + ~3 s load, a high-cardinality GROUP BY went 3.4 s → 1.9 s and GROUP BY + + HAVING 2.5 s → 0.8 s. `estimate_group_cardinality` also built one per row. + - *One constructor.* `new` / `with_date_notation` / + `with_date_notation_and_registry` differed only by a date-notation string + stored in a field **nothing read** — dates parse through the global + notation. So "pass date notation in" turned out to be "delete it": the + field, the constructors, `apply_group_by_expressions`' parameter and + `QueryEngine`'s own unread field went. Table aliases needed nothing: + alias-qualified operands already resolve in every clause probed. + - *3b — case-insensitivity.* The one context gap with a visible answer: under + `--case-insensitive`, `WHERE status = 'ACTIVE'` matched `Active` but + `SELECT status = 'ACTIVE'`, HAVING, aggregate arguments and CASE WHEN did + not — nor WHERE itself once the predicate sat under a CASE, which WHERE + hands to its inner value evaluator. Slice 1 had not pinned it (the parity + harness cannot: DuckDB has no such mode), so 3b pinned first — a + case-insensitive run of the matrix (10 predicates, expectations from DuckDB + with `COLLATE NOCASE` / `ILIKE`) plus five clause-level cases through + `QueryEngine::with_case_insensitive(true)` — recording 11 + 5 divergences. + The fix, `ArithmeticEvaluator::with_case_insensitive` handed in at all 16 + construction sites (comparisons, `IN`, `BETWEEN`, `LIKE`), flipped exactly + those and nothing else; parity report byte-identical. + - *Still case-sensitive outside WHERE, not pinned:* simple + `CASE x WHEN 'v'` (its own `values_equal`, with its own numeric rules — goes + with slice 4) and method calls such as `.Contains()` routed to registry + functions (slice 5). - **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 @@ -579,7 +611,7 @@ feature work**, and so we can tell the difference between "this is awkward" and 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 + 3. ✅ **One construction path.** *(Done 2026-09-14 — see result above.)* 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. @@ -630,7 +662,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) and 2 (3VL) DONE → 3 (construction) → 4–6 (retire WHERE arms) +R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE → 4–6 (retire WHERE arms) ``` **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator @@ -670,3 +702,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 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) | — | | 2026-09-13 | R13 slice 1: evaluator matrix (36 predicates × both evaluators × 5 NULL-bearing rows, DuckDB-derived expectations). WHERE 36/36, value evaluator 4/36. P48 widened; P50–P52 filed; six corpus cases, AGREE unchanged | — | | 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 | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 654d1506..269e47e4 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -92,7 +92,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 (one construction path) 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 (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 | | 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 |