From 5adadf8f0d5638fc96d4d4c5b6c55a1514f3ef20 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 18 Jul 2026 13:05:21 +0100 Subject: [PATCH] refactor(parser): add exhaustive SqlExpression traversal helpers Groundwork for P3 (correlated subqueries). Purely additive -- nothing calls these yet, so no behaviour changes. Migrating the ~10 query_plan transformers onto them is a separate step. There was no traversal abstraction at all: 446 `SqlExpression::` patterns across 40 files, every consumer hand-rolling its own match over 24 variants. Comparing ilike_to_like_transformer::transform_expression with cte_hoister::hoist_from_expression, the BinaryOp / FunctionCall / CaseExpression arms are byte-for-byte identical apart from the method name -- each has one real rule wrapped in ~50 lines of boilerplate. The copy-paste was already causing silent bugs, since every hand-rolled walker ends in a `_ => {}`: - expression_lifter::extract_column_references misses MethodCall, ChainedMethodCall, Unnest, InSubquery and both tuple subquery variants. - having_alias_transformer::collect_aggregates_in_having handles only FunctionCall, BinaryOp and Not, so an aggregate inside a CASE in a HAVING clause is never found. Adds `map_children` (owned rewrite) and `visit_children` / `visit_all` (borrowing collector), both written with no catch-all arm. Design notes: - Direct children only; callers drive the recursion. That is what collapses a transformer to its one real rule plus `other => walk::map_children(other, |e| self.transform(e))`. - Scope boundary = walker boundary. Subquery statements are NOT descended into: a SELECT alias from the outer query must not be expanded inside a subquery with its own FROM, which is why the alias expanders get this right today only by omission. Same-scope operands of those variants (InSubquery's `expr`, InSubqueryTuple's `exprs`) are still visited. Callers that do want to cross the boundary match those variants explicitly first. - WindowSpec::order_by holds real expressions and IS descended into. Every hand-rolled walker in the tree misses these today; two tests cover it. Verified the exhaustiveness property by temporarily adding the `Exists { negated, subquery }` variant P3 will need: both walk.rs match sites fail with E0004. Only three other matches in the codebase are compiler-guarded today (formatter.rs x2, recursive_parser.rs:569) -- everything else has a catch-all and would have stayed silent. cargo test 680 passed (+8); DuckDB parity contract holds unchanged at 64 AGREE / 1 DIFFER / 7 GAP. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/sql/parser/mod.rs | 1 + src/sql/parser/walk.rs | 425 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 426 insertions(+) create mode 100644 src/sql/parser/walk.rs diff --git a/src/sql/parser/mod.rs b/src/sql/parser/mod.rs index 429f0306..ac9f9b09 100644 --- a/src/sql/parser/mod.rs +++ b/src/sql/parser/mod.rs @@ -10,6 +10,7 @@ pub mod file_cte_parser; pub mod formatter; pub mod legacy; pub mod lexer; +pub mod walk; pub mod web_cte_parser; // Re-export commonly used types for convenience diff --git a/src/sql/parser/walk.rs b/src/sql/parser/walk.rs new file mode 100644 index 00000000..cf9a1506 --- /dev/null +++ b/src/sql/parser/walk.rs @@ -0,0 +1,425 @@ +//! Generic traversal helpers for [`SqlExpression`] trees. +//! +//! Before this module every consumer hand-rolled its own `match expr { ... }` +//! over all 24 expression variants, each ending in a `_ => {}` catch-all. The +//! duplication was not harmless: each copy silently skipped whichever variants +//! its author forgot, so a transformer would quietly no-op on `CASE`, method +//! calls, or tuple subqueries rather than fail. +//! +//! The two helpers here are **exhaustive by construction** — neither has a +//! catch-all arm — so adding a variant to `SqlExpression` becomes a compile +//! error in this file instead of a silent miss spread across the codebase. +//! +//! # Direct children only +//! +//! Both helpers visit a node's *direct* children and do not recurse. Callers +//! drive the recursion, which is what lets a transformer intercept the nodes it +//! cares about and delegate everything else: +//! +//! ```ignore +//! fn transform(&self, expr: SqlExpression) -> SqlExpression { +//! match expr { +//! SqlExpression::BinaryOp { left, op, right } if op == "ILIKE" => { +//! /* the one real rule */ +//! } +//! other => walk::map_children(other, |e| self.transform(e)), +//! } +//! } +//! ``` +//! +//! [`visit_all`] is provided for the common collector case that genuinely wants +//! every node. +//! +//! # Scope boundaries +//! +//! **Subqueries are opaque.** These helpers do not descend into the +//! `SelectStatement` inside `ScalarSubquery`, `InSubquery`, `NotInSubquery`, +//! `InSubqueryTuple` or `NotInSubqueryTuple`, because that statement is a +//! *different query scope*. Descending automatically would be wrong for the +//! alias expanders — a SELECT alias from the outer query must not be expanded +//! inside a subquery that has its own FROM. +//! +//! Same-scope operands of those variants *are* visited: `InSubquery`'s `expr` +//! and `InSubqueryTuple`'s `exprs` belong to the enclosing query, only the +//! `subquery` itself is skipped. +//! +//! A caller that does want to cross the boundary (the CTE hoister, for one) +//! matches those variants explicitly and delegates the rest here. +//! +//! Window specs, by contrast, *are* same-scope: `WindowSpec::order_by` holds +//! real expressions and is descended into. (`partition_by` is `Vec`, +//! so there is nothing to walk.) + +use super::ast::{SimpleWhenBranch, SqlExpression, WhenBranch}; + +/// Rebuild `expr`, replacing each direct child expression with `f(child)`. +/// +/// Leaf nodes are returned unchanged. See the module docs for why subqueries +/// are not descended into. +pub fn map_children( + expr: SqlExpression, + mut f: impl FnMut(SqlExpression) -> SqlExpression, +) -> SqlExpression { + match expr { + // ---- Leaves: nothing to walk ---- + e @ (SqlExpression::Column(_) + | SqlExpression::StringLiteral(_) + | SqlExpression::NumberLiteral(_) + | SqlExpression::BooleanLiteral(_) + | SqlExpression::Null + | SqlExpression::DateTimeConstructor { .. } + | SqlExpression::DateTimeToday { .. }) => e, + + // ---- Scope boundary: the inner statement is deliberately untouched ---- + e @ SqlExpression::ScalarSubquery { .. } => e, + + // ---- Same-scope children ---- + SqlExpression::MethodCall { + object, + method, + args, + } => SqlExpression::MethodCall { + object, + method, + args: args.into_iter().map(&mut f).collect(), + }, + + SqlExpression::ChainedMethodCall { base, method, args } => { + SqlExpression::ChainedMethodCall { + base: Box::new(f(*base)), + method, + args: args.into_iter().map(&mut f).collect(), + } + } + + SqlExpression::FunctionCall { + name, + args, + distinct, + } => SqlExpression::FunctionCall { + name, + args: args.into_iter().map(&mut f).collect(), + distinct, + }, + + SqlExpression::WindowFunction { + name, + args, + mut window_spec, + } => { + let args = args.into_iter().map(&mut f).collect(); + // partition_by is Vec; only order_by carries expressions. + for item in &mut window_spec.order_by { + let taken = std::mem::replace(&mut item.expr, SqlExpression::Null); + item.expr = f(taken); + } + SqlExpression::WindowFunction { + name, + args, + window_spec, + } + } + + SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { + left: Box::new(f(*left)), + op, + right: Box::new(f(*right)), + }, + + SqlExpression::InList { expr, values } => SqlExpression::InList { + expr: Box::new(f(*expr)), + values: values.into_iter().map(&mut f).collect(), + }, + + SqlExpression::NotInList { expr, values } => SqlExpression::NotInList { + expr: Box::new(f(*expr)), + values: values.into_iter().map(&mut f).collect(), + }, + + SqlExpression::Between { expr, lower, upper } => SqlExpression::Between { + expr: Box::new(f(*expr)), + lower: Box::new(f(*lower)), + upper: Box::new(f(*upper)), + }, + + SqlExpression::Not { expr } => SqlExpression::Not { + expr: Box::new(f(*expr)), + }, + + SqlExpression::CaseExpression { + when_branches, + else_branch, + } => SqlExpression::CaseExpression { + when_branches: when_branches + .into_iter() + .map(|b| WhenBranch { + condition: Box::new(f(*b.condition)), + result: Box::new(f(*b.result)), + }) + .collect(), + else_branch: else_branch.map(|e| Box::new(f(*e))), + }, + + SqlExpression::SimpleCaseExpression { + expr, + when_branches, + else_branch, + } => SqlExpression::SimpleCaseExpression { + expr: Box::new(f(*expr)), + when_branches: when_branches + .into_iter() + .map(|b| SimpleWhenBranch { + value: Box::new(f(*b.value)), + result: Box::new(f(*b.result)), + }) + .collect(), + else_branch: else_branch.map(|e| Box::new(f(*e))), + }, + + SqlExpression::Unnest { column, delimiter } => SqlExpression::Unnest { + column: Box::new(f(*column)), + delimiter, + }, + + // ---- Subquery variants: walk the same-scope operand, skip the statement ---- + SqlExpression::InSubquery { expr, subquery } => SqlExpression::InSubquery { + expr: Box::new(f(*expr)), + subquery, + }, + + SqlExpression::NotInSubquery { expr, subquery } => SqlExpression::NotInSubquery { + expr: Box::new(f(*expr)), + subquery, + }, + + SqlExpression::InSubqueryTuple { exprs, subquery } => SqlExpression::InSubqueryTuple { + exprs: exprs.into_iter().map(&mut f).collect(), + subquery, + }, + + SqlExpression::NotInSubqueryTuple { exprs, subquery } => { + SqlExpression::NotInSubqueryTuple { + exprs: exprs.into_iter().map(&mut f).collect(), + subquery, + } + } + } +} + +/// Call `f` on each direct child expression of `expr`. +/// +/// The borrowing counterpart to [`map_children`], for collectors that only read. +/// Same scope rules apply. +pub fn visit_children<'a>(expr: &'a SqlExpression, mut f: impl FnMut(&'a SqlExpression)) { + match expr { + // ---- Leaves: nothing to walk ---- + SqlExpression::Column(_) + | SqlExpression::StringLiteral(_) + | SqlExpression::NumberLiteral(_) + | SqlExpression::BooleanLiteral(_) + | SqlExpression::Null + | SqlExpression::DateTimeConstructor { .. } + | SqlExpression::DateTimeToday { .. } => {} + + // ---- Scope boundary ---- + SqlExpression::ScalarSubquery { .. } => {} + + // ---- Same-scope children ---- + SqlExpression::MethodCall { args, .. } | SqlExpression::FunctionCall { args, .. } => { + args.iter().for_each(f); + } + + SqlExpression::ChainedMethodCall { base, args, .. } => { + f(base); + args.iter().for_each(f); + } + + SqlExpression::WindowFunction { + args, window_spec, .. + } => { + args.iter().for_each(&mut f); + // partition_by is Vec; only order_by carries expressions. + window_spec.order_by.iter().for_each(|item| f(&item.expr)); + } + + SqlExpression::BinaryOp { left, right, .. } => { + f(left); + f(right); + } + + SqlExpression::InList { expr, values } | SqlExpression::NotInList { expr, values } => { + f(expr); + values.iter().for_each(f); + } + + SqlExpression::Between { expr, lower, upper } => { + f(expr); + f(lower); + f(upper); + } + + SqlExpression::Not { expr } | SqlExpression::Unnest { column: expr, .. } => f(expr), + + SqlExpression::CaseExpression { + when_branches, + else_branch, + } => { + for branch in when_branches { + f(&branch.condition); + f(&branch.result); + } + if let Some(e) = else_branch { + f(e); + } + } + + SqlExpression::SimpleCaseExpression { + expr, + when_branches, + else_branch, + } => { + f(expr); + for branch in when_branches { + f(&branch.value); + f(&branch.result); + } + if let Some(e) = else_branch { + f(e); + } + } + + // ---- Subquery variants: same-scope operand only ---- + SqlExpression::InSubquery { expr, .. } | SqlExpression::NotInSubquery { expr, .. } => { + f(expr) + } + + SqlExpression::InSubqueryTuple { exprs, .. } + | SqlExpression::NotInSubqueryTuple { exprs, .. } => exprs.iter().for_each(f), + } +} + +/// Call `f` on `expr` and every descendant, pre-order. +/// +/// The usual entry point for collectors ("find every column reference", +/// "find every aggregate"). Subquery statements are still not descended into — +/// see the module docs. +pub fn visit_all<'a>(expr: &'a SqlExpression, f: &mut impl FnMut(&'a SqlExpression)) { + f(expr); + visit_children(expr, |child| visit_all(child, f)); +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::sql::parser::ast::ColumnRef; + use crate::sql::recursive_parser::Parser; + + /// Parse `SELECT FROM t` and hand back the projected expression. + fn expr_of(select_expr: &str) -> SqlExpression { + let sql = format!("SELECT {select_expr} FROM t"); + let stmt = Parser::new(&sql) + .parse() + .unwrap_or_else(|e| panic!("{sql} should parse: {e}")); + + stmt.select_items + .into_iter() + .find_map(|item| match item { + crate::sql::parser::ast::SelectItem::Expression { expr, .. } => Some(expr), + _ => None, + }) + .expect("expected a projected expression") + } + + /// Collect every column name reachable from `expr`. + fn columns(expr: &SqlExpression) -> Vec { + let mut found = Vec::new(); + visit_all(expr, &mut |e| { + if let SqlExpression::Column(c) = e { + found.push(c.name.clone()); + } + }); + found + } + + /// Rename every column reference, recursing via `map_children`. + fn rename_all(expr: SqlExpression, to: &str) -> SqlExpression { + match expr { + SqlExpression::Column(_) => SqlExpression::Column(ColumnRef::unquoted(to.to_string())), + other => map_children(other, |e| rename_all(e, to)), + } + } + + #[test] + fn visit_all_reaches_case_branches() { + let expr = expr_of("CASE WHEN a > 1 THEN b ELSE c END"); + let mut found = columns(&expr); + found.sort(); + assert_eq!(found, vec!["a", "b", "c"]); + } + + #[test] + fn visit_all_reaches_nested_function_args() { + let expr = expr_of("UPPER(TRIM(name))"); + assert_eq!(columns(&expr), vec!["name"]); + } + + #[test] + fn visit_all_reaches_between_operands() { + let expr = expr_of("x BETWEEN lo AND hi"); + assert_eq!(columns(&expr), vec!["x", "lo", "hi"]); + } + + /// The payoff case: `WindowSpec::order_by` holds real expressions, and + /// every hand-rolled walker in the codebase missed them. + #[test] + fn visit_all_reaches_window_order_by() { + let expr = expr_of("ROW_NUMBER() OVER (ORDER BY created_at)"); + assert!( + columns(&expr).contains(&"created_at".to_string()), + "window ORDER BY expressions must be reachable" + ); + } + + /// Subqueries are a scope boundary: the operand is walked, the inner + /// statement is not. + #[test] + fn walkers_do_not_cross_into_subqueries() { + let expr = expr_of("(SELECT MAX(inner_col) FROM other)"); + assert!( + matches!(expr, SqlExpression::ScalarSubquery { .. }), + "expected a scalar subquery" + ); + assert!( + columns(&expr).is_empty(), + "must not descend into a subquery's own scope" + ); + + // ...but a same-scope operand alongside one still is. + let stmt = Parser::new("SELECT a FROM t WHERE outer_col IN (SELECT x FROM other)") + .parse() + .expect("should parse"); + let cond = &stmt.where_clause.expect("where clause").conditions[0].expr; + assert_eq!(columns(cond), vec!["outer_col"]); + } + + #[test] + fn map_children_rewrites_nested_expressions() { + let expr = expr_of("CASE WHEN a > 1 THEN UPPER(b) ELSE c END"); + let renamed = rename_all(expr, "z"); + assert_eq!(columns(&renamed), vec!["z", "z", "z"]); + } + + #[test] + fn map_children_rewrites_window_order_by() { + let expr = expr_of("ROW_NUMBER() OVER (ORDER BY created_at)"); + let renamed = rename_all(expr, "z"); + assert_eq!(columns(&renamed), vec!["z"]); + } + + #[test] + fn map_children_leaves_leaves_alone() { + let expr = expr_of("42"); + let mapped = map_children(expr, |_| panic!("a literal has no children")); + assert!(matches!(mapped, SqlExpression::NumberLiteral(ref n) if n == "42")); + } +}