From a3ac6b7c867a5fce3033d3637b4d740a34e718c6 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 19 Jul 2026 12:30:54 +0100 Subject: [PATCH] fix(having): rewrite aggregates nested in BETWEEN / IN / CASE (P9) HavingAliasTransformer's two traversals matched only FunctionCall, BinaryOp and Not before falling into a catch-all, so an aggregate reached through any other operator was never collected, never promoted into SELECT, and never rewritten to its alias. The predicate then silently did not filter -- wrong rows in both directions, no error: HAVING COUNT(*) BETWEEN 1 AND 2 1 row -> 4 rows (under-filters) HAVING COUNT(*) IN (4, 5) 2 rows -> 0 rows (over-filters) HAVING CASE WHEN COUNT(*) > 2 ... 2 rows -> 0 rows Migrate both onto the walk helpers (R2). Each now reads "handle the aggregate, delegate the rest": 74 lines of hand-rolled match become 24, and the catch-all that caused this is gone. The aggregate arm deliberately returns early rather than delegating -- that early return is what preserves the "no nested aggregates" invariant the old code kept by hand. A test pins it. Behaviour check, since map_children descends into WindowSpec::order_by and the old walker did not: the subquery boundary now works in our favour. map_children does not enter a nested SelectStatement, so an aggregate belonging to a subquery's own scope is correctly left for that scope rather than being claimed by the outer HAVING. Corpus: having_between, having_in_list and having_case drop their expect = "DIFFER" and are plain AGREE cases. having_not stays a GAP, exactly as P10 predicted -- evidence the two entries were correctly split. Full gate holds at 83 cases. Closes P9. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ENGINE_REFACTORING.md | 13 +- docs/SQL_PARITY.md | 20 ++- src/query_plan/having_alias_transformer.rs | 168 +++++++++++++-------- tests/comparison/corpus/07_grouping.toml | 11 +- 4 files changed, 139 insertions(+), 73 deletions(-) diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index f5ca53ed..c9119f39 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -85,7 +85,7 @@ feature work**, and so we can tell the difference between "this is awkward" and ### R2 — Branch logic over `SqlExpression` is copy-pasted everywhere - **Status:** 🟡 IN PROGRESS — helpers landed (#31), crossing forms made primitive - (#35), 3 of 11 transformers migrated; 8 outstanding + (#35), 4 of 11 transformers migrated; 7 outstanding - **Where:** `src/query_plan/*.rs`, `src/data/*.rs`, `src/analysis/*.rs` - **Observed:** No traversal abstraction existed. **446 `SqlExpression::` patterns across 40 files**, every consumer @@ -116,13 +116,19 @@ feature work**, and so we can tell the difference between "this is awkward" and closures that each capture `self` mutably. - **Migrated so far:** `cte_hoister`, `into_clause_remover`, `ilike_to_like_transformer` — i.e. exactly the three that cross the scope - boundary. All three now delegate; none names a subquery variant. + boundary; all three now delegate and none names a subquery variant — plus + `having_alias_transformer`, which closed [P9](SQL_PARITY.md). +- **The migration is paying for itself.** Every transformer moved onto `walk` so + far has either fixed a live silent bug or been proven not to have one. P9 is + the clearest case: 74 lines of hand-rolled match became 24, and three corpus + cases went DIFFER → AGREE with no other change. Use it as the template — + *handle the one real rule, return early, delegate everything else*. - **Next, in priority order.** Counts are `SqlExpression::` patterns remaining in each file, as a rough size guide: | Transformer | Patterns | Note | |---|---|---| - | `having_alias_transformer` | 30 | **A fix, not a refactor** — this is [P9](SQL_PARITY.md). Write the failing corpus case *first*. | + | ~~`having_alias_transformer`~~ | ~~30~~ | ✅ Done 2026-07-19 — closed [P9](SQL_PARITY.md). | | `where_alias_expander` | 60 | Largest. Outer-alias scoping is correct only *by omission* — the opaque `map_children` is load-bearing here. | | `group_by_alias_expander` | 34 | Same scoping caveat. | | `order_by_alias_transformer` | 18 | Same scoping caveat. | @@ -274,3 +280,4 @@ R5 dead code ─────── opportunistic | 2026-07-18 | R2 group 1: the three boundary-crossing transformers migrated; two silent bugs fixed | #33 | | 2026-07-18 | R3 evidence: P9–P12 filed after probing the engine; corpus gains tier 07 (grouping) | — | | 2026-07-18 | R2: `*_crossing` helpers become the primitives; the three crossing transformers stop hand-listing subquery variants | #35 | +| 2026-07-19 | R2: `having_alias_transformer` migrated — closes P9, three corpus cases DIFFER → AGREE | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index dfefca3d..7c1339b6 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -213,10 +213,10 @@ annotation be removed. `cargo test` (independent of the DuckDB corpus). ### P9 — `HAVING` silently mishandles an aggregate nested in a non-comparison operator -- **Status:** 🔴 OPEN — **theme**, covers three corpus cases -- **Corpus:** `07_grouping.toml :: having_between` (DIFFER), `having_in_list` - (DIFFER), `having_case` (DIFFER). `having_comparison` / - `having_sum_comparison` AGREE. +- **Status:** 🟢 CLOSED 2026-07-19 — all three corpus cases now AGREE +- **Corpus:** `07_grouping.toml :: having_between`, `having_in_list`, + `having_case` — `expect` dropped, they are plain AGREE cases now. + `having_comparison` / `having_sum_comparison` were already AGREE. - **Observed:** `HavingAliasTransformer` rewrites an aggregate in `HAVING` to reference its computed alias, but its traversal handles only `FunctionCall`, `BinaryOp` and `Not`. An aggregate reached through any other operator is never @@ -243,6 +243,18 @@ annotation be removed. variant. Constraint: the aggregate arm must *not* delegate to the walker, or it would start recursing into aggregate arguments and break the deliberate "no nested aggregates" invariant. +- **Fixed:** 2026-07-19. Both functions now read "handle the aggregate, delegate + the rest" — `collect_aggregates_in_having` returns early on an aggregate then + calls `visit_children`; `rewrite_having_expression` does the same with + `map_children`. 74 lines of hand-rolled match became 24. The constraint above + held: the early return *is* what keeps aggregate arguments untraversed. + Two things worth recording, neither obvious before the migration: + - **The subquery boundary works in our favour.** `map_children` does not + descend into a nested `SelectStatement`, so an aggregate belonging to a + subquery's own scope is correctly left alone — the outer HAVING must not + claim it. The opaque default is load-bearing here, not incidental. + - **`having_not` stayed a GAP**, exactly as P10 predicted. Good evidence the + two entries were correctly split rather than being one finding. ### P10 — `HAVING NOT (...)` errors in the evaluator - **Status:** 🔴 OPEN diff --git a/src/query_plan/having_alias_transformer.rs b/src/query_plan/having_alias_transformer.rs index d5ac694b..f90d910b 100644 --- a/src/query_plan/having_alias_transformer.rs +++ b/src/query_plan/having_alias_transformer.rs @@ -32,6 +32,7 @@ use crate::query_plan::pipeline::ASTTransformer; use crate::sql::parser::ast::{ CTEType, ColumnRef, QuoteStyle, SelectItem, SelectStatement, SqlExpression, TableSource, }; +use crate::sql::parser::walk; use anyhow::Result; use std::collections::HashMap; use tracing::debug; @@ -133,28 +134,23 @@ impl HavingAliasTransformer { } /// Collect all aggregate function calls from a HAVING expression + /// + /// The one real rule is the aggregate itself; everything else is plain + /// traversal. This deliberately does **not** delegate the aggregate arm to + /// the walker: recursing into an aggregate's arguments would break the + /// "no nested aggregates" invariant the old code kept by hand. + /// + /// The old version matched only `BinaryOp`, `Not` and `FunctionCall` before + /// its catch-all, so an aggregate reached through `BETWEEN`, `IN` or `CASE` + /// was never collected and never promoted -- P9, silently wrong rows. fn collect_aggregates_in_having(expr: &SqlExpression, found: &mut Vec) { - match expr { - SqlExpression::FunctionCall { args, .. } if Self::is_aggregate_function(expr) => { - found.push(expr.clone()); - // Don't recurse into aggregate args — nested aggregates aren't supported anyway - let _ = args; - } - SqlExpression::BinaryOp { left, right, .. } => { - Self::collect_aggregates_in_having(left, found); - Self::collect_aggregates_in_having(right, found); - } - SqlExpression::Not { expr } => { - Self::collect_aggregates_in_having(expr, found); - } - SqlExpression::FunctionCall { args, .. } => { - // Non-aggregate function — check its arguments for aggregates - for arg in args { - Self::collect_aggregates_in_having(arg, found); - } - } - _ => {} + if Self::is_aggregate_function(expr) { + found.push(expr.clone()); + return; } + walk::visit_children(expr, |child| { + Self::collect_aggregates_in_having(child, found) + }); } /// Promote aggregates in HAVING that aren't already in SELECT into hidden @@ -192,14 +188,21 @@ impl HavingAliasTransformer { } /// Rewrite a HAVING expression to use aliases instead of aggregates + /// + /// Mirrors [`Self::collect_aggregates_in_having`]: handle the aggregate, + /// delegate the rest, and do not descend into an aggregate's arguments. + /// + /// Note the subquery boundary works in our favour here — `map_children` + /// does not descend into a nested `SelectStatement`, so an aggregate + /// belonging to a subquery's own scope is correctly left alone. fn rewrite_having_expression( - expr: &SqlExpression, + expr: SqlExpression, aggregate_map: &HashMap, ) -> SqlExpression { - match expr { - SqlExpression::FunctionCall { .. } if Self::is_aggregate_function(expr) => { - let normalized = Self::normalize_aggregate_expr(expr); - if let Some(alias) = aggregate_map.get(&normalized) { + if Self::is_aggregate_function(&expr) { + let normalized = Self::normalize_aggregate_expr(&expr); + return match aggregate_map.get(&normalized) { + Some(alias) => { debug!( "Rewriting aggregate {} to column reference {}", normalized, alias @@ -209,37 +212,15 @@ impl HavingAliasTransformer { quote_style: QuoteStyle::None, table_prefix: None, }) - } else { - // Aggregate not found in SELECT - leave as is (will fail later with clear error) - expr.clone() } - } - SqlExpression::BinaryOp { left, op, right } => SqlExpression::BinaryOp { - left: Box::new(Self::rewrite_having_expression(left, aggregate_map)), - op: op.clone(), - right: Box::new(Self::rewrite_having_expression(right, aggregate_map)), - }, - SqlExpression::Not { expr } => SqlExpression::Not { - expr: Box::new(Self::rewrite_having_expression(expr, aggregate_map)), - }, - SqlExpression::FunctionCall { - name, - args, - distinct, - } => { - // Non-aggregate function — recurse into args - SqlExpression::FunctionCall { - name: name.clone(), - args: args - .iter() - .map(|a| Self::rewrite_having_expression(a, aggregate_map)) - .collect(), - distinct: *distinct, - } - } - // For other expressions, return as-is - _ => expr.clone(), + // Aggregate not found in SELECT - leave as is + None => expr, + }; } + + walk::map_children(expr, |child| { + Self::rewrite_having_expression(child, aggregate_map) + }) } /// Transform a SelectStatement and recursively apply to nested statements @@ -301,16 +282,17 @@ impl HavingAliasTransformer { // Step 2: Rewrite HAVING clause to use aliases if let Some(having_expr) = stmt.having.take() { - let rewritten = Self::rewrite_having_expression(&having_expr, &aggregate_map); - if format!("{:?}", having_expr) != format!("{:?}", rewritten) { + // Snapshot for the log line only, so the rewrite can consume the + // expression rather than deep-cloning it at every level. + let before = format!("{having_expr:?}"); + let rewritten = Self::rewrite_having_expression(having_expr, &aggregate_map); + if before != format!("{rewritten:?}") { debug!( "Rewrote HAVING clause with {} aggregate alias(es)", aggregate_map.len() ); - stmt.having = Some(rewritten); - } else { - stmt.having = Some(having_expr); } + stmt.having = Some(rewritten); } } } @@ -417,6 +399,74 @@ mod tests { assert_eq!(transformer.generate_alias(), "__agg_3"); } + /// P9 regression. An aggregate reached through `BETWEEN` / `IN` / `CASE` + /// used to fall into the catch-all: never collected, never promoted, never + /// rewritten — so the predicate silently didn't filter. The corpus pins the + /// row counts against DuckDB; this pins the mechanism, so a regression + /// shows up in `cargo test` and not only in the parity harness. + #[test] + fn rewrites_aggregates_nested_in_non_comparison_operators() { + use crate::sql::recursive_parser::Parser; + + for (label, sql) in [ + ( + "BETWEEN", + "SELECT region, COUNT(*) AS n FROM t GROUP BY region HAVING COUNT(*) BETWEEN 1 AND 2", + ), + ( + "IN", + "SELECT region, COUNT(*) AS n FROM t GROUP BY region HAVING COUNT(*) IN (4, 5)", + ), + ( + "CASE", + "SELECT region, COUNT(*) AS n FROM t GROUP BY region HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1", + ), + ] { + let stmt = Parser::new(sql).parse().expect("should parse"); + let result = HavingAliasTransformer::new() + .transform(stmt) + .expect("transform should succeed"); + + let having = result.having.expect("having clause"); + let mut aggregates_left = 0; + crate::sql::parser::walk::visit_all(&having, &mut |e| { + if HavingAliasTransformer::is_aggregate_function(e) { + aggregates_left += 1; + } + }); + + assert_eq!( + aggregates_left, 0, + "{label}: every aggregate in HAVING must be rewritten to its alias, \ + leaving none behind; got {having:?}" + ); + } + } + + /// The other half of the invariant: an aggregate's *arguments* must stay + /// untraversed, which is why the aggregate arm returns early instead of + /// delegating to the walker. + #[test] + fn does_not_descend_into_aggregate_arguments() { + let inner = SqlExpression::FunctionCall { + name: "COUNT".to_string(), + args: vec![SqlExpression::Column(ColumnRef { + name: "x".to_string(), + quote_style: QuoteStyle::None, + table_prefix: None, + })], + distinct: false, + }; + + let mut found = Vec::new(); + HavingAliasTransformer::collect_aggregates_in_having(&inner, &mut found); + assert_eq!( + found.len(), + 1, + "an aggregate is collected as one unit, not walked into" + ); + } + #[test] fn test_transform_with_no_having() { let mut transformer = HavingAliasTransformer::new(); diff --git a/tests/comparison/corpus/07_grouping.toml b/tests/comparison/corpus/07_grouping.toml index ff52f1b7..7d7864fd 100644 --- a/tests/comparison/corpus/07_grouping.toml +++ b/tests/comparison/corpus/07_grouping.toml @@ -37,26 +37,23 @@ sql = "SELECT region, SUM(amount) AS total FROM international_sales GROUP BY reg id = "having_between" data = "international_sales.csv" sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING COUNT(*) BETWEEN 1 AND 2 ORDER BY region" -# P9: aggregate inside BETWEEN is never rewritten to its alias, so the predicate -# does not filter — silently UNDER-filters and returns groups it should exclude. -expect = "DIFFER" +# Was P9 (silently UNDER-filtered, returning groups it should exclude). +# Closed 2026-07-19 by migrating HavingAliasTransformer onto the walk helpers. [[case]] id = "having_in_list" data = "international_sales.csv" sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING COUNT(*) IN (4, 5) ORDER BY region" -# P9: aggregate inside IN is never rewritten — silently returns 0 rows. +# Was P9 (silently returned 0 rows). Closed 2026-07-19. # NB values must MATCH real group counts (5/10/4/1). An earlier draft used # IN (2, 3), which no group satisfies, so both engines returned 0 rows and the # case passed for the wrong reason. -expect = "DIFFER" [[case]] id = "having_case" data = "international_sales.csv" sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1 ORDER BY region" -# P9: aggregate inside CASE is never rewritten — silently OVER-filters to 0 rows. -expect = "DIFFER" +# Was P9 (silently OVER-filtered to 0 rows). Closed 2026-07-19. [[case]] id = "having_not"