diff --git a/axiom/optimizer/tests/sql/window.sql b/axiom/optimizer/tests/sql/window.sql index cfcee14a8..37a728f99 100644 --- a/axiom/optimizer/tests/sql/window.sql +++ b/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 diff --git a/axiom/sql/presto/GroupByPlanner.cpp b/axiom/sql/presto/GroupByPlanner.cpp index c0ae4bc7e..0f7bbcb68 100644 --- a/axiom/sql/presto/GroupByPlanner.cpp +++ b/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); } diff --git a/axiom/sql/presto/tests/AggregationParserTest.cpp b/axiom/sql/presto/tests/AggregationParserTest.cpp index 636616beb..a35edcbfc 100644 --- a/axiom/sql/presto/tests/AggregationParserTest.cpp +++ b/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) {