fix(parser): Apply HAVING before window functions

axiom/optimizer/tests/sql/window.sql

@@ -289,3 +289,12 @@ FROM (
     GROUP BY d
 ) AS t
 CROSS JOIN UNNEST(vals) AS n(v)
+----
+-- HAVING drops a group before the window functions run, so a window nested in
+-- an expression sums only the groups that survive it.
+SELECT a, count(*) AS c,
+    1.0 * count(*) / sum(count(*)) OVER (PARTITION BY a) AS frac
+FROM (VALUES (1, 1), (1, 1), (1, 2),
+             (2, 3), (2, 3), (2, 4)) AS t(a, b)
+GROUP BY a, b
+HAVING count(*) > 1

axiom/sql/presto/GroupByPlanner.cpp

@@ -519,11 +519,6 @@ void GroupByPlanner::plan(
   // are included in the project node. Must happen before builder_->project().
   auto sortingKeyOrdinals = resolveSortOrdinals(orderBy);
 
-  // Apply HAVING filter, then project.
-  if (filter_.has_value()) {
-    builder_->filter(filter_.value());
-  }
-
   if (!isIdentityProjection()) {
     builder_->project(projections_);
   }
@@ -829,6 +824,25 @@ void GroupByPlanner::rewritePostAggregateExprs() {
     return planMarkers(rewriteIExpr(expr), exprPlanner_, planned);
   };
 
+  // HAVING takes the same two steps as the clauses below, but a column it
+  // cannot resolve is an error rather than a name to leave alone. It is
+  // applied here, before any window function: a window sees only the groups
+  // HAVING keeps.
+  if (filter_.has_value()) {
+    auto substituted = replaceInputs(
+        filter_.value().expr(),
+        keyInputs,
+        aggregateInputs,
+        [](const core::FieldAccessExpr& expr) {
+          VELOX_USER_FAIL(
+              "HAVING clause cannot reference column: {}", expr.name());
+        });
+    filter_ = lp::ExprApi(
+        planMarkers(std::move(substituted), exprPlanner_, planned),
+        filter_->alias());
+    builder_->filter(filter_.value());
+  }
+
   // Project nested window functions (e.g. sum(sum(a)) OVER () inside
   // sum(a) / sum(sum(a)) OVER ()) and add replacements to keyInputs. These
   // reach PlanBuilder here rather than through the rewrite below, so they are
@@ -852,22 +866,6 @@ void GroupByPlanner::rewritePostAggregateExprs() {
     }
   };
 
-  // HAVING takes the same two steps as the clauses below, but a column it
-  // cannot resolve is an error rather than a name to leave alone.
-  if (filter_.has_value()) {
-    auto substituted = replaceInputs(
-        filter_.value().expr(),
-        keyInputs,
-        aggregateInputs,
-        [](const core::FieldAccessExpr& expr) {
-          VELOX_USER_FAIL(
-              "HAVING clause cannot reference column: {}", expr.name());
-        });
-    filter_ = lp::ExprApi(
-        planMarkers(std::move(substituted), exprPlanner_, planned),
-        filter_->alias());
-  }
-
   for (auto& item : projections_) {
     rewriteExpr(item);
   }

axiom/sql/presto/tests/AggregationParserTest.cpp

@@ -76,6 +76,17 @@ TEST_F(AggregationParserTest, aggregateInWindowFrameBound) {
       matchScan("nation").aggregate().project().output());
 }
 
+// HAVING is applied before the window functions, so a window nested in an
+// expression aggregates only the rows HAVING keeps.
+TEST_F(AggregationParserTest, havingBeforeWindow) {
+  connector_->addTable("t", ROW({"a", "b"}, BIGINT()));
+
+  testSelect(
+      "SELECT a, count(*) / sum(count(*)) OVER (PARTITION BY a) "
+      "FROM t GROUP BY a, b HAVING count(*) > 1",
+      matchScan("t").aggregate().filter().project().project().output());
+}
+
 // On a scalar function, DISTINCT is ignored while FILTER and ORDER BY are
 // rejected, matching Presto.
 TEST_F(AggregationParserTest, modifiersOnScalarFunction) {