From 60e9acaf07ae7eedf7e76391105dac1c9ff4b9f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 19:32:56 +0000 Subject: [PATCH] Walk every child field of expressions when building subquery scopes WalkInScopeIter::get_children hand-listed the child fields of most node types and left some out, so a query inside them got no subquery scope. An aggregate's FILTER, ORDER BY and LIMIT, a window frame bound, a CAST FORMAT or DEFAULT expression and IN UNNEST(...) were all skipped, and every non-typed aggregate parses to AggregateFunction. Only the arms that enforce scope boundaries stay hand-written: Select (FROM and JOIN items are walked as before, CTEs are not), Table, Subquery, Exists, and In, whose query operand still gets its own scope. Every other node takes its children from the canonical AST traversal, so new fields are covered automatically. Stop rules are unchanged, and the existing pointer dedup keeps each query to a single scope. walk_in_scope and find_all_in_scope now also yield the nodes in those previously skipped positions. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Fa6xBWiFS7JFH5kt1epZ7o --- CHANGELOG.md | 8 ++ crates/polyglot-sql/src/scope.rs | 181 ++++++++++--------------------- 2 files changed, 63 insertions(+), 126 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08265caa..d82263e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,14 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve ### Fixed +- `build_scope` now gives a subquery scope to a query in an expression field + the scope walk skipped: an aggregate's `FILTER (WHERE ...)`, `ORDER BY` and + `LIMIT`, a window frame bound, a `CAST` `FORMAT` or `DEFAULT` expression, + `IN UNNEST(...)`, and the other fields of those nodes it did not list. The + walk now takes the children of these nodes from the AST traversal instead of + a hand-written field list. As a result `walk_in_scope` and `find_all_in_scope` + also yield the nodes in those positions, such as columns in a `FILTER` clause + or window frame bound; the scope boundaries are unchanged. - `build_scope` now gives a subquery scope to a query that is a bare operand of an expression, such as the argument of `ARRAY(SELECT ...)`, instead of leaving its tables and columns out of the scope tree. This matches sqlglot, diff --git a/crates/polyglot-sql/src/scope.rs b/crates/polyglot-sql/src/scope.rs index 812c9cc6..f0d21487 100644 --- a/crates/polyglot-sql/src/scope.rs +++ b/crates/polyglot-sql/src/scope.rs @@ -1395,9 +1395,6 @@ impl<'a> WalkInScopeIter<'a> { let mut children = Vec::new(); match expr { - Expression::Prepare(prepare) => { - children.push(&prepare.statement); - } Expression::Select(select) => { // Walk SELECT expressions for e in &select.expressions { @@ -1455,131 +1452,17 @@ impl<'a> WalkInScopeIter<'a> { children.push(&offset.this); } } - Expression::And(bin) - | Expression::Or(bin) - | Expression::Add(bin) - | Expression::Sub(bin) - | Expression::Mul(bin) - | Expression::Div(bin) - | Expression::Mod(bin) - | Expression::Eq(bin) - | Expression::Neq(bin) - | Expression::Lt(bin) - | Expression::Lte(bin) - | Expression::Gt(bin) - | Expression::Gte(bin) - | Expression::BitwiseAnd(bin) - | Expression::BitwiseOr(bin) - | Expression::BitwiseXor(bin) - | Expression::Concat(bin) => { - children.push(&bin.left); - children.push(&bin.right); - } - Expression::Like(like) | Expression::ILike(like) => { - children.push(&like.left); - children.push(&like.right); - if let Some(escape) = &like.escape { - children.push(escape); - } - } - Expression::Not(un) | Expression::Neg(un) | Expression::BitwiseNot(un) => { - children.push(&un.this); - } - Expression::Function(func) => { - for arg in &func.args { - children.push(arg); - } - } - Expression::AggregateFunction(agg) => { - for arg in &agg.args { - children.push(arg); - } - } - Expression::WindowFunction(wf) => { - children.push(&wf.this); - for e in &wf.over.partition_by { - children.push(e); - } - for e in &wf.over.order_by { - children.push(&e.this); - } - } - Expression::Alias(alias) => { - children.push(&alias.this); - } - Expression::Case(case) => { - if let Some(operand) = &case.operand { - children.push(operand); - } - for (when_expr, then_expr) in &case.whens { - children.push(when_expr); - children.push(then_expr); - } - if let Some(else_clause) = &case.else_ { - children.push(else_clause); - } - } - Expression::Paren(paren) => { - children.push(&paren.this); - } - Expression::Ordered(ord) => { - children.push(&ord.this); - } + Expression::Table(_) | Expression::Subquery(_) | Expression::Exists(_) => {} + // The query operand gets its own scope, so it is not walked here. Expression::In(in_expr) => { - children.push(&in_expr.this); - for e in &in_expr.expressions { - children.push(e); - } - // Note: in_expr.query creates a new scope - don't traverse - } - Expression::Between(between) => { - children.push(&between.this); - children.push(&between.low); - children.push(&between.high); - } - Expression::IsNull(is_null) => { - children.push(&is_null.this); - } - Expression::Cast(cast) => { - children.push(&cast.this); - } - Expression::Extract(extract) => { - children.push(&extract.this); - } - Expression::Coalesce(coalesce) => { - for e in &coalesce.expressions { - children.push(e); - } - } - Expression::NullIf(nullif) => { - children.push(&nullif.this); - children.push(&nullif.expression); - } - Expression::Table(_table) => { - // Tables don't have child expressions to traverse within scope - // (joins are handled at the Select level) - } - Expression::TryCatch(try_catch) => { - for stmt in &try_catch.try_body { - children.push(stmt); - } - if let Some(catch_body) = &try_catch.catch_body { - for stmt in catch_body { - children.push(stmt); - } - } - } - Expression::Column(_) | Expression::Literal(_) | Expression::Identifier(_) => { - // Leaf nodes - no children - } - // Subqueries and Exists create new scopes - don't traverse into them - Expression::Subquery(_) | Expression::Exists(_) => {} - _ => { - // Use the canonical AST traversal for other scalar expressions - // (typed functions, field access, filters, etc.). Scope - // boundaries are still enforced by should_stop_at. - children.extend(expr.children()); + children.extend(expr.children().into_iter().filter(|child| { + !in_expr + .query + .as_ref() + .is_some_and(|query| std::ptr::eq(query, *child)) + })); } + _ => children.extend(expr.children()), } children @@ -2108,6 +1991,52 @@ mod tests { } } + #[test] + fn test_subquery_in_expression_field_is_one_subquery_scope() { + use crate::DialectType::{BigQuery, Generic, Oracle, PostgreSQL}; + + for (dialect, sql) in [ + ( + PostgreSQL, + "SELECT json_agg(u.a) FILTER (WHERE u.a > (SELECT max(t.x) FROM t)) FROM u", + ), + ( + PostgreSQL, + "SELECT json_agg(u.a) FILTER (WHERE u.a = ARRAY(SELECT t.x FROM t)) FROM u", + ), + ( + PostgreSQL, + "SELECT json_agg(u.a ORDER BY (SELECT max(t.x) FROM t)) FROM u", + ), + ( + Generic, + "SELECT SUM(u.a) OVER (ORDER BY u.a ROWS BETWEEN (SELECT max(t.x) FROM t) \ + PRECEDING AND CURRENT ROW) FROM u", + ), + ( + BigQuery, + "SELECT CAST(u.a AS STRING FORMAT ARRAY_TO_STRING(ARRAY(SELECT t.x FROM t), ',')) FROM u", + ), + ( + Oracle, + "SELECT CAST(u.a AS NUMBER DEFAULT (SELECT max(t.x) FROM t) ON CONVERSION ERROR) \ + FROM u", + ), + ( + BigQuery, + "SELECT u.a IN UNNEST(ARRAY(SELECT t.x FROM t)) FROM u", + ), + ] { + let ast = crate::parse(sql, dialect).expect("Failed to parse SQL"); + let scope = build_scope(&ast[0]); + assert_eq!(scope.subquery_scopes.len(), 1, "{sql}"); + let subquery = &scope.subquery_scopes[0]; + assert!(subquery.is_subquery(), "{sql}"); + assert!(subquery.sources.contains_key("t"), "{sql}"); + assert!(!scope.sources.contains_key("t"), "{sql}"); + } + } + #[test] fn test_nested_and_mixed_subquery_operands_register_once() { let scope = parse_and_build_scope(