fix(having): rewrite aggregates nested in BETWEEN / IN / CASE (P9)

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::<Variant>` 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 | — |

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

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<SqlExpression>) {
-        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<String, String>,
     ) -> 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();

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"