diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index f2a4bd7b..6f064a65 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -292,20 +292,44 @@ annotation be removed. matching DuckDB. ### P11 — A `SELECT` alias is not expanded on the LHS of an `IN` subquery -- **Status:** 🔴 OPEN -- **Corpus:** `02_where.toml :: select_alias_in_in_subquery` (GAP) -- **Observed:** `SELECT symbol, price * 2 AS dbl FROM trades WHERE dbl IN +- **Status:** 🟢 FIXED (2026-07-25) +- **Corpus:** `02_where.toml :: select_alias_in_in_subquery` (now AGREE; `expect` + dropped) +- **Observed (was):** `SELECT symbol, price * 2 AS dbl FROM trades WHERE dbl IN (SELECT ...)` → `Column 'dbl' not found`. The same alias resolves fine as the - LHS of a plain comparison or an `IN`-list, so this is specific to the subquery - form. -- **Root cause:** [R3](ENGINE_REFACTORING.md). `WhereAliasExpander` never matches - `InSubquery` / `NotInSubquery` / the tuple forms, so the **same-scope** LHS - operand is skipped along with the subquery. Note the expander is right not to - descend into the subquery *body* (different scope) — the bug is that it drops - the operand too. -- **Decision:** **Fix** via the R2 migration. `walk` visits `InSubquery`'s `expr` - while still treating the nested statement as a scope boundary, which is exactly - the required behaviour. + LHS of a plain comparison or an `IN`-list, so this looked specific to the + subquery form. +- **Root cause (documented):** [R3](ENGINE_REFACTORING.md). `WhereAliasExpander` + never matched `InSubquery` / `NotInSubquery` / the tuple forms, so the + **same-scope** LHS operand was skipped along with the subquery. +- **Second, undocumented layer (found while fixing):** the alias fix alone was + *necessary but not sufficient*. Once `dbl` expands to `price * 2`, the LHS is a + compound expression — and `price * 2 IN (SELECT ...)` **fails even with no + alias at all**. The `InOperatorLifter` transformer normally lifts an + expression-LHS `IN`-list into a computed CTE column, but it only matches + `InList` / `NotInList`; an `IN`-**subquery**'s `InList` is synthesized *later* + by `SubqueryExecutor`, after the lifter has run, so `evaluate_in_list` received + a raw expression LHS and its `extract_column_name` errored. This was a + pre-existing bug the alias case merely exposed. +- **Fix (two parts):** + 1. **R2 migration of `WhereAliasExpander`.** Rewrote `expand_expression` to + intercept only the two nodes that carry a real rule (a bare column naming an + alias; a method-call *string* receiver the walker can't reach) and delegate + all structural recursion to `walk::map_children`. That retired ~230 lines of + hand-rolled arms **and** the `_ => (clone, false)` catch-all, and picked up + the four subquery-LHS variants for free — the walker visits their same-scope + operands while treating the nested `SelectStatement` as an opaque scope + boundary (so a `dbl` inside the subquery body is correctly left alone). + Regression: `where_alias_expander.rs :: + test_expands_alias_on_in_subquery_lhs_not_body`. + 2. **Expression LHS in `evaluate_in_list` / `evaluate_between`.** Added + `RecursiveWhereEvaluator::evaluate_operand_value`, which looks up a plain + column but evaluates any other expression through the `ArithmeticEvaluator` + — the same delegation `evaluate_binary_op` already does for its LHS. Both + `IN` and `BETWEEN` now accept a compound LHS. +- **Note:** the ideal long-term home for part 2 is the [R7](ENGINE_REFACTORING.md) + per-row subquery evaluation; this is the correct localized fix until then, and + it stands on its own (it fixes alias-free `expr IN (subquery)` too). ### P12 — `WITH` is rejected in expression position - **Status:** 🟢 FIXED (2026-07-25) diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index e73e3a18..328a8d03 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -737,6 +737,35 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { } } + /// 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. + fn evaluate_operand_value( + &self, + expr: &SqlExpression, + 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(|| anyhow::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)?)) + } + } + } + fn evaluate_in_list( &self, expr: &SqlExpression, @@ -744,13 +773,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { row_index: usize, _ignore_case: bool, ) -> Result { - 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 cell_value = self.table.get_value(row_index, col_index).cloned(); + let cell_value = self.evaluate_operand_value(expr, row_index)?; for value_expr in values { let compare_value = self.extract_value(value_expr)?; @@ -773,13 +796,7 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { upper: &SqlExpression, row_index: usize, ) -> Result { - 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 cell_value = self.table.get_value(row_index, col_index).cloned(); + let cell_value = self.evaluate_operand_value(expr, row_index)?; let lower_value = self.extract_value(lower)?; let upper_value = self.extract_value(upper)?; diff --git a/src/query_plan/where_alias_expander.rs b/src/query_plan/where_alias_expander.rs index 09b92880..b26a01ad 100644 --- a/src/query_plan/where_alias_expander.rs +++ b/src/query_plan/where_alias_expander.rs @@ -34,6 +34,7 @@ use crate::query_plan::pipeline::ASTTransformer; use crate::sql::parser::ast::{SelectItem, SelectStatement, SqlExpression}; +use crate::sql::parser::walk; use anyhow::Result; use std::collections::HashMap; use tracing::debug; @@ -66,16 +67,26 @@ impl WhereAliasExpander { aliases } - /// Recursively expand aliases in an expression - /// Returns the expanded expression and whether any expansion occurred + /// Recursively expand aliases in an expression. + /// Returns the expanded expression and whether any expansion occurred. + /// + /// Only two node kinds carry a real rule; everything else is pure structural + /// recursion, so it is delegated to [`walk::map_children`]. That helper is + /// exhaustive by construction and treats a nested subquery's `SelectStatement` + /// as an **opaque scope boundary** — it descends into the same-scope operands + /// of `InSubquery` / `NotInSubquery` / the tuple forms (fixing P11: an alias + /// on the LHS of `x IN (SELECT ...)`) while never reaching into the subquery + /// body, where an outer alias must not leak. Before this migration those + /// variants fell into a `_ => (clone, false)` catch-all and the LHS operand + /// was silently skipped along with the subquery. fn expand_expression( expr: &SqlExpression, aliases: &HashMap, ) -> (SqlExpression, bool) { match expr { - // Check if this column reference is actually an alias + // Rule 1: a bare (un-prefixed) column reference that names a SELECT + // alias is replaced by that alias's expression. SqlExpression::Column(col_ref) => { - // Only expand if it's a simple column (no table prefix) if col_ref.table_prefix.is_none() { if let Some(alias_expr) = aliases.get(&col_ref.name) { debug!( @@ -88,209 +99,10 @@ impl WhereAliasExpander { (expr.clone(), false) } - // Recursively expand binary operations - SqlExpression::BinaryOp { left, op, right } => { - let (new_left, left_expanded) = Self::expand_expression(left, aliases); - let (new_right, right_expanded) = Self::expand_expression(right, aliases); - let expanded = left_expanded || right_expanded; - - ( - SqlExpression::BinaryOp { - left: Box::new(new_left), - op: op.clone(), - right: Box::new(new_right), - }, - expanded, - ) - } - - // Expand in NOT expressions - SqlExpression::Not { expr: inner } => { - let (new_expr, expanded) = Self::expand_expression(inner, aliases); - ( - SqlExpression::Not { - expr: Box::new(new_expr), - }, - expanded, - ) - } - - // Expand in function arguments - SqlExpression::FunctionCall { - name, - args, - distinct, - } => { - let mut expanded = false; - let new_args: Vec = args - .iter() - .map(|arg| { - let (new_arg, arg_expanded) = Self::expand_expression(arg, aliases); - expanded = expanded || arg_expanded; - new_arg - }) - .collect(); - - ( - SqlExpression::FunctionCall { - name: name.clone(), - args: new_args, - distinct: *distinct, - }, - expanded, - ) - } - - // Expand in IN list expressions - SqlExpression::InList { - expr: inner, - values, - } => { - let (new_expr, expr_expanded) = Self::expand_expression(inner, aliases); - let mut expanded = expr_expanded; - - let new_values: Vec = values - .iter() - .map(|val| { - let (new_val, val_expanded) = Self::expand_expression(val, aliases); - expanded = expanded || val_expanded; - new_val - }) - .collect(); - - ( - SqlExpression::InList { - expr: Box::new(new_expr), - values: new_values, - }, - expanded, - ) - } - - // Expand in NOT IN list expressions - SqlExpression::NotInList { - expr: inner, - values, - } => { - let (new_expr, expr_expanded) = Self::expand_expression(inner, aliases); - let mut expanded = expr_expanded; - - let new_values: Vec = values - .iter() - .map(|val| { - let (new_val, val_expanded) = Self::expand_expression(val, aliases); - expanded = expanded || val_expanded; - new_val - }) - .collect(); - - ( - SqlExpression::NotInList { - expr: Box::new(new_expr), - values: new_values, - }, - expanded, - ) - } - - // Expand in BETWEEN expressions - SqlExpression::Between { expr, lower, upper } => { - let (new_expr, expr_expanded) = Self::expand_expression(expr, aliases); - let (new_lower, lower_expanded) = Self::expand_expression(lower, aliases); - let (new_upper, upper_expanded) = Self::expand_expression(upper, aliases); - let expanded = expr_expanded || lower_expanded || upper_expanded; - - ( - SqlExpression::Between { - expr: Box::new(new_expr), - lower: Box::new(new_lower), - upper: Box::new(new_upper), - }, - expanded, - ) - } - - // Expand in CASE expressions - SqlExpression::CaseExpression { - when_branches, - else_branch, - } => { - let mut expanded = false; - let new_branches: Vec<_> = when_branches - .iter() - .map(|branch| { - let (new_condition, cond_expanded) = - Self::expand_expression(&branch.condition, aliases); - let (new_result, result_expanded) = - Self::expand_expression(&branch.result, aliases); - expanded = expanded || cond_expanded || result_expanded; - - crate::sql::parser::ast::WhenBranch { - condition: Box::new(new_condition), - result: Box::new(new_result), - } - }) - .collect(); - - let new_else = else_branch.as_ref().map(|e| { - let (new_e, else_expanded) = Self::expand_expression(e, aliases); - expanded = expanded || else_expanded; - Box::new(new_e) - }); - - ( - SqlExpression::CaseExpression { - when_branches: new_branches, - else_branch: new_else, - }, - expanded, - ) - } - - // Expand in simple CASE expressions - SqlExpression::SimpleCaseExpression { - expr, - when_branches, - else_branch, - } => { - let (new_expr, expr_expanded) = Self::expand_expression(expr, aliases); - let mut expanded = expr_expanded; - - let new_branches: Vec<_> = when_branches - .iter() - .map(|branch| { - let (new_value, value_expanded) = - Self::expand_expression(&branch.value, aliases); - let (new_result, result_expanded) = - Self::expand_expression(&branch.result, aliases); - expanded = expanded || value_expanded || result_expanded; - - crate::sql::parser::ast::SimpleWhenBranch { - value: Box::new(new_value), - result: Box::new(new_result), - } - }) - .collect(); - - let new_else = else_branch.as_ref().map(|e| { - let (new_e, else_expanded) = Self::expand_expression(e, aliases); - expanded = expanded || else_expanded; - Box::new(new_e) - }); - - ( - SqlExpression::SimpleCaseExpression { - expr: Box::new(new_expr), - when_branches: new_branches, - else_branch: new_else, - }, - expanded, - ) - } - - // Expand in method calls, e.g. `alias.Contains('x')`. - // The receiver is a bare column-name string, so an alias can only be - // substituted if it resolves to a simple (un-prefixed) column. + // Rule 2: a method call's receiver is a bare column-name *string*, not + // a child expression, so the walker can't reach it. Substitute the + // receiver here when it names an alias resolving to a simple column, + // then recurse into the args normally. SqlExpression::MethodCall { object, method, @@ -328,32 +140,18 @@ impl WhereAliasExpander { ) } - // Expand in chained method calls, e.g. `(alias).Trim().Contains('x')`. - // The base is itself an expression, so recurse into it normally. - SqlExpression::ChainedMethodCall { base, method, args } => { - let (new_base, base_expanded) = Self::expand_expression(base, aliases); - let mut expanded = base_expanded; - let new_args: Vec = args - .iter() - .map(|arg| { - let (new_arg, arg_expanded) = Self::expand_expression(arg, aliases); - expanded = expanded || arg_expanded; - new_arg - }) - .collect(); - - ( - SqlExpression::ChainedMethodCall { - base: Box::new(new_base), - method: method.clone(), - args: new_args, - }, - expanded, - ) + // Everything else: structural recursion via the walker. It visits + // same-scope children (including subquery LHS operands) and leaves + // subquery bodies opaque. + other => { + let mut expanded = false; + let new_expr = walk::map_children(other.clone(), |child| { + let (new_child, child_expanded) = Self::expand_expression(&child, aliases); + expanded = expanded || child_expanded; + new_child + }); + (new_expr, expanded) } - - // For all other expressions, return as-is - _ => (expr.clone(), false), } } @@ -625,6 +423,53 @@ mod tests { )); } + #[test] + fn test_expands_alias_on_in_subquery_lhs_not_body() { + // P11: `WHERE dbl IN (SELECT ...)` where `dbl` aliases `price * 2`. + // The walker migration must expand the same-scope LHS operand while + // leaving the subquery body (a different scope) untouched. + let double = SqlExpression::BinaryOp { + left: Box::new(SqlExpression::Column(ColumnRef::unquoted("price".into()))), + op: "*".to_string(), + right: Box::new(SqlExpression::NumberLiteral("2".to_string())), + }; + let aliases = HashMap::from([("dbl".to_string(), double.clone())]); + + // The subquery body also references `dbl` — it must NOT be expanded, + // because that name belongs to the subquery's own scope. + let body = SelectStatement { + where_clause: Some(WhereClause { + conditions: vec![Condition { + expr: SqlExpression::Column(ColumnRef::unquoted("dbl".into())), + connector: None, + }], + }), + ..Default::default() + }; + + let expr = SqlExpression::InSubquery { + expr: Box::new(SqlExpression::Column(ColumnRef::unquoted("dbl".into()))), + subquery: Box::new(body.clone()), + }; + + let (expanded, changed) = WhereAliasExpander::expand_expression(&expr, &aliases); + assert!(changed, "the LHS alias should have been expanded"); + + match expanded { + SqlExpression::InSubquery { expr, subquery } => { + // LHS expanded to the aliased expression. + assert!(matches!(expr.as_ref(), SqlExpression::BinaryOp { .. })); + // Subquery body left verbatim: still the bare `dbl` column. + let inner = &subquery.where_clause.as_ref().unwrap().conditions[0].expr; + assert!( + matches!(inner, SqlExpression::Column(c) if c.name == "dbl"), + "subquery body must not be touched (different scope), got {inner:?}" + ); + } + other => panic!("expected InSubquery, got {other:?}"), + } + } + #[test] fn test_does_not_expand_table_prefixed_columns() { let aliases = HashMap::from([( diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index afe9ae4f..78c8a8be 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -74,7 +74,10 @@ sql = "SELECT symbol FROM trades WHERE symbol IN (SELECT symbol FROM trades WHER id = "select_alias_in_in_subquery" data = "trades.csv" sql = "SELECT symbol, price * 2 AS dbl FROM trades WHERE dbl IN (SELECT price * 2 FROM trades WHERE price > 185)" -# P11: a SELECT alias used as the LHS of an IN-subquery is never expanded, so -# the query errors with "Column 'dbl' not found". The same alias works fine as -# the LHS of a plain comparison or an IN-list. -expect = "GAP" +# Was P11: a SELECT alias on the LHS of an IN-subquery errored ("Column 'dbl' +# not found"). Fixed 2026-07-25 in two layers: (1) WhereAliasExpander migrated +# onto walk::map_children, which visits the subquery LHS operand (the alias) +# while leaving the subquery body opaque; (2) the substituted IN-list's LHS may +# be an arbitrary expression (price*2), which the IN-subquery path never lifted, +# so evaluate_in_list/between now evaluate an expression LHS via the arithmetic +# evaluator. Now AGREE.