From e5f9a8931f888b2a30cd75f053900b314c3791a6 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 13 Sep 2026 08:52:10 +0100 Subject: [PATCH 1/3] test(parity): pin every site of the P46 right-hand-operand hole Re-probing P46 from a second angle showed the cause is wider than column-vs-column: every right-hand operand of a WHERE predicate (the comparison RHS, IN-list items, BETWEEN bounds) went through a literal-only reader that answered NULL for anything else. Six cases, all DIFFER against the current binary, including the NOT (expr < col) shape where P48 leaks into WHERE and returns too many rows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW --- tests/comparison/corpus/02_where.toml | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index d563f828..6331ef61 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -200,3 +200,61 @@ expect = "DIFFER" # P46 again, on strings and on equality, to pin that the defect is the # column-vs-column *shape* rather than one operator or one type. DuckDB matches # 57 of the 200 rows; we match none. + +# The cases below were added 2026-09-13 when P46 was re-probed from a second +# angle. The cause turned out wider than "column vs column": every right-hand +# operand of a WHERE predicate went through a literal-only reader that answered +# NULL for anything else - a column, a CASE, an expression - so the predicate +# was UNKNOWN on every row. These pin each site that reader served. + +[[case]] +id = "where_col_eq_same_col" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE team = team ORDER BY id" +expect = "DIFFER" +# The tell from the P46 entry: the same column on both sides. No type, no +# coercion, no data involved. DuckDB keeps every non-NULL team (all but 7, 8). + +[[case]] +id = "where_not_col_lt_col" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE NOT (id < partner_id) ORDER BY id" +expect = "DIFFER" +# P46 under NOT. DuckDB: 2, 5, 8 - the NULL-partner rows stay out, because +# NOT UNKNOWN is UNKNOWN. Guards the fix against landing as "false" for NULL. + +[[case]] +id = "where_not_expr_lt_col" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE NOT (id + 0 < partner_id) ORDER BY id" +expect = "DIFFER" +# The one P46-adjacent shape that did NOT return zero rows: an arithmetic +# operand sent the whole comparison to the ArithmeticEvaluator, which answers +# false (not NULL) for a NULL operand - P48 leaking into WHERE. Under NOT that +# admits the NULL-partner rows: we returned 8 rows, DuckDB 2, 5, 8. + +[[case]] +id = "where_col_lt_case" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE score < CASE WHEN team = 'alpha' THEN 60 ELSE 40 END ORDER BY id" +expect = "DIFFER" +# Not a column at all on the right - a CASE - and the same zero rows. DuckDB: +# 1, 2, 5, 8, 9 (rows 7/8 have a NULL team, so the WHEN is UNKNOWN and they +# take the ELSE bound of 40). + +[[case]] +id = "where_between_col_bound" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE partner_id BETWEEN id AND 10 ORDER BY id" +expect = "DIFFER" +# BETWEEN's bounds used the same literal-only reader. DuckDB: 1, 4, 7 +# (row 9's partner 11 is above the literal upper bound). + +[[case]] +id = "where_in_list_expr_item" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE partner_id IN (id + 2, 5) ORDER BY id" +expect = "DIFFER" +# And IN-list items. The literal 5 still matched, so this one hid as a +# too-few-rows answer rather than an empty one: we returned only 4, DuckDB +# 1, 4, 9. From 1512523a5286e607f13d98502ea6c41f3f85ded4 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 13 Sep 2026 09:21:30 +0100 Subject: [PATCH 2/3] fix(parity): resolve every WHERE operand through one path (P46) The WHERE evaluator read right-hand operands with a literal-only reader that ended `_ => Ok(Null)`: a column, CASE or expression on the right of a comparison, in an IN list or as a BETWEEN bound read as NULL, so the predicate was UNKNOWN on every row and returned zero rows. A literal on the LEFT was a hard error (`WHERE 1=0`), and an arithmetic operand handed the whole comparison to the ArithmeticEvaluator, which answers false for NULL - so NOT admitted the NULL rows. All operands now go through `evaluate_operand_value` (columns and literals direct, anything else via the ArithmeticEvaluator) and every comparison stays in `compare_trilean`, keeping the NULL -> UNKNOWN rule. `extract_value` and `ExprValue` are gone. Two traps handled on the way: - one ArithmeticEvaluator is now shared per evaluation; building one per row cost 9.8s on 100k rows and made an inline window quadratic (2k rows: 9.5s) - a raw window-function operand is refused, as evaluate_value_as_predicate already did: evaluating it ranks the rows WHERE is still filtering, so QUALIFY + WHERE silently returned the wrong rows (P15 stays GAP, guarded by qualify_inline_with_where) Parity 157 -> 168 AGREE over 197 cases. Three correlated-subquery cases move because outer references now bind to the inner table (P49), accepted and re-bucketed with comments. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW --- src/data/recursive_where_evaluator.rs | 444 +++++++++++++-------- tests/comparison/corpus/02_where.toml | 33 +- tests/comparison/corpus/05_subqueries.toml | 21 +- tests/comparison/corpus/09_window.toml | 13 + 4 files changed, 322 insertions(+), 189 deletions(-) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index 92902e1e..09871e11 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -6,7 +6,8 @@ use crate::data::trilean::Trilean; use crate::data::value_comparisons::compare_with_op; use crate::sql::recursive_parser::{Condition, LogicalOp, SqlExpression, WhereClause}; use anyhow::{anyhow, Result}; -use chrono::{DateTime, Local, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Utc}; +use chrono::{Local, NaiveDate, NaiveDateTime, NaiveTime}; +use std::cell::RefCell; use tracing::debug; /// Evaluates WHERE clauses from `recursive_parser` directly against `DataTable` @@ -15,6 +16,11 @@ pub struct RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive: bool, context: Option<&'ctx mut EvaluationContext>, exec_context: Option<&'exec ExecutionContext>, + /// One `ArithmeticEvaluator` for the whole evaluation, built on first use. + /// Constructing one builds the function and aggregate registries, and its + /// window-context cache only pays off if it outlives a single row -- a + /// fresh evaluator per row made an inline window predicate quadratic. + arithmetic: RefCell>>, } impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { @@ -25,6 +31,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive: false, context: None, exec_context: None, + arithmetic: RefCell::new(None), } } @@ -36,6 +43,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive, context: Some(context), exec_context: None, + arithmetic: RefCell::new(None), } } @@ -50,6 +58,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive, context: None, exec_context: Some(exec_context), + arithmetic: RefCell::new(None), } } @@ -65,6 +74,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive, context: Some(context), exec_context: Some(exec_context), + arithmetic: RefCell::new(None), } } @@ -131,6 +141,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive, context: None, exec_context: None, + arithmetic: RefCell::new(None), } } @@ -145,6 +156,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { case_insensitive, context: None, exec_context: None, + arithmetic: RefCell::new(None), } } @@ -169,26 +181,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { Trilean::from_bool(compare_with_op(left, right, op, self.case_insensitive)) } - /// Convert ExprValue to DataValue for centralized comparison - fn expr_value_to_data_value(&self, expr_value: &ExprValue) -> DataValue { - match expr_value { - ExprValue::String(s) => DataValue::String(s.clone()), - ExprValue::Number(n) => { - // Check if it's an integer or float - if n.fract() == 0.0 && *n >= i64::MIN as f64 && *n <= i64::MAX as f64 { - DataValue::Integer(*n as i64) - } else { - DataValue::Float(*n) - } - } - ExprValue::Boolean(b) => DataValue::Boolean(*b), - ExprValue::DateTime(dt) => { - DataValue::DateTime(dt.format("%Y-%m-%d %H:%M:%S%.3f").to_string()) - } - ExprValue::Null => DataValue::Null, - } - } - /// Evaluate the `Length()` method on a column value fn evaluate_length( &self, @@ -588,41 +580,35 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { }); } - // For complex expressions (arithmetic, functions), use ArithmeticEvaluator - if matches!(left, SqlExpression::BinaryOp { .. }) - || matches!(left, SqlExpression::FunctionCall { .. }) - || matches!(right, SqlExpression::BinaryOp { .. }) - || matches!(right, SqlExpression::FunctionCall { .. }) - { - let comparison_expr = SqlExpression::BinaryOp { + let op_upper = op.to_uppercase(); + + // An operator that does not yield a truth value (`WHERE a + b`, + // `WHERE x || y`) makes the whole expression a value used as a + // predicate -- the P37 rule, rather than a comparison under an + // operator `compare_with_op` would silently answer false for. + if !is_predicate_operator(&op_upper) { + let value_expr = SqlExpression::BinaryOp { left: Box::new(left.clone()), op: op.to_string(), right: Box::new(right.clone()), }; - - let mut evaluator = ArithmeticEvaluator::new(self.table); - let result = evaluator.evaluate(&comparison_expr, row_index)?; - - // Convert the result to a truth value. A NULL out of the - // arithmetic evaluator means a NULL propagated through the - // expression, so the comparison is UNKNOWN rather than false. - return match result { - DataValue::Boolean(b) => Ok(Trilean::from_bool(b)), - DataValue::Null => Ok(Trilean::Unknown), - _ => Err(anyhow!("Comparison did not return a boolean value")), - }; + return self.evaluate_value_as_predicate(&value_expr, row_index); } - // For simple comparisons, use the original WHERE clause logic with improved date parsing - // Handle left side - could be a column or a method call - let (cell_value, column_name) = match left { + // Both operands are resolved to values here and compared below, so the + // NULL -> UNKNOWN rule in `compare_trilean` applies whatever shape the + // operands take. Comparisons with an arithmetic operand used to be + // handed whole to the ArithmeticEvaluator, whose comparison answers + // `false` for a NULL operand -- which `NOT` then flipped to TRUE. + // The left side can also be one of the legacy method calls. + let cell_value = match left { SqlExpression::MethodCall { object, method, args, } => { // Handle method calls that return values (like Length(), IndexOf()) - match method.to_lowercase().as_str() { + let (value, _label) = match method.to_lowercase().as_str() { "length" => { if !args.is_empty() { return Err(anyhow::anyhow!("Length() takes no arguments")); @@ -660,56 +646,34 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { method )); } - } - } - _ => { - // Regular column reference - let column_name = self.extract_column_name(left)?; - if row_index < 3 { - debug!( - "RecursiveWhereEvaluator: evaluate_binary_op() - column_name = '{}'", - column_name - ); - } - - let col_index = self.table.get_column_index(&column_name).ok_or_else(|| { - let suggestion = self.find_similar_column(&column_name); - match suggestion { - Some(similar) => anyhow!( - "Column '{}' not found. Did you mean '{}'?", - column_name, - similar - ), - None => anyhow!("Column '{}' not found", column_name), - } - })?; - - let cell_value = self.table.get_value(row_index, col_index).cloned(); - (cell_value, column_name) + }; + value } + _ => self.evaluate_operand_value(left, row_index)?, }; + let compare_value = self + .evaluate_operand_value(right, row_index)? + .unwrap_or(DataValue::Null); + if row_index < 3 { debug!( - "RecursiveWhereEvaluator: evaluate_binary_op() - row {} column '{}' value = {:?}", - row_index, column_name, cell_value + "RecursiveWhereEvaluator: evaluate_binary_op() - row {} {:?} {} {:?}", + row_index, cell_value, op, compare_value ); } - // Get comparison value from right side - let compare_value = self.extract_value(right)?; - // Handle special operators that aren't standard comparisons - let op_upper = op.to_uppercase(); match op_upper.as_str() { // LIKE operator - handle specially "LIKE" => { let table_value = cell_value.unwrap_or(DataValue::Null); let pattern = match compare_value { - ExprValue::String(s) => s, + DataValue::String(s) => s, + DataValue::InternedString(s) => s.to_string(), // A NULL pattern makes the whole predicate UNKNOWN; any // other non-string pattern is simply not a match. - ExprValue::Null => return Ok(Trilean::Unknown), + DataValue::Null => return Ok(Trilean::Unknown), _ => return Ok(Trilean::False), }; @@ -747,39 +711,37 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { cell_value.is_some() && !matches!(cell_value, Some(DataValue::Null)), )), - // Handle IS / IS NOT with NULL explicitly - "IS" if matches!(compare_value, ExprValue::Null) => Ok(Trilean::from_bool( + // `x IS NULL` written with an explicit NULL operand. Matched on the + // expression, not the value: `a IS b` where b happens to be NULL on + // this row is not a NULL test. + "IS" if matches!(right, SqlExpression::Null) => Ok(Trilean::from_bool( cell_value.is_none() || matches!(cell_value, Some(DataValue::Null)), )), - "IS NOT" if matches!(compare_value, ExprValue::Null) => Ok(Trilean::from_bool( + "IS NOT" if matches!(right, SqlExpression::Null) => Ok(Trilean::from_bool( cell_value.is_some() && !matches!(cell_value, Some(DataValue::Null)), )), // Standard comparison operators - use centralized logic _ => { let table_value = cell_value.unwrap_or(DataValue::Null); - let comparison_value = self.expr_value_to_data_value(&compare_value); - - if row_index < 3 { - debug!( - "RecursiveWhereEvaluator: Using centralized comparison - table: {:?}, op: '{}', comparison: {:?}, case_insensitive: {}", - table_value, op, comparison_value, self.case_insensitive - ); - } - - Ok(self.compare_trilean(&table_value, &comparison_value, op)) + Ok(self.compare_trilean(&table_value, &compare_value, op)) } } } - /// Resolve the value of a WHERE operand that is expected to yield a scalar: - /// a plain column reference (looked up in the row) or an arbitrary - /// expression (evaluated with the `ArithmeticEvaluator`). IN / BETWEEN take - /// such an operand on their left; it is usually a bare column, but after an - /// IN-subquery is substituted into an IN-list the LHS keeps its original - /// expression form (e.g. `price * 2`), which the `InOperatorLifter` never - /// got to lift because that runs before subquery substitution. Mirrors the - /// arithmetic delegation `evaluate_binary_op` already does for its LHS. + /// Resolve the value of any WHERE operand -- either side of a comparison, + /// the probe and items of an IN list, the value and bounds of BETWEEN. + /// + /// Columns and literals are read directly; anything else (arithmetic, a + /// function, a CASE) is evaluated by the `ArithmeticEvaluator`. The direct + /// arms are a fast path, not a semantic difference: constructing an + /// `ArithmeticEvaluator` builds the function and aggregate registries, and + /// `WHERE x = 5` evaluates its literal once per row. + /// + /// This replaced a literal-only reader that answered NULL for every other + /// shape, which made `WHERE a < b` UNKNOWN on every row (P46). There is + /// deliberately no catch-all that invents a value: an operand the + /// evaluator cannot handle is an error. fn evaluate_operand_value( &self, expr: &SqlExpression, @@ -788,19 +750,77 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { 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(|| anyhow::anyhow!("Column '{}' not found", column_name))?; + 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), + } + })?; Ok(self.table.get_value(row_index, col_index).cloned()) } - _ => { - let mut evaluator = ArithmeticEvaluator::new(self.table); - Ok(Some(evaluator.evaluate(expr, row_index)?)) + SqlExpression::StringLiteral(s) => Ok(Some(DataValue::String(s.clone()))), + SqlExpression::BooleanLiteral(b) => Ok(Some(DataValue::Boolean(*b))), + SqlExpression::Null => Ok(Some(DataValue::Null)), + SqlExpression::NumberLiteral(n) => Ok(Some(number_literal_value(n))), + SqlExpression::DateTimeConstructor { + year, + month, + day, + hour, + minute, + second, + } => { + let naive_date = NaiveDate::from_ymd_opt(*year, *month, *day) + .ok_or_else(|| anyhow::anyhow!("Invalid date: {}-{}-{}", year, month, day))?; + let naive_time = NaiveTime::from_hms_opt( + hour.unwrap_or(0), + minute.unwrap_or(0), + second.unwrap_or(0), + ) + .ok_or_else(|| anyhow::anyhow!("Invalid time"))?; + Ok(Some(datetime_value(NaiveDateTime::new( + naive_date, naive_time, + )))) + } + SqlExpression::DateTimeToday { + hour, + minute, + second, + } => { + let today = Local::now().date_naive(); + let time = NaiveTime::from_hms_opt( + hour.unwrap_or(0), + minute.unwrap_or(0), + second.unwrap_or(0), + ) + .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)?)), } } + /// Evaluate a value expression with the evaluation's shared + /// `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) + } + fn evaluate_in_list( &self, expr: &SqlExpression, @@ -818,10 +838,11 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { // 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 compare_value = self.extract_value(value_expr)?; - let table_value = cell_value.as_ref().unwrap_or(&DataValue::Null); - let comparison_value = self.expr_value_to_data_value(&compare_value); + 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), @@ -845,12 +866,14 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { row_index: usize, ) -> Result { let cell_value = self.evaluate_operand_value(expr, row_index)?; - let lower_value = self.extract_value(lower)?; - let upper_value = self.extract_value(upper)?; + 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); - let lower_data_value = self.expr_value_to_data_value(&lower_value); - let upper_data_value = self.expr_value_to_data_value(&upper_value); // 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 @@ -1058,59 +1081,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { } } - fn extract_value(&self, expr: &SqlExpression) -> Result { - match expr { - SqlExpression::StringLiteral(s) => Ok(ExprValue::String(s.clone())), - SqlExpression::BooleanLiteral(b) => Ok(ExprValue::Boolean(*b)), - SqlExpression::NumberLiteral(n) => { - if let Ok(num) = n.parse::() { - Ok(ExprValue::Number(num)) - } else { - Ok(ExprValue::String(n.clone())) - } - } - SqlExpression::DateTimeConstructor { - year, - month, - day, - hour, - minute, - second, - } => { - // Create a DateTime from the constructor - let naive_date = NaiveDate::from_ymd_opt(*year, *month, *day) - .ok_or_else(|| anyhow::anyhow!("Invalid date: {}-{}-{}", year, month, day))?; - let naive_time = NaiveTime::from_hms_opt( - hour.unwrap_or(0), - minute.unwrap_or(0), - second.unwrap_or(0), - ) - .ok_or_else(|| anyhow::anyhow!("Invalid time"))?; - let naive_datetime = NaiveDateTime::new(naive_date, naive_time); - let datetime = Utc.from_utc_datetime(&naive_datetime); - Ok(ExprValue::DateTime(datetime)) - } - SqlExpression::DateTimeToday { - hour, - minute, - second, - } => { - // Get today's date with optional time - let today = Local::now().date_naive(); - let time = NaiveTime::from_hms_opt( - hour.unwrap_or(0), - minute.unwrap_or(0), - second.unwrap_or(0), - ) - .ok_or_else(|| anyhow::anyhow!("Invalid time"))?; - let naive_datetime = NaiveDateTime::new(today, time); - let datetime = Utc.from_utc_datetime(&naive_datetime); - Ok(ExprValue::DateTime(datetime)) - } - _ => Ok(ExprValue::Null), - } - } - /// Evaluate a CASE expression as a boolean (for WHERE clauses) fn evaluate_case_expression_as_bool( &mut self, @@ -1171,8 +1141,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { )); } - let mut evaluator = crate::data::arithmetic_evaluator::ArithmeticEvaluator::new(self.table); - let value = evaluator.evaluate(expr, row_index)?; + let value = self.evaluate_arithmetic(expr, row_index)?; use crate::data::datatable::DataValue; Ok(match value { @@ -1216,12 +1185,40 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { } } -enum ExprValue { - String(String), - Number(f64), - Boolean(bool), - DateTime(DateTime), - Null, +/// 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 { + matches!( + op_upper, + "=" | "==" + | "!=" + | "<>" + | "<" + | "<=" + | ">" + | ">=" + | "LIKE" + | "IS" + | "IS NOT" + | "IS NULL" + | "IS NOT NULL" + ) +} + +/// A numeric literal as the WHERE path has always read it: whole numbers as +/// `Integer`, others as `Float`, and text that does not parse as a string. +fn number_literal_value(n: &str) -> DataValue { + match n.parse::() { + Ok(num) if num.fract() == 0.0 && num >= i64::MIN as f64 && num <= i64::MAX as f64 => { + DataValue::Integer(num as i64) + } + Ok(num) => DataValue::Float(num), + Err(_) => DataValue::String(n.to_string()), + } +} + +fn datetime_value(naive: NaiveDateTime) -> DataValue { + DataValue::DateTime(naive.format("%Y-%m-%d %H:%M:%S%.3f").to_string()) } #[cfg(test)] @@ -1554,3 +1551,102 @@ mod bare_value_predicate_tests { ); } } + +#[cfg(test)] +mod operand_resolution_tests { + //! Regression tests for P46 -- every WHERE operand resolves to its real + //! value. The right-hand side used to go through a literal-only reader that + //! answered NULL for a column, a CASE or an expression, so any such + //! predicate was UNKNOWN on every row. Asserting the `Trilean` per row keeps + //! UNKNOWN and FALSE apart, which a row count could not. + + use super::*; + use crate::data::datatable::{DataColumn, DataRow}; + use crate::sql::recursive_parser::Parser; + + /// Rows: 0 = (1, 3), 1 = (5, 4), 2 = (6, NULL). + fn pairs() -> DataTable { + let mut table = DataTable::new("t"); + table.add_column(DataColumn::new("a")); + table.add_column(DataColumn::new("b")); + for (a, b) in [(1, Some(3)), (5, Some(4)), (6, None)] { + table + .add_row(DataRow::new(vec![ + DataValue::Integer(a), + b.map_or(DataValue::Null, DataValue::Integer), + ])) + .unwrap(); + } + table + } + + fn try_eval(table: &DataTable, predicate: &str, row: usize) -> Result { + let sql = format!("SELECT * FROM t WHERE {predicate}"); + let statement = Parser::new(&sql).parse().expect("failed to parse"); + let where_clause = statement.where_clause.expect("expected a WHERE clause"); + RecursiveWhereEvaluator::new(table).evaluate(&where_clause, row) + } + + fn eval(table: &DataTable, predicate: &str, row: usize) -> Trilean { + try_eval(table, predicate, row).expect("evaluation failed") + } + + #[test] + fn column_against_column() { + let t = pairs(); + assert_eq!(eval(&t, "a < b", 0), Trilean::True); + assert_eq!(eval(&t, "a < b", 1), Trilean::False); + assert_eq!(eval(&t, "a = a", 1), Trilean::True); + } + + #[test] + fn column_against_null_column_stays_unknown_under_not() { + // The fix must not land as FALSE for a NULL operand: NOT would flip it. + let t = pairs(); + assert_eq!(eval(&t, "a < b", 2), Trilean::Unknown); + assert_eq!(eval(&t, "NOT (a < b)", 2), Trilean::Unknown); + // The arithmetic-operand shape used to answer FALSE here (P48 leaking + // into WHERE through the ArithmeticEvaluator's comparison). + assert_eq!(eval(&t, "NOT (a + 0 < b)", 2), Trilean::Unknown); + } + + #[test] + fn case_and_expression_operands() { + let t = pairs(); + assert_eq!( + eval(&t, "a < CASE WHEN b > 3 THEN 10 ELSE 0 END", 0), + Trilean::False + ); + assert_eq!( + eval(&t, "a < CASE WHEN b > 3 THEN 10 ELSE 0 END", 1), + Trilean::True + ); + assert_eq!(eval(&t, "b = a + 2", 0), Trilean::True); + } + + #[test] + fn between_bounds_and_in_items_resolve_columns() { + let t = pairs(); + assert_eq!(eval(&t, "b BETWEEN a AND 10", 0), Trilean::True); + assert_eq!(eval(&t, "b BETWEEN a AND 10", 1), Trilean::False); + assert_eq!(eval(&t, "b IN (a + 2, 99)", 0), Trilean::True); + assert_eq!(eval(&t, "b IN (a + 2, 99)", 2), Trilean::Unknown); + } + + #[test] + fn a_literal_on_the_left_resolves_too() { + let t = pairs(); + assert_eq!(eval(&t, "3 < a", 1), Trilean::True); + assert_eq!(eval(&t, "3 < a", 0), Trilean::False); + } + + #[test] + fn a_raw_window_operand_errors_rather_than_ranking_unfiltered_rows() { + // An inline QUALIFY reaches here unlifted (P15). Evaluating it would + // rank the rows the WHERE has not yet filtered -- a silent wrong answer. + let t = pairs(); + let err = try_eval(&t, "ROW_NUMBER() OVER (ORDER BY a) = 1", 0) + .expect_err("a raw window operand must not evaluate quietly"); + assert!(err.to_string().contains("ROW_NUMBER"), "got: {err}"); + } +} diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index 6331ef61..d1028f58 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -179,13 +179,16 @@ expect = "GAP" # doubles as coverage that NOT LIKE inherits the fixed NULL semantics; the # evaluator side is already implemented and returns UNKNOWN for a NULL operand. -# --- P46: a column-to-column comparison in WHERE returns zero rows --- +# --- P46: a column-to-column comparison in WHERE returned zero rows --- +# FIXED 2026-09-13. All eight cases below were DIFFER and now AGREE. Every WHERE +# operand (both comparison sides, IN-list items, BETWEEN bounds) now resolves +# through one `evaluate_operand_value`, and the literal-only reader that +# answered NULL for any other shape is gone. [[case]] id = "where_col_lt_col" data = "null_edges.csv" sql = "SELECT id, partner_id FROM null_edges WHERE id < partner_id ORDER BY id" -expect = "DIFFER" # P46, found 2026-09-12 writing examples/countries.sql. Both operands are # columns, and that alone is enough: we return NO rows, DuckDB returns # 1,4,7,9 (the NULL-partner rows are correctly excluded by both). @@ -196,7 +199,6 @@ expect = "DIFFER" id = "where_col_eq_col_string" data = "instruments.csv" sql = "SELECT instrument_id, currency, trading_currency FROM instruments WHERE currency = trading_currency ORDER BY instrument_id LIMIT 5" -expect = "DIFFER" # P46 again, on strings and on equality, to pin that the defect is the # column-vs-column *shape* rather than one operator or one type. DuckDB matches # 57 of the 200 rows; we match none. @@ -211,7 +213,6 @@ expect = "DIFFER" id = "where_col_eq_same_col" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE team = team ORDER BY id" -expect = "DIFFER" # The tell from the P46 entry: the same column on both sides. No type, no # coercion, no data involved. DuckDB keeps every non-NULL team (all but 7, 8). @@ -219,7 +220,6 @@ expect = "DIFFER" id = "where_not_col_lt_col" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE NOT (id < partner_id) ORDER BY id" -expect = "DIFFER" # P46 under NOT. DuckDB: 2, 5, 8 - the NULL-partner rows stay out, because # NOT UNKNOWN is UNKNOWN. Guards the fix against landing as "false" for NULL. @@ -227,7 +227,6 @@ expect = "DIFFER" id = "where_not_expr_lt_col" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE NOT (id + 0 < partner_id) ORDER BY id" -expect = "DIFFER" # The one P46-adjacent shape that did NOT return zero rows: an arithmetic # operand sent the whole comparison to the ArithmeticEvaluator, which answers # false (not NULL) for a NULL operand - P48 leaking into WHERE. Under NOT that @@ -237,7 +236,6 @@ expect = "DIFFER" id = "where_col_lt_case" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE score < CASE WHEN team = 'alpha' THEN 60 ELSE 40 END ORDER BY id" -expect = "DIFFER" # Not a column at all on the right - a CASE - and the same zero rows. DuckDB: # 1, 2, 5, 8, 9 (rows 7/8 have a NULL team, so the WHEN is UNKNOWN and they # take the ELSE bound of 40). @@ -246,7 +244,6 @@ expect = "DIFFER" id = "where_between_col_bound" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE partner_id BETWEEN id AND 10 ORDER BY id" -expect = "DIFFER" # BETWEEN's bounds used the same literal-only reader. DuckDB: 1, 4, 7 # (row 9's partner 11 is above the literal upper bound). @@ -254,7 +251,25 @@ expect = "DIFFER" id = "where_in_list_expr_item" data = "null_edges.csv" sql = "SELECT id FROM null_edges WHERE partner_id IN (id + 2, 5) ORDER BY id" -expect = "DIFFER" # And IN-list items. The literal 5 still matched, so this one hid as a # too-few-rows answer rather than an empty one: we returned only 4, DuckDB # 1, 4, 9. + +[[case]] +id = "where_literal_on_left" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE 3 < id ORDER BY id" +# The mirror of P46: the LEFT operand had to be a column, so a literal there was +# a hard error ("Expected column name, got: NumberLiteral"). Found in the P46 +# re-probe, and reported independently from the countries demo as +# `WHERE 1=0` being rejected while `LIMIT 0` worked. Fixed by the same change - +# both operands now resolve through one path. Was never in the corpus, so it +# lands as AGREE. + +[[case]] +id = "where_constant_true" +data = "null_edges.csv" +sql = "SELECT id FROM null_edges WHERE 1 = 1 AND team = 'beta' ORDER BY id" +# The `WHERE 1=1 AND ...` idiom that generated SQL leans on. Constant-false is +# the more natural probe, but an empty result cannot be compared by the harness +# today (our empty JSON carries no column list), so the true form pins it. diff --git a/tests/comparison/corpus/05_subqueries.toml b/tests/comparison/corpus/05_subqueries.toml index af1c64e9..a2c00d16 100644 --- a/tests/comparison/corpus/05_subqueries.toml +++ b/tests/comparison/corpus/05_subqueries.toml @@ -20,15 +20,22 @@ sql = "SELECT symbol, price FROM trades WHERE price IN (SELECT price FROM trades id = "in_subquery_correlated" data = "trades.csv" sql = "SELECT symbol, price FROM trades t WHERE price IN (SELECT price FROM trades x WHERE x.symbol = t.symbol AND x.price > 185)" -# Correlation not applied: returns empty. See "correlated subqueries" in SQL_PARITY.md. -expect = "DIFFER" +# Correlation not applied (P3). AGREEs since 2026-09-13, BY COINCIDENCE: the +# outer `t.symbol` now binds to the inner table's own `symbol` (P49), so the +# subquery runs uncorrelated as "prices above 185", and no two symbols in +# trades.csv share such a price. Before, the outer reference read as NULL and +# the result was empty. Do not read this AGREE as P3 progress. [[case]] id = "scalar_subquery_correlated" data = "international_sales.csv" sql = "SELECT region, amount FROM international_sales s WHERE amount = (SELECT MAX(amount) FROM international_sales x WHERE x.region = s.region)" -# Correlation not applied: inner sees 0 rows. See "correlated subqueries" in SQL_PARITY.md. -expect = "GAP" +# Correlation not applied (P3). Was GAP ("returned 0 rows") until 2026-09-13; +# now DIFFER, because the outer `s.region` binds to the inner table's `region` +# (P49), `x.region = x.region` holds for every row, and the subquery returns the +# global MAX. A loud error became a wrong answer: see P49 for why that was +# accepted rather than guarded in the P46 fix. +expect = "DIFFER" [[case]] id = "exists_correlated" @@ -48,5 +55,7 @@ expect = "GAP" id = "scalar_subquery_in_select_correlated" data = "international_sales.csv" sql = "SELECT region, country, (SELECT SUM(amount) FROM international_sales x WHERE x.region = s.region) AS region_total FROM international_sales s" -# Correlation not applied in SELECT-list scalar subquery. See SQL_PARITY.md. -expect = "GAP" +# Correlation not applied in SELECT-list scalar subquery (P3). Was GAP until +# 2026-09-13; now DIFFER for the same reason as scalar_subquery_correlated - +# every row gets the grand total (P49). +expect = "DIFFER" diff --git a/tests/comparison/corpus/09_window.toml b/tests/comparison/corpus/09_window.toml index 6134d1a1..177d2713 100644 --- a/tests/comparison/corpus/09_window.toml +++ b/tests/comparison/corpus/09_window.toml @@ -286,6 +286,19 @@ expect = "GAP" # written inline in QUALIFY is never lifted and arrives at the WHERE evaluator # as a raw WindowFunction. Fix site is `expression_lifter`, not QUALIFY. +[[case]] +id = "qualify_inline_with_where" +data = "international_sales.csv" +sql = "SELECT region, amount FROM international_sales WHERE amount < 3000 QUALIFY ROW_NUMBER() OVER (PARTITION BY region ORDER BY amount DESC) = 1 ORDER BY region" +expect = "GAP" +# P15, added 2026-09-13 as a guard. QUALIFY must rank the rows the WHERE kept: +# DuckDB returns Europe 2799.99, North America 2499.99, Oceania 2199.5. During +# the P46 fix the WHERE evaluator briefly learned to evaluate a raw window +# function operand, which made qualify_row_number above AGREE - but ranked the +# UNFILTERED table, so this query returned Oceania alone. The evaluator now +# refuses a raw window operand. If P15 is ever "fixed" by evaluating the window +# inside the WHERE instead of lifting it, this case moves to DIFFER. + [[case]] id = "window_in_where_inline" data = "international_sales.csv" From f061d128d09960ddcc1516571931f7e544fb4a8d Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 13 Sep 2026 09:21:30 +0100 Subject: [PATCH 3/3] docs(parity): close P46, file P49, decouple P48 P46 was wider than filed - every non-literal right-hand WHERE operand read as NULL - and the entry now records the re-probe, the fix, and the two traps it walked into. P49 logs the correlated-subquery trade accepted with it. P48 no longer needs to land with P46 and is projection-only. P15 gains a warning not to fix it inside the WHERE evaluator. R3 gains the catch-all row. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GpuTexUeuyuAdmDsukMTRW --- docs/ENGINE_REFACTORING.md | 1 + docs/SQL_PARITY.md | 189 +++++++++++++++++++++++++++++++++---- 2 files changed, 171 insertions(+), 19 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index d10f9b92..01391b76 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -176,6 +176,7 @@ feature work**, and so we can tell the difference between "this is awkward" and | [P15](SQL_PARITY.md) | An inline window function in `QUALIFY` is never lifted (the lifter walks the SELECT list only) → `Expected column name, got: WindowFunction` | hard error | | *(fixed, PR #33)* | `ILIKE` inside `OVER (ORDER BY ...)` left unrewritten, reaching the executor as an unknown operator | hard error | | *(fixed, PR #33)* | `INTO` inside `(a, b) IN (SELECT ...)` never removed | reaches executor | + | *(fixed 2026-09-13)* [P46](SQL_PARITY.md#p46) | The WHERE evaluator's literal reader ended `_ => Ok(Null)`, so every column, `CASE` or expression on the right of a comparison, in an `IN` list or as a `BETWEEN` bound read as NULL — **zero rows, no error**. Not a walker, but the same catch-all shape one layer down: the arm that should have been a compile-time gap was a value | **wrong results, no error** | P9 is the one that matters most: `HAVING COUNT(*) BETWEEN 1 AND 2` returned 4 rows where DuckDB returns 1, with no error. A catch-all turned a missing diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index f646ae7f..d5d78c8a 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -65,9 +65,9 @@ session apiece to fix properly, so **discovery is paused and the effort moves to picking them off**. Widen the corpus again when the open list is short, or opportunistically when a fix needs a case that doesn't exist yet. -Corpus coverage today: tiers 01–10, **188 cases** (157 AGREE / 16 DIFFER / -12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-12, after [P46](#p46)–[P48](#p48) -added seven — six DIFFER and one AGREE anchor). The largest single movement so far remains the 2026-09-05 +Corpus coverage today: tiers 01–10, **197 cases** (168 AGREE / 15 DIFFER / +11 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-13, after the [P46](#p46) fix +added nine and moved eleven — see that entry and [P49](#p49)). The largest single movement so far remains the 2026-09-05 NULL-ordering slice, which closed [P13](#p13) stage 2 and [P17](#p17) together — eleven cases in one change. **Tier 10 (aggregate & NULL edges) is still deliberately partial** — it holds the P14, P18–P20 and P41 cases and their @@ -91,7 +91,8 @@ Suggested fix order, by silent blast radius: | ~~9c~~ | ~~[P17](#p17) + [P13](#p13) stage 2 — NULL ordering~~ | ✅ **Fixed 2026-09-05** — 141 → **152 AGREE**, eleven cases in one change. Both halves were the same comparator's NULL rule, so they were taken as one slice. The two sorts that disagreed with each other now *share* one function (`compare_for_order_by`), which is the part that stops the divergence recurring; the window site turned out to be sorting NULL as the **maximum** via a derived `PartialOrd`, not merely following a different rule | | ~~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) | -| **NEXT** | [P46](#p46) column-vs-column `WHERE` returns 0 rows, [P47](#p47) `MIN`/`MAX` ranked by type, [P48](#p48) NULL comparison projects `false` | All three found 2026-09-12 writing one examples file, all three silent. P46 is the [P30](#p30) disguise again — zero rows reads as "no data" — and P47 hands back a number wrong by a factor of half a million from an ordinary `GROUP BY`. Both are cheap: P47's correct comparator is already in the tree, and P48 is the [P18](#p18)/[P19](#p19) rule at one more site. P46 and P48 sit in neighbouring code, so take them together | +| ~~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** | [P47](#p47) `MIN`/`MAX` ranked by type, [P48](#p48) NULL comparison projects `false` | Both silent, both cheap. P47 hands back a number wrong by a factor of half a million from an ordinary `GROUP BY`, and its correct comparator is already in the tree. P48 is now projection-only and independent of P46 | | 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 | | 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 | | 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32), [P38](#p38) | Hard errors — visible, so less urgent than any of the above | @@ -129,6 +130,15 @@ worth mining deliberately when the open list runs short: unlike the corpus, it is written to be *interesting* rather than to be exhaustive, which is exactly how it wanders outside the tested envelope. +**A fifth, from fixing P46 (2026-09-13): a case moving the right way still has to +be explained.** The `--check` contract flags *any* drift, and the easy reading of +a GAP → AGREE is "bonus fix". Four cases moved that P46 did not target. One +(`qualify_row_number`) was a wrong answer that happened to agree on a query with +no WHERE; one (`in_subquery_correlated`) agreed by coincidence of the data; two +went from a loud error to a silent wrong answer. Each unexpected move got its +own probe against DuckDB before its `expect` was touched, and two of them +changed the fix. Treat unexplained movement in either direction as a finding. + **A fourth lesson, from closing P41 (2026-09-06): the entry named a fix site, and the fix site was dead code.** `ModeState` in `src/sql/aggregates/mod.rs` is exactly what you find by grepping for MODE, and it is shadowed — a second, newer @@ -240,10 +250,13 @@ annotation be removed. ### P3 — Correlated subqueries do not apply the outer-row correlation - **Status:** 🔴 OPEN — **theme / root cause**, covers several corpus cases -- **Corpus:** `05_subqueries.toml :: in_subquery_correlated` (DIFFER, returns empty), - `scalar_subquery_correlated` (GAP, "returned 0 rows"), - `scalar_subquery_in_select_correlated` (GAP), - `exists_correlated` (GAP), `not_exists_correlated` (GAP). +- **Corpus:** `05_subqueries.toml :: in_subquery_correlated` (AGREE by + coincidence since 2026-09-13; was DIFFER, returned empty), + `scalar_subquery_correlated` (DIFFER since 2026-09-13; was GAP, "returned 0 rows"), + `scalar_subquery_in_select_correlated` (DIFFER since 2026-09-13; was GAP), + `exists_correlated` (GAP), `not_exists_correlated` (GAP). The 2026-09-13 moves + are [P49](#p49), not progress here: outer references now bind to the inner + table instead of reading as NULL. - **Observed:** A subquery referencing an outer column (`WHERE x.region = s.region`) does not see the outer row — it evaluates as if the outer reference is empty, so correlated scalar subqueries error ("0 rows"), correlated `IN` returns an @@ -772,6 +785,14 @@ annotation be removed. QUALIFY to WHERE *first* and the WHERE lifting path, which works, picks it up for free. **Check the alias form against the swap before taking it**: the ordering was chosen for that case, and `WhereAliasExpander` runs after both. +- **Do not fix this in the WHERE evaluator (learned 2026-09-13).** During the + [P46](#p46) fix, operands briefly resolved through the `ArithmeticEvaluator`, + which *can* compute a window — and `qualify_row_number` went AGREE. That is a + trap: the window is then computed over the rows the WHERE is still filtering, + so a QUALIFY combined with a WHERE ranks the unfiltered table (1 row where + DuckDB returns 3). The evaluator now refuses a raw window operand, and + `09_window.toml :: qualify_inline_with_where` (GAP) will turn DIFFER if anyone + takes that route again. The error message moved with it; the fix site did not. ### P16 — `ORDER BY ` is silently ignored - **Status:** 🟢 FIXED 2026-08-31 — 134 → 139 AGREE (+2 fixed, +3 new coverage, @@ -2032,11 +2053,17 @@ out of date. --- ### P46 — A column-to-column comparison in `WHERE` returns zero rows -- **Status:** 🔴 OPEN — silent, and it returns the emptiest possible wrong answer -- **Corpus:** `02_where.toml :: where_col_lt_col`, `where_col_eq_col_string` - (both `expect = "DIFFER"`); the contrast anchor is - `01_select.toml :: select_col_vs_col_projected`, which AGREEs. -- **Observed:** if both sides of a comparison are columns, the predicate never +- **Status:** 🟢 FIXED 2026-09-13 — 157 → **168 AGREE** over 197 cases (+8 fixed, + +2 new coverage, +1 coincidental — see [P49](#p49)). Spun off [P49](#p49). +- **Corpus:** `02_where.toml :: where_col_lt_col`, `where_col_eq_col_string`, + and six added by the re-probe — `where_col_eq_same_col`, + `where_not_col_lt_col`, `where_not_expr_lt_col`, `where_col_lt_case`, + `where_between_col_bound`, `where_in_list_expr_item` (all were DIFFER, now + AGREE). Added with the fix: `where_literal_on_left`, `where_constant_true` + (AGREE); and in tier 09 `qualify_inline_with_where` (GAP, a guard for + [P15](#p15) — see below). The contrast anchor remains + `01_select.toml :: select_col_vs_col_projected`. +- **Observed (was):** if both sides of a comparison are columns, the predicate never matches. Not one operator, not one type — the shape: | Query | Ours | DuckDB 1.5.5 | @@ -2069,14 +2096,82 @@ out of date. into a CTE or `#temp`, then `WHERE frontier = 'cross'`. Arithmetic serves the same purpose when the columns are numeric: project `COUNT('*') - SUM(flag) AS remainder`, then `WHERE remainder = 0`. -- **Decision:** fix. While in there, check the other predicate sites for the +- **Decision (was):** fix. While in there, check the other predicate sites for the same hole — `JOIN ... ON` and `HAVING` both compare operands of their own — and re-probe from a second angle first, per the [P28](#p28) lesson: this entry was written from `WHERE` alone. +- **The re-probe paid for itself: the filed shape was too narrow.** The defect + was not "column vs column" but *any right-hand operand that is not a literal*. + `RecursiveWhereEvaluator::extract_value` understood string, number, boolean + and date literals and ended in `_ => Ok(ExprValue::Null)` — so a column, a + `CASE` or an expression on the right read as NULL, the comparison was UNKNOWN + on every row, and every row was dropped. It served four sites, all broken the + same way: + + | Shape | Was | DuckDB | + |---|---|---| + | `id < partner_id`, `team = team` | 0 rows 🚫 | 1, 4, 7, 9 / all non-NULL | + | `NOT (id < partner_id)` | 0 rows 🚫 | 2, 5, 8 | + | `score < CASE WHEN … END` | 0 rows 🚫 | 1, 2, 5, 8, 9 | + | `partner_id BETWEEN id AND 10` | 0 rows 🚫 | 1, 4, 7 | + | `partner_id IN (id + 2, 5)` | **4 only** 🚫 — the literal still matched | 1, 4, 9 | + | `NOT (id + 0 < partner_id)` | **8 rows** 🚫 — too many | 2, 5, 8 | + | `3 < id`, `WHERE 1=0` | hard error "Expected column name" | ✅ | + + The last two rows are two more holes in the same function. An *arithmetic* + operand dodged `extract_value` by handing the whole comparison to the + `ArithmeticEvaluator`, whose comparison answers `false` for a NULL operand — + [P48](#p48) leaking into `WHERE`, visible only under `NOT`. And the **left** + operand had to be a column, so a literal there errored; `WHERE 1=0` being + rejected while `LIMIT 0` worked was reported from the same countries demo. + `JOIN ... ON` and `HAVING` were probed and are clean. +- **Not the evaluator standing apart — one site that never got the shared + path.** The WHERE evaluator already delegated to the shared pieces: every + comparison goes through `compare_trilean` → `value_comparisons::compare_with_op`, + and `evaluate_operand_value` (added for [P11](#p11)) already resolved the + *left* of `IN`/`BETWEEN` as column-or-`ArithmeticEvaluator`. The right-hand + sides simply still used the older literal-only reader. +- **Fix:** every operand — both sides of a comparison, each `IN` item, both + `BETWEEN` bounds — now resolves through `evaluate_operand_value`, and the + comparison always happens in `compare_trilean`, so the R10 NULL → UNKNOWN rule + holds whatever shape the operands take. `extract_value`, `ExprValue` and the + whole-comparison delegation branch are gone. Columns and literals keep a + direct path; everything else goes to the `ArithmeticEvaluator`. An operator + that yields no truth value (`WHERE a + b`) is a value used as a predicate, per + the [P37](#p37) rule. +- **Two traps the fix walked into, recorded so the next change does not:** + 1. **Performance.** `ArithmeticEvaluator::new` builds the function and both + aggregate registries, and the old branch built one *per row*: + `WHERE price + 0 > quantity` took 9.8 s over 100k rows. Worse, a window + function evaluated that way rebuilt its window context per row — inline + `QUALIFY` over just **2k** rows took 9.5 s. The evaluator now holds one + `ArithmeticEvaluator` for the whole evaluation: 2.9 s and 60 ms. (The + arithmetic operand still costs ~4× a bare column; not chased here.) + 2. **A loud error nearly became a silent wrong answer.** Once operands could + be arbitrary expressions, a raw `WindowFunction` operand *evaluated* — which + made `09_window :: qualify_row_number` AGREE and looked like a free + [P15](#p15) fix. It is not: the window is computed over the rows the WHERE + is still filtering, so `WHERE amount < 3000 QUALIFY ROW_NUMBER() … = 1` + ranked the unfiltered table and returned 1 row where DuckDB returns 3. A raw + window operand is now refused, the same rule `evaluate_value_as_predicate` + already applied, and `qualify_inline_with_where` pins it as GAP. +- **One trade accepted rather than guarded — [P49](#p49).** Resolving right-hand + columns exposed the resolver's unqualified fallback to correlated subqueries: + an outer reference now binds to the inner table's same-named column. Filed + separately; the reasoning is there. +- **Harness note:** an empty result cannot currently be compared — our empty + JSON output carries no column list, so the diff reports + `column mismatch: ref-only cols=[…]` rather than agreeing on zero rows. That is + why `where_constant_true` pins the `1=1` form instead of `1=0`. Every P46 + "0 rows" case showed as a column mismatch for the same reason. +- **Tests:** `recursive_where_evaluator.rs :: operand_resolution_tests` — six + per-row `Trilean` assertions, including the NOT-over-NULL shapes and the + refused window operand, which no row count could distinguish. - **Related:** [P30](#p30) (same zero-rows disguise), [P7](#p7) (a - multi-condition join evaluating two-column operands by position — possibly the - same root, worth checking together), [P48](#p48) (the NULL half of the same - comparison story, found while pinning this one). + multi-condition join evaluating two-column operands by position — single- + condition `ON` probed clean here, multi-condition not re-checked), + [P48](#p48) (its WHERE leak closed by this fix; the projection half is still + open), [P15](#p15), [P37](#p37), [P49](#p49). --- @@ -2181,14 +2276,70 @@ out of date. job, and the unguarded form kept as this entry's case. - **Decision:** fix — the comparison arms of the arithmetic evaluator should yield NULL when either operand is NULL, matching the WHERE evaluator rather - than diverging from it. Small, and it wants doing alongside [P46](#p46), which - is in neighbouring code. + than diverging from it. Small (`arithmetic_evaluator.rs`, the + `">" | "<" | …` arm of `evaluate_binary_op`). +- **Update 2026-09-13 — no longer tied to [P46](#p46).** This was expected to + land alongside P46 because the obvious P46 fix routed WHERE comparisons + through this evaluator. The fix taken instead resolves only the *operands* + there and keeps the comparison in the WHERE evaluator's `compare_trilean`, so + the two became independent — and that also closed the one place this bug + leaked into `WHERE` (`NOT (id + 0 < partner_id)` returned 8 rows, DuckDB 3). + What remains is projection only. Check `CASE`, `AND`/`OR` and `to_bool` still + handle a NULL comparison result when taking it. - **Related:** [P18](#p18), [P19](#p19), [P20](#p20) (`||` treating NULL as an empty string — the same coercion instinct in the string operators), [P46](#p46). --- +### P49 — An unknown table prefix silently falls back to the unqualified column +- **Status:** 🔴 OPEN — silent; accepted knowingly as the cost of the + [P46](#p46) fix +- **Corpus:** `05_subqueries.toml :: scalar_subquery_correlated`, + `scalar_subquery_in_select_correlated` (both GAP → `expect = "DIFFER"`), + `in_subquery_correlated` (DIFFER → AGREE, **by coincidence**). All three are + [P3](#p3) cases; their comments say what moved them. +- **Observed:** `ExecutionContext::resolve_column_index` (`src/data/query_engine.rs`) + resolves `prefix.col` by trying the qualified name and then, if that fails, + **the bare `col`** — whatever the prefix was. So `zz.symbol` against a table + with no `zz` in scope reads `symbol`. That fallback is deliberate (it is what + makes `trades.symbol` work against a table whose columns are unqualified) and + it predates P46: `WHERE zz.symbol = 'AAPL'` already answered on main. +- **Why it surfaced now.** Correlated subqueries are not evaluated per outer row + ([P3](#p3) / [R7](ENGINE_REFACTORING.md#r7)), so in + `WHERE x.region = s.region` the outer `s.region` is resolved against the + *inner* table. Before P46 it sat on the right-hand side, which read as NULL — + the inner query saw no rows and the correlated scalar subqueries failed loudly + ("returned 0 rows"). With right-hand columns resolved, `s.region` binds to + `x`'s own `region`, `x.region = x.region` is true on every row, and the + subquery runs **uncorrelated**: + + | Case | Before P46 fix | After | DuckDB | + |---|---|---|---| + | scalar `amount = (SELECT MAX(amount) … WHERE x.region = s.region)` | error | 1 row (global max) 🚫 | 4 rows | + | `(SELECT SUM(amount) … x.region = s.region)` in SELECT | error | grand total on every row 🚫 | per-region totals | + | `price IN (SELECT price … x.symbol = t.symbol AND x.price > 185)` | 0 rows 🚫 | ✅ matches | — only because no two symbols share such a price | + +- **Why it was accepted, not guarded (decided 2026-09-13).** Three options were + on the table: (1) ship and file this; (2) make the shared resolver reject a + prefix that is neither an alias, the table, nor a qualifier on its columns; + (3) apply that check only to WHERE right-hand operands. (3) would make the two + sides of one comparison resolve by different rules again — the exact + asymmetry P46 removed. (2) is the right shape but changes resolution for every + clause and wants its own probing (CTEs, joins, `#temp`, the generous + `column_resolution_error::prefix_is_in_scope`, which was built for *messages* + and says so). And the queries affected were already wrong — an error instead + of the answer. Option (1), keeping P46 a contained slice. +- **Decision:** fix properly as part of [P3](#p3) — per-row correlation gives + outer references somewhere real to resolve, at which point an unmatched + prefix can be an error. If P3 stays deferred long, (2) is worth doing on its + own; a typo'd alias in a *non*-correlated query reads the wrong table's + column just as quietly. +- **Related:** [P3](#p3), [P46](#p46), [R7](ENGINE_REFACTORING.md#r7), + [R11](ENGINE_REFACTORING.md#r11) (ORDER BY's own copy of the resolver). + +--- + ## Deferred / won't fix (intentional) ### D1 — Recursive CTEs (`WITH RECURSIVE`)