[opt](pipeline) Add passthrough between repeat and streaming aggregation - #66339
[opt](pipeline) Add passthrough between repeat and streaming aggregation#66339Mryange wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Repeat expands grouping sets before local streaming aggregation. Without a passthrough local exchange, independent pipeline tasks create duplicate partial aggregation states, while hash shuffling every expanded row adds unnecessary hashing and skew. Request a passthrough exchange for a direct Repeat-to-StreamingAgg edge in both FE-planned and BE-planned local shuffle paths, and cover both distribution planning paths with tests. ### Release note Improve local streaming aggregation performance for grouping sets by distributing Repeat output through a passthrough local exchange. ### Check List (For Author) - Test: Not run; pending the separate validation step - Behavior changed: Yes; Repeat-to-StreamingAgg now requests PASSTHROUGH local exchange - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 29091 ms |
TPC-DS: Total hot run time: 167474 ms |
ClickBench: Total hot run time: 23.99 s |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
924060929
left a comment
There was a problem hiding this comment.
The Repeat-to-StreamingAgg PASSTHROUGH direction is reasonable for avoiding per-row hash work and grouping-set skew, and the FE !needsFinalize guard correctly limits it to partial aggregation. However, the current implementation makes the optimization unconditional and bypasses the dedicated StreamingAgg local-exchange switch in both planning paths. Please preserve the existing opt-out semantics and cover the on/off matrix before merging.
| if (children.get(0) instanceof HashJoinNode | ||
| // Repeat expands grouping sets before this local preaggregation. Use PASSTHROUGH | ||
| // to distribute the expanded blocks without hashing every row. | ||
| if (!needsFinalize && children.get(0) instanceof RepeatNode) { |
There was a problem hiding this comment.
This special case unconditionally inserts a local exchange and bypasses the opt-out semantics of enable_local_exchange_before_streaming_agg.
That dedicated switch was added with a default of false specifically so StreamingAgg preserves its inherited distribution unless local exchange is explicitly enabled. In the BE path, the new Repeat check likewise runs before enable_local_exchange_before_streaming_agg() is checked. In the FE path, this branch also precedes the existing enableLeBeforeAgg fallback and does not consult the streaming-specific switch at all.
A two-phase grouping-sets query therefore gets StreamingAgg <- LE(PASSTHROUGH) <- Repeat even when the streaming-agg switch is false. This should not change query results because this is partial aggregation followed by a final merge, but it makes the extra pipeline boundary, queues, and memory overhead impossible to disable, and silently changes the default behavior established by #66222.
Please gate the Repeat PASSTHROUGH optimization with enableLocalExchangeBeforeStreamingAgg here and with enable_local_exchange_before_streaming_agg() in BE, and add FE-/BE-planned tests for both switch values. If Repeat is intentionally meant to ignore the existing switch, that needs an explicit separately named switch and documented default behavior rather than bypassing the current opt-out.
What problem does this PR solve?
Problem Summary: Repeat expands grouping sets before local streaming aggregation. Without a passthrough local exchange, independent pipeline tasks create duplicate partial aggregation states, while hash shuffling every expanded row adds unnecessary hashing and skew. Request a passthrough exchange for a direct Repeat-to-StreamingAgg edge in both FE-planned and BE-planned local shuffle paths, and cover both distribution planning paths with tests.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)