From 6e69d3628b48ece19cdd70a44b79bb6d7b1c6b7b Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 20:45:57 +0100 Subject: [PATCH 1/6] test(r13): pin window-operand guard and alias resolution for BETWEEN / IN (R13 slice 4) Before WHERE's BETWEEN and IN arms delegate to the value evaluator: - the P15 raw-window-operand error in every BETWEEN / IN / NOT IN operand position, which the value evaluator would otherwise evaluate quietly; - alias-qualified operands over two columns sharing a name. Latent (join output disambiguates before WHERE, so not reachable from SQL as probed), but WHERE's column fast path throws the resolved index away and looks the bare name up again, and the inner value evaluator has no aliases. Five divergences recorded; the next commit is expected to flip them. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/recursive_where_evaluator.rs | 79 +++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index 34bf016e..a05f5d94 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -1651,4 +1651,83 @@ mod operand_resolution_tests { .expect_err("a raw window operand must not evaluate quietly"); assert!(err.to_string().contains("ROW_NUMBER"), "got: {err}"); } + + #[test] + fn a_raw_window_operand_errors_in_between_and_in_too() { + // Same P15 rule in every operand position, so it has to survive these + // arms delegating to the value evaluator (R13 slice 4), which would + // otherwise evaluate the window function over the unfiltered rows. + let t = pairs(); + for predicate in [ + "ROW_NUMBER() OVER (ORDER BY a) BETWEEN 1 AND 2", + "a BETWEEN ROW_NUMBER() OVER (ORDER BY a) AND 10", + "ROW_NUMBER() OVER (ORDER BY a) IN (1, 2)", + "a IN (ROW_NUMBER() OVER (ORDER BY a), 2)", + "ROW_NUMBER() OVER (ORDER BY a) NOT IN (1, 2)", + ] { + let err = try_eval(&t, predicate, 0) + .expect_err("a raw window operand must not evaluate quietly"); + assert!( + err.to_string().contains("ROW_NUMBER"), + "{predicate}: got {err}" + ); + } + } + + /// Two source tables joined, both with a column `a`, qualified by table + /// name: row 0 = (orders.a 1, items.a 7), row 1 = (2, 9). + fn joined() -> DataTable { + let mut table = DataTable::new("joined"); + table.add_column(DataColumn::new("a").with_qualified_name("orders")); + table.add_column(DataColumn::new("a").with_qualified_name("items")); + for (o, i) in [(1, 7), (2, 9)] { + table + .add_row(DataRow::new(vec![ + DataValue::Integer(o), + DataValue::Integer(i), + ])) + .unwrap(); + } + table + } + + #[test] + fn alias_qualified_operands_resolve_through_the_execution_context() { + // `i` is an alias for `items`. The unqualified fallback would find + // `orders.a` first, so a wrong resolution shows as a wrong answer. + // + // Latent, not reachable from SQL as probed: join output already + // disambiguates duplicate names before WHERE sees them. But WHERE's + // column fast path resolves the alias to an index and then looks the + // bare name up again, and the inner value evaluator is given no + // aliases at all -- so both answer from `orders.a` here. + let t = joined(); + let mut ctx = ExecutionContext::new(); + ctx.register_alias("i".to_string(), "items".to_string()); + let eval = |predicate: &str, row: usize| { + let sql = format!("SELECT * FROM joined WHERE {predicate}"); + let statement = Parser::new(&sql).parse().expect("failed to parse"); + let where_clause = statement.where_clause.expect("expected a WHERE clause"); + RecursiveWhereEvaluator::with_exec_context(&t, &ctx, false) + .evaluate(&where_clause, row) + .expect("evaluation failed") + }; + // (predicate, correct answer for row 0, answer today if it differs) + for (predicate, correct, known) in [ + ("i.a = 7", Trilean::True, Some(Trilean::False)), + ("i.a BETWEEN 5 AND 8", Trilean::True, Some(Trilean::False)), + ("i.a IN (7, 99)", Trilean::True, Some(Trilean::False)), + ("i.a NOT IN (7, 99)", Trilean::False, Some(Trilean::True)), + ("i.a + 0 = 7", Trilean::True, Some(Trilean::False)), + ] { + let observed = eval(predicate, 0); + match known { + Some(today) => assert_eq!( + observed, today, + "{predicate}: recorded divergence changed -- if fixed, drop it" + ), + None => assert_eq!(observed, correct, "{predicate}"), + } + } + } } From 8fdf68b2ff2d2c8408cdbbe95f987924ee7dee49 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 20:50:36 +0100 Subject: [PATCH 2/6] fix(r13): WHERE column operands keep their resolved index; inner evaluator gets aliases (R13 slice 4) WHERE's column fast path resolved an alias-qualified operand to an index through the ExecutionContext, turned it back into a bare name, and looked that up again - finding the first column of the name, not the one the qualifier picked. The value evaluator WHERE hands expression operands to was built with no aliases, so `i.a + 0` had the same fault from the other side. Now the index is used directly, and the inner evaluator is given the context's aliases, so a column means the same thing whichever of the two resolves it - the precondition for BETWEEN / IN delegating. Latent (join output disambiguates duplicate names first); flips the five recorded divergences. Parity report byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/recursive_where_evaluator.rs | 89 +++++++++++++-------------- 1 file changed, 43 insertions(+), 46 deletions(-) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index a05f5d94..e80b0942 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -4,6 +4,7 @@ use crate::data::evaluation_context::EvaluationContext; use crate::data::query_engine::ExecutionContext; use crate::data::trilean::Trilean; use crate::data::value_comparisons::compare_with_op; +use crate::sql::parser::ast::ColumnRef; use crate::sql::recursive_parser::{Condition, LogicalOp, SqlExpression, WhereClause}; use anyhow::{anyhow, Result}; use chrono::{Local, NaiveDate, NaiveDateTime, NaiveTime}; @@ -748,19 +749,8 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { row_index: usize, ) -> Result> { match expr { - SqlExpression::Column(_) => { - let column_name = self.extract_column_name(expr)?; - let col_index = - self.table.get_column_index(&column_name).ok_or_else(|| { - match self.find_similar_column(&column_name) { - Some(similar) => anyhow!( - "Column '{}' not found. Did you mean '{}'?", - column_name, - similar - ), - None => anyhow!("Column '{}' not found", column_name), - } - })?; + SqlExpression::Column(column_ref) => { + let col_index = self.resolve_column_index(column_ref)?; Ok(self.table.get_value(row_index, col_index).cloned()) } SqlExpression::StringLiteral(s) => Ok(Some(DataValue::String(s.clone()))), @@ -818,7 +808,14 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { 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).with_case_insensitive(self.case_insensitive) + let evaluator = + ArithmeticEvaluator::new(self.table).with_case_insensitive(self.case_insensitive); + // The same aliases WHERE's own column operands resolve through, + // so `i.a` means the same column inside an expression operand. + match self.exec_context { + Some(exec_ctx) => evaluator.with_table_aliases(exec_ctx.get_aliases()), + None => evaluator, + } }) .evaluate(expr, row_index) } @@ -1050,30 +1047,30 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { } } - fn extract_column_name(&self, expr: &SqlExpression) -> Result { - match expr { - SqlExpression::Column(column_ref) => { - // Use ExecutionContext for proper alias resolution if available - if let Some(exec_ctx) = self.exec_context { - // Use the unified column resolution - let col_idx = exec_ctx.resolve_column_index(self.table, column_ref)?; - // Return the actual column name (not the qualified one) - Ok(self.table.column_names()[col_idx].clone()) - } else { - // Fallback: Handle qualified column names the old way (for backward compatibility) - if column_ref.name.contains('.') { - if let Some(dot_pos) = column_ref.name.rfind('.') { - Ok(column_ref.name[dot_pos + 1..].to_string()) - } else { - Ok(column_ref.name.clone()) - } - } else { - Ok(column_ref.name.clone()) - } - } - } - _ => Err(anyhow::anyhow!("Expected column name, got: {:?}", expr)), + /// The index of a column operand. With an `ExecutionContext` this is the + /// unified alias-aware resolution, and its answer is the index itself: it + /// used to be turned back into a bare name and looked up again, which + /// finds the first column of that name rather than the one the qualifier + /// picked. + fn resolve_column_index(&self, column_ref: &ColumnRef) -> Result { + if let Some(exec_ctx) = self.exec_context { + return exec_ctx.resolve_column_index(self.table, column_ref); } + // Without a context: a dotted name is read as its last segment. + let column_name = match column_ref.name.rfind('.') { + Some(dot_pos) => &column_ref.name[dot_pos + 1..], + None => column_ref.name.as_str(), + }; + self.table.get_column_index(column_name).ok_or_else(|| { + match self.find_similar_column(column_name) { + Some(similar) => anyhow!( + "Column '{}' not found. Did you mean '{}'?", + column_name, + similar + ), + None => anyhow!("Column '{}' not found", column_name), + } + }) } fn extract_string_value(&self, expr: &SqlExpression) -> Result { @@ -1697,10 +1694,10 @@ mod operand_resolution_tests { // `orders.a` first, so a wrong resolution shows as a wrong answer. // // Latent, not reachable from SQL as probed: join output already - // disambiguates duplicate names before WHERE sees them. But WHERE's - // column fast path resolves the alias to an index and then looks the - // bare name up again, and the inner value evaluator is given no - // aliases at all -- so both answer from `orders.a` here. + // disambiguates duplicate names before WHERE sees them. WHERE's column + // fast path used to resolve the alias to an index and then look the + // bare name up again, and the inner value evaluator was given no + // aliases -- so both answered from `orders.a` here. let t = joined(); let mut ctx = ExecutionContext::new(); ctx.register_alias("i".to_string(), "items".to_string()); @@ -1714,11 +1711,11 @@ mod operand_resolution_tests { }; // (predicate, correct answer for row 0, answer today if it differs) for (predicate, correct, known) in [ - ("i.a = 7", Trilean::True, Some(Trilean::False)), - ("i.a BETWEEN 5 AND 8", Trilean::True, Some(Trilean::False)), - ("i.a IN (7, 99)", Trilean::True, Some(Trilean::False)), - ("i.a NOT IN (7, 99)", Trilean::False, Some(Trilean::True)), - ("i.a + 0 = 7", Trilean::True, Some(Trilean::False)), + ("i.a = 7", Trilean::True, None), + ("i.a BETWEEN 5 AND 8", Trilean::True, None), + ("i.a IN (7, 99)", Trilean::True, None), + ("i.a NOT IN (7, 99)", Trilean::False, None), + ("i.a + 0 = 7", Trilean::True, None), ] { let observed = eval(predicate, 0); match known { From e438511684f9989f168fdfae94baeddd6e7a6f3e Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 21:04:52 +0100 Subject: [PATCH 3/6] perf(r13): drop per-node logging from the value evaluator (R13 slice 4) Non-interactive mode installs a TRACE subscriber writing to a ring buffer and a log file, so every `debug!` in `ArithmeticEvaluator::evaluate` - once per expression node per row, formatting the whole expression with `{:?}` - was paid in full. It was most of the evaluator's cost: with the filter lowered (`RUST_LOG=warn`) `WHERE price + 0 > quantity` over trades_100000.csv ran in load time. Removed the entry log, the binary-op log (and its now-unused format_value), column resolution, method proxy and CASE branch logs. Best of 3, ~2.4 s load: WHERE price + 0 > quantity 5.5 s -> 3.0 s SELECT price * quantity 4.8 s -> 2.6 s GROUP BY UPPER(trader) 4.3 s -> 2.6 s GROUP BY trader 3.7 s -> 2.5 s Found measuring slice 4: WHERE arms that delegate to this evaluator must not cost more than the arms they replace. Parity report byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/arithmetic_evaluator.rs | 61 +++----------------------------- 1 file changed, 5 insertions(+), 56 deletions(-) diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 77f068cf..87e9de98 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -119,12 +119,12 @@ impl<'a> ArithmeticEvaluator<'a> { } /// Evaluate an SQL expression to produce a `DataValue` + /// + /// No logging on this path: it runs once per expression node per row, and + /// non-interactive mode records TRACE to a file, so a `debug!` formatting + /// the expression here cost more than evaluating it (2.5 s of a 5 s + /// `WHERE price + 0 > quantity` over 100k rows). pub fn evaluate(&mut self, expr: &SqlExpression, row_index: usize) -> Result { - debug!( - "ArithmeticEvaluator: evaluating {:?} for row {}", - expr, row_index - ); - match expr { SqlExpression::Column(column_ref) => self.evaluate_column_ref(column_ref, row_index), SqlExpression::StringLiteral(s) => Ok(DataValue::String(s.clone())), @@ -225,10 +225,6 @@ impl<'a> ArithmeticEvaluator<'a> { let qualified_name = format!("{}.{}", actual_table, column_ref.name); if let Some(col_idx) = self.table.find_column_by_qualified_name(&qualified_name) { - debug!( - "Resolved {}.{} -> '{}' at index {}", - table_prefix, column_ref.name, qualified_name, col_idx - ); return self .table .get_value(row_index, col_idx) @@ -238,10 +234,6 @@ impl<'a> ArithmeticEvaluator<'a> { // Fallback: try unqualified lookup if let Some(col_idx) = self.table.get_column_index(&column_ref.name) { - debug!( - "Resolved {}.{} -> unqualified '{}' at index {}", - table_prefix, column_ref.name, column_ref.name, col_idx - ); return self .table .get_value(row_index, col_idx) @@ -275,10 +267,6 @@ impl<'a> ArithmeticEvaluator<'a> { // For now, just use the column name part // In the future, we could validate the table/alias part - debug!( - "Resolving qualified column: {} -> {}", - column_name, col_name - ); col_name.to_string() } else { column_name.to_string() @@ -358,13 +346,6 @@ impl<'a> ArithmeticEvaluator<'a> { let left_val = self.evaluate(left, row_index)?; let right_val = self.evaluate(right, row_index)?; - debug!( - "ArithmeticEvaluator: {} {} {}", - self.format_value(&left_val), - op, - self.format_value(&right_val) - ); - match op { "+" => self.add_values(&left_val, &right_val), "-" => self.subtract_values(&left_val, &right_val), @@ -486,16 +467,6 @@ impl<'a> ArithmeticEvaluator<'a> { } } - /// Format a `DataValue` for debug output - fn format_value(&self, value: &DataValue) -> String { - match value { - DataValue::Integer(i) => i.to_string(), - DataValue::Float(f) => f.to_string(), - DataValue::String(s) => format!("'{s}'"), - _ => format!("{value:?}"), - } - } - /// Convert DataValue to string for pattern matching fn value_to_string(&self, value: &DataValue) -> String { match value { @@ -1431,11 +1402,6 @@ impl<'a> ArithmeticEvaluator<'a> { // Check if we have this function in the registry if self.function_registry.get(function_name).is_some() { - debug!( - "Proxying method '{}' through function registry as '{}'", - method, function_name - ); - // Prepare arguments: receiver is the first argument, followed by method args let mut func_args = vec![value.clone()]; @@ -1464,28 +1430,20 @@ impl<'a> ArithmeticEvaluator<'a> { else_branch: &Option>, row_index: usize, ) -> Result { - debug!( - "ArithmeticEvaluator: evaluating CASE expression for row {}", - row_index - ); - // Evaluate each WHEN condition in order for branch in when_branches { // Evaluate the condition as a boolean let condition_result = self.evaluate_condition_as_bool(&branch.condition, row_index)?; if condition_result { - debug!("CASE: WHEN condition matched, evaluating result expression"); return self.evaluate(&branch.result, row_index); } } // If no WHEN condition matched, evaluate ELSE clause (or return NULL) if let Some(else_expr) = else_branch { - debug!("CASE: No WHEN matched, evaluating ELSE expression"); self.evaluate(else_expr, row_index) } else { - debug!("CASE: No WHEN matched and no ELSE, returning NULL"); Ok(DataValue::Null) } } @@ -1498,14 +1456,8 @@ impl<'a> ArithmeticEvaluator<'a> { else_branch: &Option>, row_index: usize, ) -> Result { - debug!( - "ArithmeticEvaluator: evaluating simple CASE expression for row {}", - row_index - ); - // Evaluate the main expression once let case_value = self.evaluate(expr, row_index)?; - debug!("Simple CASE: evaluated expression to {:?}", case_value); // Compare against each WHEN value in order for branch in when_branches { @@ -1514,17 +1466,14 @@ impl<'a> ArithmeticEvaluator<'a> { // Check for equality if self.values_equal(&case_value, &when_value)? { - debug!("Simple CASE: WHEN value matched, evaluating result expression"); return self.evaluate(&branch.result, row_index); } } // If no WHEN value matched, evaluate ELSE clause (or return NULL) if let Some(else_expr) = else_branch { - debug!("Simple CASE: No WHEN matched, evaluating ELSE expression"); self.evaluate(else_expr, row_index) } else { - debug!("Simple CASE: No WHEN matched and no ELSE, returning NULL"); Ok(DataValue::Null) } } From 16c7561a05a01af2c5b55be4aba9b46aeb4aca94 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 21:10:29 +0100 Subject: [PATCH 4/6] refactor(r13): WHERE's BETWEEN delegates to the value evaluator (R13 slice 4) The BETWEEN arm now evaluates through `ArithmeticEvaluator` and reads the answer back with `Trilean::from_value`; `evaluate_between` and its own copy of the AND-over-two-comparisons rule are gone. `evaluate_delegated` is the one crossing every delegated arm will use. The P15 raw-window-operand check stays on WHERE's side (`reject_unlifted_window`, now shared with `evaluate_operand_value`), since the value evaluator would evaluate one quietly. Parity report byte-identical, matrix green (both runs), pins from the first commit hold. `WHERE price BETWEEN 100 AND 500` over trades_100000.csv 3.0-3.2 s -> 2.5-2.7 s. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/recursive_where_evaluator.rs | 73 ++++++++++++++------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index e80b0942..ab2bee76 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -506,8 +506,15 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { // stay UNKNOWN rather than flip to TRUE. That flip is P19. Ok(in_result.negate()) } - SqlExpression::Between { expr, lower, upper } => { - self.evaluate_between(expr, lower, upper, row_index) + SqlExpression::Between { + expr: value, + lower, + upper, + } => { + for operand in [value, lower, upper] { + reject_unlifted_window(operand)?; + } + self.evaluate_delegated(expr, row_index) } SqlExpression::Not { expr } => { let inner_result = self.evaluate_expression(expr, row_index)?; @@ -791,15 +798,10 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { .ok_or_else(|| anyhow::anyhow!("Invalid time"))?; Ok(Some(datetime_value(NaiveDateTime::new(today, time)))) } - // Same rule as `evaluate_value_as_predicate`: a raw window function - // here was not lifted to a CTE column. Evaluating it would compute - // the window over the rows the WHERE is still filtering, so an - // inline `QUALIFY ROW_NUMBER() OVER (...) = 1` combined with a WHERE - // would silently rank the unfiltered table (P15). - SqlExpression::WindowFunction { name, .. } => Err(anyhow!( - "Window function {name} cannot be used directly in a comparison (the expression was not lifted to a CTE column)" - )), - _ => Ok(Some(self.evaluate_arithmetic(expr, row_index)?)), + _ => { + reject_unlifted_window(expr)?; + Ok(Some(self.evaluate_arithmetic(expr, row_index)?)) + } } } @@ -857,31 +859,15 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { }) } - fn evaluate_between( - &self, - expr: &SqlExpression, - lower: &SqlExpression, - upper: &SqlExpression, - row_index: usize, - ) -> Result { - let cell_value = self.evaluate_operand_value(expr, row_index)?; - let lower_data_value = self - .evaluate_operand_value(lower, row_index)? - .unwrap_or(DataValue::Null); - let upper_data_value = self - .evaluate_operand_value(upper, row_index)? - .unwrap_or(DataValue::Null); - - let table_value = cell_value.unwrap_or(DataValue::Null); - - // BETWEEN is defined as `value >= lower AND value <= upper`, so it takes - // AND's truth table too: FALSE on either side still wins outright (a - // value below `lower` is not between, whatever `upper` is), and UNKNOWN - // only survives when the other side is TRUE or UNKNOWN. - let ge_lower = self.compare_trilean(&table_value, &lower_data_value, ">="); - let le_upper = self.compare_trilean(&table_value, &upper_data_value, "<="); - - Ok(ge_lower.and(le_upper)) + /// Evaluate a predicate with the value evaluator and read its truth value + /// back, NULL as UNKNOWN (R13 slice 4). An arm that delegates here has no + /// rules of its own left in this file: the value evaluator's arm is the + /// only implementation, and this is the one place its answer crosses back + /// into a `Trilean`. + fn evaluate_delegated(&self, expr: &SqlExpression, row_index: usize) -> Result { + let value = self.evaluate_arithmetic(expr, row_index)?; + Trilean::from_value(&value) + .ok_or_else(|| anyhow!("Predicate did not evaluate to a truth value: {value:?}")) } fn evaluate_method_call( @@ -1184,6 +1170,21 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { } } +/// A raw window function in a WHERE operand was not lifted to a CTE column. +/// Evaluating it would compute the window over the rows the WHERE is still +/// filtering, so an inline `QUALIFY ROW_NUMBER() OVER (...) = 1` combined with +/// a WHERE would silently rank the unfiltered table (P15). The value evaluator +/// would evaluate one quietly, so this check stays on WHERE's side of every arm +/// that delegates to it. +fn reject_unlifted_window(expr: &SqlExpression) -> Result<()> { + match expr { + SqlExpression::WindowFunction { name, .. } => Err(anyhow!( + "Window function {name} cannot be used directly in a comparison (the expression was not lifted to a CTE column)" + )), + _ => Ok(()), + } +} + /// 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 { From 4393f5d20a69b826426585d13ddc5fb7c3312031 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 21:16:36 +0100 Subject: [PATCH 5/6] refactor(r13): WHERE's IN / NOT IN delegate to the value evaluator (R13 slice 4) Both arms now evaluate through `ArithmeticEvaluator` via `evaluate_delegated`; `evaluate_in_list` - WHERE's copy of the OR-over- equalities rule, with an unused `_ignore_case` parameter - is gone. The raw window check covers the probe and every list item, as before. Parity report byte-identical, matrix green (both runs). Over trades_100000.csv: IN 3.6 s -> 3.0 s, NOT IN 3.0 s -> 2.5 s. Co-Authored-By: Claude Opus 5 (1M context) --- src/data/recursive_where_evaluator.rs | 57 ++++++--------------------- 1 file changed, 12 insertions(+), 45 deletions(-) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index ab2bee76..793e889a 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -496,15 +496,19 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { SqlExpression::BinaryOp { left, op, right } => { self.evaluate_binary_op(left, op, right, row_index) } - SqlExpression::InList { expr, values } => { - self.evaluate_in_list(expr, values, row_index, false) + SqlExpression::InList { + expr: probe, + values, } - SqlExpression::NotInList { expr, values } => { - let in_result = self.evaluate_in_list(expr, values, row_index, false)?; - // Three-valued negation: once `evaluate_in_list` can return - // UNKNOWN (a NULL operand, or a NULL in the list), this must - // stay UNKNOWN rather than flip to TRUE. That flip is P19. - Ok(in_result.negate()) + | SqlExpression::NotInList { + expr: probe, + values, + } => { + reject_unlifted_window(probe)?; + for item in values { + reject_unlifted_window(item)?; + } + self.evaluate_delegated(expr, row_index) } SqlExpression::Between { expr: value, @@ -822,43 +826,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { .evaluate(expr, row_index) } - fn evaluate_in_list( - &self, - expr: &SqlExpression, - values: &[SqlExpression], - row_index: usize, - _ignore_case: bool, - ) -> Result { - let cell_value = self.evaluate_operand_value(expr, row_index)?; - - // `x IN (a, b, ...)` is `x = a OR x = b OR ...`, so it inherits OR's - // truth table: a TRUE anywhere wins outright, but if nothing matched - // and any comparison was UNKNOWN, the answer is UNKNOWN — not FALSE. - // That distinction is invisible under `IN` (both drop the row) and is - // the whole of P19 under `NOT IN`, where FALSE would wrongly negate to - // TRUE and admit the NULL rows. - let mut saw_unknown = false; - - let table_value = cell_value.as_ref().unwrap_or(&DataValue::Null); - for value_expr in values { - let comparison_value = self - .evaluate_operand_value(value_expr, row_index)? - .unwrap_or(DataValue::Null); - - match self.compare_trilean(table_value, &comparison_value, "=") { - Trilean::True => return Ok(Trilean::True), - Trilean::Unknown => saw_unknown = true, - Trilean::False => {} - } - } - - Ok(if saw_unknown { - Trilean::Unknown - } else { - Trilean::False - }) - } - /// Evaluate a predicate with the value evaluator and read its truth value /// back, NULL as UNKNOWN (R13 slice 4). An arm that delegates here has no /// rules of its own left in this file: the value evaluator's arm is the From a66fa7b170fb885daf1901ee8e4e7b26d0d5b97d Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 15 Sep 2026 21:17:31 +0100 Subject: [PATCH 6/6] docs(r13): record slice 4 first part (BETWEEN / IN delegate, logging cost) Co-Authored-By: Claude Opus 5 (1M context) --- docs/ENGINE_REFACTORING.md | 43 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index a6f6d075..c175e1fb 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -473,7 +473,7 @@ 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 - next. + in progress (`BETWEEN`, `IN`, `NOT IN` delegated 2026-09-15). **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 → @@ -597,6 +597,44 @@ feature work**, and so we can tell the difference between "this is awkward" and `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). +- **Slice 4, first part (2026-09-15).** `BETWEEN`, `IN` and `NOT IN` in WHERE + now evaluate through the value evaluator and cross back once, in + `evaluate_delegated` (`Trilean::from_value`); WHERE's own copies of the + AND/OR-over-comparisons rules are deleted. The P15 raw-window-operand check + stays on WHERE's side of the delegation (`reject_unlifted_window`) — the value + evaluator would evaluate one quietly. Five commits, parity report + byte-identical after each, matrix green in both runs: + - *Pin, then fix, alias resolution.* Delegating moves column resolution onto + the value evaluator, so both resolvers were probed first. Both were wrong + for an alias-qualified operand over two columns sharing a name: WHERE's fast + path resolved `i.a` to an index through the `ExecutionContext`, turned it + back into the bare name and looked *that* up (finding the first `a`), and + the inner value evaluator was built with no aliases. Latent — join output + disambiguates duplicate names before WHERE sees them, so no SQL probe + reached it — but pinned as five recorded divergences, then fixed by using + the index directly and handing the context's aliases in. + - *Logging was the cost.* The first delegation (BETWEEN) looked like a 6× + per-row regression. It was not the evaluator: non-interactive mode records + TRACE to a ring buffer and a log file, and `ArithmeticEvaluator::evaluate` + formatted the expression with `{:?}` at every node of every row. Removed — + and every existing value-evaluator path got faster with it (over + `trades_100000.csv`, ~2.4 s load: `WHERE price + 0 > quantity` 5.5 → 3.0 s, + `SELECT price * quantity` 4.8 → 2.6 s, `GROUP BY UPPER(trader)` 4.3 → + 2.6 s). Delegated BETWEEN / IN / NOT IN each ended *faster* than the arm + they replaced. + - *Still ahead for the comparison arms* — each needs its own pin, since the + operand readers are not quite the same: WHERE reads `DATETIME(...)` as a + `DateTime` and `TODAY` in **local** time, the value evaluator as a `String` + and in **UTC** (the two disagree near midnight); WHERE reads `5.0` as + `Integer`, the value evaluator as `Float`; a comparison's left side may be a + legacy method call (`Length()`, `IndexOf()`, `Trim*()`) with no registry + twin yet (slice 5). `LIKE` has a regex cache in `EvaluationContext` that the + value evaluator's matcher does not use. `AND` / `OR` / `NOT` / `CASE` stay + recursive until their children can be WHERE-only method calls no longer. + - *Noticed, not done:* `WHERE price > quantity` (WHERE's own path) still runs + ~1 s over load where the delegated shapes do not — likely more per-row + logging in or around the WHERE loop; and window evaluation logs `info!` + timings per row (out of R13's scope). - **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 @@ -662,7 +700,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–6 (retire WHERE arms) +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 ``` **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator @@ -703,3 +741,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 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 | — | +| 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 | — |