diff --git a/src/query_plan/cte_hoister.rs b/src/query_plan/cte_hoister.rs index 4ca60df6..d0038e0b 100644 --- a/src/query_plan/cte_hoister.rs +++ b/src/query_plan/cte_hoister.rs @@ -1,6 +1,7 @@ use crate::sql::parser::ast::{ - CTEType, Condition, SelectItem, SelectStatement, SqlExpression, WhereClause, CTE, + CTEType, SelectItem, SelectStatement, SqlExpression, WhereClause, CTE, }; +use crate::sql::parser::walk; use std::collections::{HashMap, HashSet}; /// CTE Hoister - Analyzes and rewrites nested CTEs @@ -123,113 +124,57 @@ impl CTEHoister { } /// Hoist CTEs from an expression + /// + /// The only real rule is the subquery arms: recurse into the nested + /// statement so its CTEs get pulled up to the top level. They stay + /// explicit because [`walk::map_children`] treats a subquery statement as + /// a scope boundary -- crossing it is precisely this transformer's job. fn hoist_from_expression(&mut self, expr: SqlExpression) -> SqlExpression { match expr { - SqlExpression::ScalarSubquery { query } => { - let rewritten = self.hoist_from_statement(*query); - SqlExpression::ScalarSubquery { - query: Box::new(rewritten), - } - } - SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { - left: Box::new(self.hoist_from_expression(*left)), - op, - right: Box::new(self.hoist_from_expression(*right)), - }, - SqlExpression::FunctionCall { - name, - args, - distinct, - } => SqlExpression::FunctionCall { - name, - args: args - .into_iter() - .map(|arg| self.hoist_from_expression(arg)) - .collect(), - distinct, - }, - SqlExpression::CaseExpression { - when_branches, - else_branch, - } => SqlExpression::CaseExpression { - when_branches: when_branches - .into_iter() - .map(|branch| crate::sql::parser::ast::WhenBranch { - condition: Box::new(self.hoist_from_expression(*branch.condition)), - result: Box::new(self.hoist_from_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(self.hoist_from_expression(*e))), + SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { + query: Box::new(self.hoist_from_statement(*query)), }, - SqlExpression::InList { expr, values } => SqlExpression::InList { + SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { expr: Box::new(self.hoist_from_expression(*expr)), - values: values - .into_iter() - .map(|e| self.hoist_from_expression(e)) - .collect(), + subquery: Box::new(self.hoist_from_statement(*subquery)), }, - SqlExpression::NotInList { expr, values } => SqlExpression::NotInList { + SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { expr: Box::new(self.hoist_from_expression(*expr)), - values: values + subquery: Box::new(self.hoist_from_statement(*subquery)), + }, + // Defensive, not a demonstrable fix: the tuple forms fell into the + // old catch-all, but the parser currently rejects WITH anywhere in + // expression position ("Tuple IN requires a subquery on the right"), + // so no input reaches these arms today. Kept for symmetry with the + // other subquery arms, which are equally unreachable for the same + // reason. + SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { + exprs: exprs .into_iter() .map(|e| self.hoist_from_expression(e)) .collect(), + subquery: Box::new(self.hoist_from_statement(*subquery)), }, - SqlExpression::InSubquery { expr, subquery } => { - let rewritten = self.hoist_from_statement(*subquery); - SqlExpression::InSubquery { - expr: Box::new(self.hoist_from_expression(*expr)), - subquery: Box::new(rewritten), - } - } - SqlExpression::NotInSubquery { expr, subquery } => { - let rewritten = self.hoist_from_statement(*subquery); - SqlExpression::NotInSubquery { - expr: Box::new(self.hoist_from_expression(*expr)), - subquery: Box::new(rewritten), + SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + SqlExpression::NotInSubqueryTuple { + exprs: exprs + .into_iter() + .map(|e| self.hoist_from_expression(e)) + .collect(), + subquery: Box::new(self.hoist_from_statement(*subquery)), } } - SqlExpression::Between { expr, lower, upper } => SqlExpression::Between { - expr: Box::new(self.hoist_from_expression(*expr)), - lower: Box::new(self.hoist_from_expression(*lower)), - upper: Box::new(self.hoist_from_expression(*upper)), - }, - SqlExpression::Not { expr } => SqlExpression::Not { - expr: Box::new(self.hoist_from_expression(*expr)), - }, - // For other expression types that might contain subqueries - SqlExpression::SimpleCaseExpression { - expr, - when_branches, - else_branch, - } => SqlExpression::SimpleCaseExpression { - expr: Box::new(self.hoist_from_expression(*expr)), - when_branches: when_branches - .into_iter() - .map(|branch| crate::sql::parser::ast::SimpleWhenBranch { - value: Box::new(self.hoist_from_expression(*branch.value)), - result: Box::new(self.hoist_from_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(self.hoist_from_expression(*e))), - }, - // Terminal expressions don't contain subqueries - other => other, + other => walk::map_children(other, |e| self.hoist_from_expression(e)), } } - /// Hoist CTEs from WHERE clause + /// Recursively hoist from a WHERE clause fn hoist_from_where_clause(&mut self, where_clause: &mut WhereClause) { for condition in &mut where_clause.conditions { condition.expr = self.hoist_from_expression(condition.expr.clone()); } } - /// Recursively hoist from a condition - fn hoist_from_condition(&mut self, condition: &mut Condition) { - condition.expr = self.hoist_from_expression(condition.expr.clone()); - } - /// Add a CTE to the hoisted collection fn add_cte(&mut self, cte: CTE) { // Track dependencies for proper ordering @@ -291,67 +236,34 @@ impl CTEHoister { } /// Find CTE references in an expression + /// + /// The only real rule is the subquery arms: descend into the nested + /// statement and look for CTE references there. Those must stay explicit + /// because [`walk::visit_children`] treats a subquery statement as a scope + /// boundary and will not enter it. Everything else is plain traversal. fn find_cte_refs_in_expression(&self, expr: &SqlExpression, deps: &mut HashSet) { match expr { SqlExpression::ScalarSubquery { query } => { self.find_cte_references(query, deps); } - SqlExpression::InSubquery { subquery, .. } => { - self.find_cte_references(subquery, deps); - } - SqlExpression::NotInSubquery { subquery, .. } => { - self.find_cte_references(subquery, deps); - } - SqlExpression::FunctionCall { args, .. } => { - for arg in args { - self.find_cte_refs_in_expression(arg, deps); - } - } - SqlExpression::BinaryOp { left, right, .. } => { - self.find_cte_refs_in_expression(left, deps); - self.find_cte_refs_in_expression(right, deps); - } - SqlExpression::CaseExpression { - when_branches, - else_branch, - } => { - for branch in when_branches { - self.find_cte_refs_in_expression(&branch.condition, deps); - self.find_cte_refs_in_expression(&branch.result, deps); - } - if let Some(else_expr) = else_branch { - self.find_cte_refs_in_expression(else_expr, deps); - } - } - SqlExpression::SimpleCaseExpression { - expr, - when_branches, - else_branch, - } => { + SqlExpression::InSubquery { expr, subquery } + | SqlExpression::NotInSubquery { expr, subquery } => { self.find_cte_refs_in_expression(expr, deps); - for branch in when_branches { - self.find_cte_refs_in_expression(&branch.value, deps); - self.find_cte_refs_in_expression(&branch.result, deps); - } - if let Some(else_expr) = else_branch { - self.find_cte_refs_in_expression(else_expr, deps); - } + self.find_cte_references(subquery, deps); } - SqlExpression::InList { expr, values } | SqlExpression::NotInList { expr, values } => { - self.find_cte_refs_in_expression(expr, deps); - for value in values { - self.find_cte_refs_in_expression(value, deps); + // Tuple forms were missing from the old catch-all. Unlike the + // hoisting path these ARE reachable: the subquery need not contain + // a WITH, only a reference to an already-hoisted CTE. + SqlExpression::InSubqueryTuple { exprs, subquery } + | SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + for e in exprs { + self.find_cte_refs_in_expression(e, deps); } + self.find_cte_references(subquery, deps); } - SqlExpression::Between { expr, lower, upper } => { - self.find_cte_refs_in_expression(expr, deps); - self.find_cte_refs_in_expression(lower, deps); - self.find_cte_refs_in_expression(upper, deps); - } - SqlExpression::Not { expr } => { - self.find_cte_refs_in_expression(expr, deps); + other => { + walk::visit_children(other, |child| self.find_cte_refs_in_expression(child, deps)) } - _ => {} } } diff --git a/src/query_plan/ilike_to_like_transformer.rs b/src/query_plan/ilike_to_like_transformer.rs index 8afc335c..33262768 100644 --- a/src/query_plan/ilike_to_like_transformer.rs +++ b/src/query_plan/ilike_to_like_transformer.rs @@ -32,9 +32,9 @@ use crate::query_plan::pipeline::ASTTransformer; use crate::sql::parser::ast::{ - CTEType, Condition, OrderByItem, SelectItem, SelectStatement, SimpleWhenBranch, SqlExpression, - WhenBranch, WhereClause, CTE, + CTEType, Condition, OrderByItem, SelectItem, SelectStatement, SqlExpression, WhereClause, CTE, }; +use crate::sql::parser::walk; use anyhow::Result; use tracing::debug; @@ -68,68 +68,13 @@ impl ILikeToLikeTransformer { } } - // Recursively transform nested expressions - SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { - left: Box::new(self.transform_expression(*left)), - op, - right: Box::new(self.transform_expression(*right)), - }, - - SqlExpression::FunctionCall { - name, - args, - distinct, - } => SqlExpression::FunctionCall { - name, - args: args - .into_iter() - .map(|arg| self.transform_expression(arg)) - .collect(), - distinct, - }, - - SqlExpression::CaseExpression { - when_branches, - else_branch, - } => SqlExpression::CaseExpression { - when_branches: when_branches - .into_iter() - .map(|branch| WhenBranch { - condition: Box::new(self.transform_expression(*branch.condition)), - result: Box::new(self.transform_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(self.transform_expression(*e))), - }, - - SqlExpression::SimpleCaseExpression { - expr, - when_branches, - else_branch, - } => SqlExpression::SimpleCaseExpression { - expr: Box::new(self.transform_expression(*expr)), - when_branches: when_branches - .into_iter() - .map(|branch| SimpleWhenBranch { - value: Box::new(self.transform_expression(*branch.value)), - result: Box::new(self.transform_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(self.transform_expression(*e))), - }, - - SqlExpression::Between { expr, lower, upper } => SqlExpression::Between { - expr: Box::new(self.transform_expression(*expr)), - lower: Box::new(self.transform_expression(*lower)), - upper: Box::new(self.transform_expression(*upper)), - }, - - SqlExpression::InList { expr, values } => SqlExpression::InList { - expr: Box::new(self.transform_expression(*expr)), - values: values - .into_iter() - .map(|v| self.transform_expression(v)) - .collect(), + // Subqueries are a scope boundary for `walk::map_children`, which + // will not descend into a nested statement. ILIKE -> LIKE is + // scope-independent, so this transformer deliberately crosses that + // boundary and these arms stay explicit. Delegating them would + // silently stop ILIKE being rewritten inside subqueries. + SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { + query: Box::new(self.transform_statement(*query)), }, SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { @@ -137,71 +82,34 @@ impl ILikeToLikeTransformer { subquery: Box::new(self.transform_statement(*subquery)), }, - SqlExpression::NotInList { expr, values } => SqlExpression::NotInList { + SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { expr: Box::new(self.transform_expression(*expr)), - values: values - .into_iter() - .map(|v| self.transform_expression(v)) - .collect(), + subquery: Box::new(self.transform_statement(*subquery)), }, - SqlExpression::MethodCall { - object, - method, - args, - } => SqlExpression::MethodCall { - object, - method, - args: args + // Previously missing: the tuple forms fell into the hand-rolled + // catch-all, so neither the LHS operands nor the subquery were + // transformed at all. + SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { + exprs: exprs .into_iter() - .map(|arg| self.transform_expression(arg)) + .map(|e| self.transform_expression(e)) .collect(), + subquery: Box::new(self.transform_statement(*subquery)), }, - SqlExpression::ChainedMethodCall { base, method, args } => { - SqlExpression::ChainedMethodCall { - base: Box::new(self.transform_expression(*base)), - method, - args: args + SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + SqlExpression::NotInSubqueryTuple { + exprs: exprs .into_iter() - .map(|arg| self.transform_expression(arg)) + .map(|e| self.transform_expression(e)) .collect(), + subquery: Box::new(self.transform_statement(*subquery)), } } - SqlExpression::Not { expr } => SqlExpression::Not { - expr: Box::new(self.transform_expression(*expr)), - }, - - SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { - query: Box::new(self.transform_statement(*query)), - }, - - SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { - expr: Box::new(self.transform_expression(*expr)), - subquery: Box::new(self.transform_statement(*subquery)), - }, - - SqlExpression::WindowFunction { - name, - args, - window_spec, - } => SqlExpression::WindowFunction { - name, - args: args - .into_iter() - .map(|arg| self.transform_expression(arg)) - .collect(), - window_spec, - }, - - SqlExpression::Unnest { column, delimiter } => SqlExpression::Unnest { - column: Box::new(self.transform_expression(*column)), - delimiter, - }, - - // Literals and simple expressions don't need transformation - _ => expr, + // Everything else is plain traversal. + other => walk::map_children(other, |e| self.transform_expression(e)), } } @@ -368,6 +276,86 @@ impl ASTTransformer for ILikeToLikeTransformer { mod tests { use super::*; use crate::sql::parser::ast::{ColumnRef, QuoteStyle}; + use crate::sql::recursive_parser::Parser; + + /// Render just enough of an expression to assert on operators, so these + /// tests don't depend on the exact AST shape. + fn ops_in(expr: &SqlExpression) -> Vec { + let mut ops = Vec::new(); + crate::sql::parser::walk::visit_all(expr, &mut |e| { + if let SqlExpression::BinaryOp { op, .. } = e { + ops.push(op.clone()); + } + }); + ops + } + + /// Regression for the walk.rs migration. + /// + /// `WindowSpec::order_by` holds real expressions, and the old hand-rolled + /// walker passed `window_spec` through untouched — so an ILIKE inside + /// `OVER (ORDER BY ...)` was silently left as ILIKE and would reach the + /// executor as an unknown operator. + #[test] + fn transforms_ilike_inside_window_order_by() { + let stmt = Parser::new( + "SELECT ROW_NUMBER() OVER (ORDER BY CASE WHEN name ILIKE '%a%' THEN 1 ELSE 0 END) AS rn FROM t", + ) + .parse() + .expect("query should parse"); + + let result = ILikeToLikeTransformer::new().transform_statement(stmt); + + let expr = result + .select_items + .iter() + .find_map(|i| match i { + SelectItem::Expression { expr, .. } => Some(expr), + _ => None, + }) + .expect("expected a projected expression"); + + let ops = ops_in(expr); + assert!( + !ops.iter().any(|o| o == "ILIKE"), + "ILIKE inside a window ORDER BY must be rewritten, found ops: {ops:?}" + ); + assert!( + ops.iter().any(|o| o == "LIKE"), + "expected a LIKE after rewriting, found ops: {ops:?}" + ); + } + + /// The tuple subquery forms fell into the old catch-all, so neither the + /// LHS operands nor the subquery body were transformed. + #[test] + fn transforms_ilike_inside_tuple_subquery() { + let stmt = Parser::new( + "SELECT a FROM t WHERE (a, b) IN (SELECT x, y FROM u WHERE note ILIKE '%z%')", + ) + .parse() + .expect("query should parse"); + + let result = ILikeToLikeTransformer::new().transform_statement(stmt); + + let cond = &result.where_clause.expect("where clause").conditions[0].expr; + let inner = match cond { + SqlExpression::InSubqueryTuple { subquery, .. } => subquery, + other => panic!("expected a tuple IN subquery, got {other:?}"), + }; + let inner_cond = &inner + .where_clause + .as_ref() + .expect("inner where clause") + .conditions[0] + .expr; + + let ops = ops_in(inner_cond); + assert!( + !ops.iter().any(|o| o == "ILIKE"), + "ILIKE inside a tuple subquery must be rewritten, found ops: {ops:?}" + ); + } #[test] fn test_ilike_simple() { diff --git a/src/query_plan/into_clause_remover.rs b/src/query_plan/into_clause_remover.rs index e0c5c49b..7ddeeb48 100644 --- a/src/query_plan/into_clause_remover.rs +++ b/src/query_plan/into_clause_remover.rs @@ -1,4 +1,5 @@ -use crate::sql::parser::ast::SelectStatement; +use crate::sql::parser::ast::{SelectStatement, SqlExpression}; +use crate::sql::parser::walk; /// INTO Clause Remover - Removes INTO clause from AST for execution /// @@ -106,11 +107,17 @@ impl IntoClauseRemover { } /// Remove INTO from expressions (handles subqueries) - fn remove_from_expression( - expr: crate::sql::parser::ast::SqlExpression, - ) -> crate::sql::parser::ast::SqlExpression { - use crate::sql::parser::ast::SqlExpression; - + /// + /// The only real rule here is about subqueries: every nested + /// `SelectStatement` needs its `into_table` cleared. Everything else is + /// plain traversal, delegated to [`walk::map_children`]. + /// + /// The subquery arms must stay explicit. `map_children` treats a subquery + /// statement as a **scope boundary** and deliberately does not descend into + /// it — correct for the alias expanders, but exactly what this transformer + /// has to do. Delegating them would silently stop INTO being removed from + /// nested queries. + fn remove_from_expression(expr: SqlExpression) -> SqlExpression { match expr { SqlExpression::ScalarSubquery { query } => SqlExpression::ScalarSubquery { query: Box::new(Self::remove_from_statement(*query)), @@ -123,75 +130,25 @@ impl IntoClauseRemover { expr: Box::new(Self::remove_from_expression(*expr)), subquery: Box::new(Self::remove_from_statement(*subquery)), }, - SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { - left: Box::new(Self::remove_from_expression(*left)), - op, - right: Box::new(Self::remove_from_expression(*right)), - }, - SqlExpression::FunctionCall { - name, - args, - distinct, - } => SqlExpression::FunctionCall { - name, - args: args - .into_iter() - .map(|arg| Self::remove_from_expression(arg)) - .collect(), - distinct, - }, - SqlExpression::CaseExpression { - when_branches, - else_branch, - } => SqlExpression::CaseExpression { - when_branches: when_branches - .into_iter() - .map(|branch| crate::sql::parser::ast::WhenBranch { - condition: Box::new(Self::remove_from_expression(*branch.condition)), - result: Box::new(Self::remove_from_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(Self::remove_from_expression(*e))), - }, - SqlExpression::SimpleCaseExpression { - expr, - when_branches, - else_branch, - } => SqlExpression::SimpleCaseExpression { - expr: Box::new(Self::remove_from_expression(*expr)), - when_branches: when_branches - .into_iter() - .map(|branch| crate::sql::parser::ast::SimpleWhenBranch { - value: Box::new(Self::remove_from_expression(*branch.value)), - result: Box::new(Self::remove_from_expression(*branch.result)), - }) - .collect(), - else_branch: else_branch.map(|e| Box::new(Self::remove_from_expression(*e))), - }, - SqlExpression::InList { expr, values } => SqlExpression::InList { - expr: Box::new(Self::remove_from_expression(*expr)), - values: values - .into_iter() - .map(|e| Self::remove_from_expression(e)) - .collect(), - }, - SqlExpression::NotInList { expr, values } => SqlExpression::NotInList { - expr: Box::new(Self::remove_from_expression(*expr)), - values: values + // Previously missing: the tuple forms fell into the catch-all, so + // `WHERE (a, b) IN (SELECT ... INTO #t ...)` kept its INTO clause. + SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { + exprs: exprs .into_iter() - .map(|e| Self::remove_from_expression(e)) + .map(Self::remove_from_expression) .collect(), + subquery: Box::new(Self::remove_from_statement(*subquery)), }, - SqlExpression::Between { expr, lower, upper } => SqlExpression::Between { - expr: Box::new(Self::remove_from_expression(*expr)), - lower: Box::new(Self::remove_from_expression(*lower)), - upper: Box::new(Self::remove_from_expression(*upper)), - }, - SqlExpression::Not { expr } => SqlExpression::Not { - expr: Box::new(Self::remove_from_expression(*expr)), - }, - // Terminal expressions don't contain subqueries - other => other, + SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + SqlExpression::NotInSubqueryTuple { + exprs: exprs + .into_iter() + .map(Self::remove_from_expression) + .collect(), + subquery: Box::new(Self::remove_from_statement(*subquery)), + } + } + other => walk::map_children(other, Self::remove_from_expression), } } } @@ -201,6 +158,40 @@ mod tests { use super::*; use crate::sql::parser::ast::IntoTable; + /// Regression for the walk.rs migration: the tuple subquery forms used to + /// fall into the hand-rolled catch-all, so an INTO inside + /// `(a, b) IN (SELECT ...)` was never removed and would reach the executor. + /// + /// Parsed rather than hand-built so the AST is one the parser actually + /// produces (see R4 in docs/ENGINE_REFACTORING.md). + #[test] + fn removes_into_inside_tuple_subquery() { + use crate::sql::recursive_parser::Parser; + + let stmt = Parser::new("SELECT a FROM t WHERE (a, b) IN (SELECT x, y FROM u INTO #inner)") + .parse() + .expect("query should parse"); + + // Precondition: the parser really did put an INTO on the inner query. + let inner_into = |s: &SelectStatement| match &s.where_clause { + Some(w) => match &w.conditions[0].expr { + SqlExpression::InSubqueryTuple { subquery, .. } => subquery.into_table.clone(), + other => panic!("expected a tuple IN subquery, got {other:?}"), + }, + None => panic!("expected a where clause"), + }; + assert!( + inner_into(&stmt).is_some(), + "test is meaningless unless the inner query starts with an INTO" + ); + + let result = IntoClauseRemover::remove_into_clause(stmt); + assert!( + inner_into(&result).is_none(), + "INTO must be removed from inside a tuple subquery" + ); + } + #[test] fn test_remove_simple_into() { let stmt = SelectStatement {