Repository navigation
[VL] Eager aggregation: Refactor option partialMerge.mode - #13328
Merged
Merged
Conversation
|
Run Gluten Clickhouse CI on x86 |
JkSelf
approved these changes
Oct 16, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
TOP staging retains reducer references that are unavailable after mapper aggregation.
3 open findings
What changed in this PR
Separates eager aggregation’s logical merge placement from physical staging.
Changes:
- Replaces
partialMerge.modewithpartialMerge.enabledandtwoStage.mode. - Adds optional mapper-side merging before final aggregation.
- Updates configuration documentation and test variants.
| File | Description |
|---|---|
| gluten-substrait/src/test/scala/org/apache/gluten/execution/PushAggregateThroughJoinSuite.scala | Updates logical merge-count expectations. |
| gluten-substrait/src/main/scala/org/apache/gluten/extension/joinagg/PushAggregateThroughJoin.scala | Retains logical merges only between joins. |
| gluten-substrait/src/main/scala/org/apache/gluten/extension/joinagg/ImplementJoinAggregate.scala | Implements optional two-stage final aggregation. |
| gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala | Defines replacement configuration options. |
| docs/Configuration.md | Documents the new options. |
| backends-velox/src/test/scala/org/apache/gluten/execution/StarSchemaJoinAggregateSuite.scala | Reorganizes configuration-specific test suites. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| distribution) | ||
| } else { | ||
| planFinalOutput(grouping, resultExpressions, preparedChild, rewrittenAggExprs) | ||
| buildFinalAggregate(grouping, resultExpressions, stageChild, stageExpressions, distribution) |
Comment on lines
+182
to
+183
| val stageChild = if (stage == AggregationStage.TWO_STAGE) { | ||
| buildMapperAggregate(grouping, preparedChild, rewrittenAggExprs) |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
| ae => | ||
| val mode = ae.mode match { | ||
| case Partial => PartialMerge | ||
| case Complete => Final |
| distribution) | ||
| } else { | ||
| planFinalOutput(grouping, resultExpressions, preparedChild, rewrittenAggExprs) | ||
| buildFinalAggregate(grouping, resultExpressions, stageChild, stageExpressions, distribution) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Separate aggregate pushdown placement from physical staging:
This avoids a separate logical merge above the topmost join and leaves BHJ inputs unchanged.
Breaking change: replaces partialMerge.mode. Old NONE → false/NONE, TOP → false/FINAL, ALL → true/FINAL.
Updates configuration docs and separates logical-pushdown and physical-staging tests.