You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "Dhruv-meesho (via GitHub)" <gi...@apache.org> on 2026/09/02 20:19:10 UTC
[PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Dhruv-meesho opened a new pull request, #12960:
URL: https://github.com/apache/gluten/pull/12960
Fixes #12959.
## What changes are proposed in this pull request?
`FlushableHashAggregateRule` decides an aggregate is intermediate via:
```scala
agg.aggregateExpressions.forall(p => p.mode == Partial || p.mode == PartialMerge)
```
For a grouping-only aggregate (`SELECT DISTINCT`, or `GROUP BY` with no aggregate functions), `aggregateExpressions` is empty (`Seq.empty`), so `forall` is vacuously `true`. The check cannot distinguish the partial stage from the final (or complete) stage. The final stage is therefore converted into `FlushableHashAggregateExecTransformer`, which sets `allowFlush = true` and maps to `AggregationNode::Step::kPartial` in Velox.
When Velox abandons aggregation (based on memory/cardinality heuristics), it emits duplicate grouping keys. Because this is the final aggregation stage, no subsequent aggregate collapses the duplicates, resulting in silent duplicate rows.
The rule only visits aggregates below a shuffle exchange, so a plain `SELECT DISTINCT` whose result is consumed directly is not affected. However, when the distinct output is repartitioned again (e.g., when feeding a join on a strict subset of the distinct columns), the final aggregate enters the rule's scope and gets converted.
### History & Context
- This case was previously covered by `isAggInputAlreadyDistributedWithAggKeys`, added in #4443 (for #4421).
- It regressed in #12098 when the rule was narrowed to only protect against `AggUtils.planAggregateWithOneDistinct`.
### Fix
Rather than restoring the broader distribution check and giving up the performance gains of #12098, this PR introduces a targeted guard `isGroupingOnlyFinalAgg`:
```scala
private def isGroupingOnlyFinalAgg(agg: HashAggregateExecTransformer): Boolean = {
agg.aggregateExpressions.isEmpty && agg.requiredChildDistributionExpressions.isDefined
}
```
This skips conversion only when `aggregateExpressions` is empty and `requiredChildDistributionExpressions.isDefined` (`None` for partial aggregates, `Some(groupingAttributes)` for final aggregates in `AggUtils.planAggregateWithoutDistinct`). This matches the condition Spark checks in `HashAggregateExec.adaptivePartialAggEnabled` (SPARK-58511). Aggregates with aggregate functions are unaffected and preserve the behavior from #12098.
## How was this patch tested?
- Added regression test `flushable aggregate rule - distinct feeding a join keeps final agg regular` to `VeloxAggregateFunctionsFlushSuite`.
- Verified that joining on a strict subset of distinct keys leaves the final distinct aggregate as `RegularHashAggregateExecTransformer` and produces output identical to vanilla Spark.
- Verified style check passed via `./dev/format-scala-code.sh`.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-3-7-sonnet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "Dhruv-meesho (via GitHub)" <gi...@apache.org>.
Dhruv-meesho commented on code in PR #12960:
URL: https://github.com/apache/gluten/pull/12960#discussion_r3921828140
##########
backends-velox/src/main/scala/org/apache/gluten/extension/FlushableHashAggregateRule.scala:
##########
@@ -70,12 +70,32 @@ case class FlushableHashAggregateRule(session: SparkSession) extends Rule[SparkP
aggExprs.exists(isUnsupportedAggregation)
}
+ /**
+ * Returns true if the aggregate applies no aggregate functions and is the final (or complete)
+ * stage, e.g. the last step of `SELECT DISTINCT`, or of a `GROUP BY` without aggregate functions.
+ *
+ * Such an aggregate must fully aggregate. Flushing lets Velox abandon aggregation and emit
+ * duplicate grouping keys, and since no further aggregate follows, the duplicates reach the
+ * consumer. When the consumer is a join, they turn into extra join output rows.
+ *
+ * The mode check used by the other guards cannot detect this case: `aggregateExpressions` is
+ * empty here, so `forall(_.mode == Partial | PartialMerge)` is vacuously true and says nothing
+ * about which stage this is. Spark distinguishes the stages with
+ * `requiredChildDistributionExpressions`, which is `None` for a partial aggregate and
+ * `Some(groupingAttributes)` for the final one -- see `AggUtils.planAggregateWithoutDistinct`.
+ * Spark makes the same check in its own guard for the equivalent runtime bypass, see
+ * `HashAggregateExec.adaptivePartialAggEnabled`.
+ */
Review Comment:
Trimmed, thanks. @zhztheplayer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "Dhruv-meesho (via GitHub)" <gi...@apache.org>.
Dhruv-meesho commented on PR #12960:
URL: https://github.com/apache/gluten/pull/12960#issuecomment-5524218414
Hi @zhztheplayer @kevinwilfong @LuciferYang can we please merge this....all the checks have been successfully passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #12960:
URL: https://github.com/apache/gluten/pull/12960#discussion_r3920464804
##########
backends-velox/src/main/scala/org/apache/gluten/extension/FlushableHashAggregateRule.scala:
##########
@@ -70,12 +70,32 @@ case class FlushableHashAggregateRule(session: SparkSession) extends Rule[SparkP
aggExprs.exists(isUnsupportedAggregation)
}
+ /**
+ * Returns true if the aggregate applies no aggregate functions and is the final (or complete)
+ * stage, e.g. the last step of `SELECT DISTINCT`, or of a `GROUP BY` without aggregate functions.
+ *
+ * Such an aggregate must fully aggregate. Flushing lets Velox abandon aggregation and emit
+ * duplicate grouping keys, and since no further aggregate follows, the duplicates reach the
+ * consumer. When the consumer is a join, they turn into extra join output rows.
+ *
+ * The mode check used by the other guards cannot detect this case: `aggregateExpressions` is
+ * empty here, so `forall(_.mode == Partial | PartialMerge)` is vacuously true and says nothing
+ * about which stage this is. Spark distinguishes the stages with
+ * `requiredChildDistributionExpressions`, which is `None` for a partial aggregate and
+ * `Some(groupingAttributes)` for the final one -- see `AggUtils.planAggregateWithoutDistinct`.
+ * Spark makes the same check in its own guard for the equivalent runtime bypass, see
+ * `HashAggregateExec.adaptivePartialAggEnabled`.
+ */
Review Comment:
nit: Can we only keep the 1st paragraph of the comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer merged PR #12960:
URL: https://github.com/apache/gluten/pull/12960
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "Dhruv-meesho (via GitHub)" <gi...@apache.org>.
Dhruv-meesho commented on PR #12960:
URL: https://github.com/apache/gluten/pull/12960#issuecomment-5521683451
<!-- ctxm-scan-marker
sha: 67ed775d6f9f791c235a9ef7e4306e6f858092b9
result: clean
scanned_at: 2026-09-03T06:43:41+00:00
-->
🟢 **Context Maintainer** scanned commit `67ed775d6f9f` — 13 pre-existing drifts on the baseline, no new drift on this commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "Dhruv-meesho (via GitHub)" <gi...@apache.org>.
Dhruv-meesho commented on PR #12960:
URL: https://github.com/apache/gluten/pull/12960#issuecomment-5516041485
cc @zhztheplayer @yikaifei for review when you have time, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12959][VL] Keep the final stage of a grouping-only aggregate non-flushable [gluten]
Posted by "Dhruv-meesho (via GitHub)" <gi...@apache.org>.
Dhruv-meesho commented on PR #12960:
URL: https://github.com/apache/gluten/pull/12960#issuecomment-5521810424
Thanks for the quick review!
The spark40 / spark41 shard-3 failures were the TPC-DS plan stability checks, on the same 13 queries #12098 updated — expected, since this puts those final aggregates back to regular. I've updated the approved plans for both spark versions; the partial stage under the shuffle is untouched and still flushes. I diffed the updated simplified.txt files against the actual plans printed in the last CI run's failure output and all 26 match exactly.
Also trimmed the comment to the first paragraph as suggested.
Looks like the new workflow runs are waiting for approval — Can you please trigger them when you get a chance?
Thanks!
cc:- @ArnavBalyan @zhztheplayer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org