Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-08-03 Thread via GitHub 


yikf commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5163414681

   thanks @zhztheplayer @zzcclp 


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-08-02 Thread via GitHub 


zzcclp merged PR #12653:
URL: https://github.com/apache/gluten/pull/12653


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-08-02 Thread via GitHub 


yikf commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5161907849

   > @yikf The CH CI failure seems related. Can you take a look?
   
   Addressed, 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: [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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-31 Thread via GitHub 


github-actions[bot] commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5143293672

   Run Gluten Clickhouse CI on x86


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-31 Thread via GitHub 


zhztheplayer commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5142113440

   @yikf The CH CI failure seems related. Can you take a look?


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-30 Thread via GitHub 


yikf commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5138611398

   @zzcclp @zhztheplayer Could you please take a look? 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: [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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-29 Thread via GitHub 


github-actions[bot] commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5127278768

   Run Gluten Clickhouse CI on x86


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-29 Thread via GitHub 


github-actions[bot] commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5118114022

   Run Gluten Clickhouse CI on x86


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-29 Thread via GitHub 


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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-29 Thread via GitHub 


github-actions[bot] commented on PR #12653:
URL: https://github.com/apache/gluten/pull/12653#issuecomment-5117420248

   Run Gluten Clickhouse CI on x86


-- 
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]

Re: [PR] [GLUTEN-12652][CORE] Support merging two-phase aggregates with FILTER clause [gluten]

2026-07-29 Thread via GitHub 


Copilot commented on code in PR #12653:
URL: https://github.com/apache/gluten/pull/12653#discussion_r3673891683


##
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/MergeTwoPhasesHashBaseAggregate.scala:
##
@@ -59,6 +64,26 @@ case class MergeTwoPhasesHashBaseAggregate(session: 
SparkSession)
 }
   }
 
+  /**
+   * Builds Complete mode aggregate expressions from the final aggregate. The 
physical final
+   * aggregate no longer carries the FILTER predicate (see `isPartialAgg`), so 
the FILTER is
+   * restored from the partial aggregate, whose expressions align one-to-one 
with the final ones.
+   */
+  private def toCompleteAggregateExpressions(
+  partialAgg: BaseAggregateExec,
+  finalAggExpressions: Seq[AggregateExpression]): Seq[AggregateExpression] 
= {
+require(
+  finalAggExpressions.length == partialAgg.aggregateExpressions.length,
+  s"Expected partial and final aggregate expression lists to align 1:1, 
but got " +
+s"${partialAgg.aggregateExpressions.length} partial and " +
+s"${finalAggExpressions.length} final expressions"
+)
+finalAggExpressions.zip(partialAgg.aggregateExpressions).map {
+  case (finalExpr, partialExpr) =>
+finalExpr.copy(mode = Complete, filter = partialExpr.filter)
+}

Review Comment:
   This restores `FILTER` by positional alignment only. If Spark ever reorders 
(or otherwise rewrites) `partialAgg.aggregateExpressions` vs 
`finalAggExpressions` while keeping the same length, the wrong `FILTER` 
predicate can be attached to the wrong aggregate expression, producing 
incorrect results (especially for mixed filtered + non-filtered aggregates). A 
more robust approach is to validate pairwise identity before copying the 
`filter` (e.g., match on `resultId` / `aggregateFunction` semantic equality) 
and skip merging if a mismatch is detected.



##
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/MergeTwoPhasesHashBaseAggregate.scala:
##
@@ -59,6 +64,26 @@ case class MergeTwoPhasesHashBaseAggregate(session: 
SparkSession)
 }
   }
 
+  /**
+   * Builds Complete mode aggregate expressions from the final aggregate. The 
physical final
+   * aggregate no longer carries the FILTER predicate (see `isPartialAgg`), so 
the FILTER is
+   * restored from the partial aggregate, whose expressions align one-to-one 
with the final ones.
+   */
+  private def toCompleteAggregateExpressions(
+  partialAgg: BaseAggregateExec,
+  finalAggExpressions: Seq[AggregateExpression]): Seq[AggregateExpression] 
= {
+require(
+  finalAggExpressions.length == partialAgg.aggregateExpressions.length,
+  s"Expected partial and final aggregate expression lists to align 1:1, 
but got " +
+s"${partialAgg.aggregateExpressions.length} partial and " +
+s"${finalAggExpressions.length} final expressions"
+)

Review Comment:
   `require(...)` in a Spark planner/optimizer rule can fail the entire query 
planning/execution path with an exception. Since this rule is an optimization, 
it’s generally safer to “decline to merge” and fall back to the original plan 
when invariants aren’t met. Consider replacing the `require` with a graceful 
fallback (e.g., return `finalAggExpressions.map(_.copy(mode = Complete))` or 
simply skip merging earlier) to avoid turning a non-critical optimization into 
a hard failure.



-- 
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]