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 | diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 24492166..77f068cf 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; @@ -12,15 +11,34 @@ 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> { table: &'a DataTable, - _date_notation: String, function_registry: Arc, aggregate_registry: Arc, // Old registry (being phased out) new_aggregate_registry: Arc, // New registry @@ -28,36 +46,23 @@ 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> { #[must_use] pub fn new(table: &'a DataTable) -> Self { + let registries = shared_registries(); 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(), - } - } - - #[must_use] - pub fn with_date_notation(table: &'a DataTable, date_notation: String) -> Self { - 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(), + case_insensitive: false, } } @@ -68,30 +73,19 @@ impl<'a> ArithmeticEvaluator<'a> { self } - /// Set table aliases for qualified column resolution + /// Compare strings case-insensitively, as the engine's case-insensitive + /// mode does for WHERE: comparisons, IN, BETWEEN and LIKE. #[must_use] - pub fn with_table_aliases(mut self, aliases: HashMap) -> Self { - self.table_aliases = aliases; + 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_date_notation_and_registry( - table: &'a DataTable, - date_notation: String, - 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(), - } + pub fn with_table_aliases(mut self, aliases: HashMap) -> Self { + self.table_aliases = aliases; + self } /// Find a column name similar to the given name using edit distance @@ -166,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 @@ -385,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))), @@ -397,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)) } @@ -1545,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; } @@ -1665,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 89fda2b4..90146681 100644 --- a/src/data/group_by_expressions.rs +++ b/src/data/group_by_expressions.rs @@ -49,8 +49,7 @@ pub trait GroupByExpressions { group_by_exprs: &[SqlExpression], select_items: &[SelectItem], having: Option<&SqlExpression>, - _case_insensitive: bool, - date_notation: String, + case_insensitive: bool, ) -> Result<(DataView, GroupByPhaseInfo)>; } @@ -82,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()); @@ -132,8 +132,7 @@ impl GroupByExpressions for QueryEngine { group_by_exprs: &[SqlExpression], select_items: &[SelectItem], having: Option<&SqlExpression>, - _case_insensitive: bool, - date_notation: String, + case_insensitive: bool, ) -> Result<(DataView, GroupByPhaseInfo)> { use std::time::Instant; let start = Instant::now(); @@ -282,11 +281,9 @@ 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_case_insensitive(case_insensitive) + .with_visible_rows(group_rows.clone()); let value = if group_view.row_count() > 0 && !group_rows.is_empty() { evaluator @@ -305,10 +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::with_date_notation( - group_view.source(), - date_notation.clone(), - ); + let mut evaluator = ArithmeticEvaluator::new(group_view.source()) + .with_case_insensitive(case_insensitive); evaluator .evaluate(expr, group_rows[0]) .unwrap_or(DataValue::Null) @@ -342,7 +337,7 @@ impl GroupByExpressions for QueryEngine { // Evaluate HAVING expression let mut evaluator = - ArithmeticEvaluator::with_date_notation(&temp_table, date_notation.clone()); + 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 1c158712..f389d2d0 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,15 +291,20 @@ 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, } } + /// 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(); @@ -1302,10 +1299,8 @@ 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) + .with_case_insensitive(self.case_insensitive); let dummy_row = 0; let mut evaluated_args = Vec::new(); @@ -2320,9 +2315,9 @@ 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_case_insensitive(self.case_insensitive) + .with_visible_rows(view.visible_row_indices().to_vec()); // Populate table aliases from exec_context if available if let Some(exec_ctx) = exec_context { @@ -2755,7 +2750,7 @@ impl QueryEngine { // Process each input row let mut evaluator = - ArithmeticEvaluator::with_date_notation(source_table, self.date_notation.clone()); + 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 @@ -2932,9 +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::with_date_notation(source_table, self.date_notation.clone()) - .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(); @@ -3300,7 +3295,6 @@ impl QueryEngine { select_items, having, self.case_insensitive, - self.date_notation.clone(), )?; // Add detailed phase information to the execution plan @@ -3351,6 +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()).with_case_insensitive(self.case_insensitive); let visible_rows = view.get_visible_rows(); for (i, &row_idx) in visible_rows.iter().enumerate() { @@ -3361,7 +3357,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); } 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 66b71608..fc804bfd 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,13 @@ struct Known { // here, with its finding, rather than left failing. const KNOWN: &[Known] = &[]; +// 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 { Trilean::True => 'T', @@ -143,13 +169,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 +184,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,7 +195,7 @@ 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:?}"), }; - 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', @@ -183,13 +209,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 +233,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 +257,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 +273,85 @@ 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 +/// 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", + None, + ), + ( + "SELECT s, COUNT(*) AS n FROM t GROUP BY s HAVING s = 'ABC'", + "abc,2", + 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", + None, + ), + ( + "SELECT SUM(CASE WHEN s = 'ABC' THEN 1 ELSE 0 END) AS n FROM t", + "2", + 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", + None, + ), +]; + +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")); +}