Copilot commented on code in PR #12653:
URL: https://github.com/apache/gluten/pull/12653#discussion_r3674258988
##########
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/MergeTwoPhasesHashBaseAggregate.scala:
##########
@@ -45,10 +45,17 @@ case class MergeTwoPhasesHashBaseAggregate(session:
SparkSession)
val mergeTwoPhasesAggEnabled: Boolean =
GlutenConfig.get.mergeTwoPhasesAggEnabled
private def isPartialAgg(partialAgg: BaseAggregateExec, finalAgg:
BaseAggregateExec): Boolean = {
- // TODO: now it can not support to merge agg which there are the filters
in the aggregate exprs.
+ // Aggregates with a FILTER clause can be merged as long as the FILTER
predicate is carried
+ // over to the Complete mode aggregate. Note the physical final aggregate
has its FILTER
+ // stripped (Spark's AggUtils.mayRemoveAggFilters only keeps FILTER in
Partial/Complete modes),
+ // so the FILTER must be restored from the partial aggregate when merging.
The partial and
+ // final expressions are both copied from the same logical aggregate
expressions (with only
+ // the `mode` changed), so a partial/final pair shares the same `resultId`.
if (
- partialAgg.aggregateExpressions.forall(x => x.mode == Partial &&
x.filter.isEmpty) &&
- finalAgg.aggregateExpressions.forall(x => x.mode == Final &&
x.filter.isEmpty)
+ partialAgg.aggregateExpressions.forall(x => x.mode == Partial) &&
+ finalAgg.aggregateExpressions.forall(x => x.mode == Final) &&
+ partialAgg.aggregateExpressions.map(_.resultId).toSet ==
+ finalAgg.aggregateExpressions.map(_.resultId).toSet
Review Comment:
`toSet` drops duplicates, so this guard can incorrectly pass when
`resultId`s are not a true 1:1 match (e.g., duplicates on either side). That
can lead to wrong FILTER restoration (or later failures). Consider
strengthening the check to enforce equal lengths and either (a) uniqueness of
`resultId` on both sides, or (b) multiset equality (count per `resultId`) so
duplicates are detected.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
