Skip to content

[fix](fe) Fix redundant aggregation in agg-union query plan#62231

Merged
924060929 merged 3 commits intoapache:masterfrom
feiniaofeiafei:fix-agg-union
Apr 10, 2026
Merged

[fix](fe) Fix redundant aggregation in agg-union query plan#62231
924060929 merged 3 commits intoapache:masterfrom
feiniaofeiafei:fix-agg-union

Conversation

@feiniaofeiafei
Copy link
Copy Markdown
Contributor

@feiniaofeiafei feiniaofeiafei commented Apr 8, 2026

Problem

When multiple grouped queries are combined via UNION ALL on randomly distributed tables without PhysicalDistribute operators, Doris was generating redundant local aggregation and global aggregation layers without a distribute operator between them.

The issue was in the banAggUnionAll method in ChildrenPropertiesRegulator, which was too aggressively forbidding one-phase aggregation for all agg-union patterns, regardless of whether the union inputs had PhysicalDistribute operators.

Solution

Modified the banAggUnionAll method to only forbid one-phase aggregation when the union inputs actually have PhysicalDistribute operators:

  1. Checks each group in the union
  2. Examines all physical expressions in each group
  3. Only forbids one-phase agg if any input has PhysicalDistribute
  4. Allows one-phase agg if no inputs have PhysicalDistribute

Test

Added regression test agg_union_group_by.groovy to verify correct behavior

Impact

Improved query optimization: Eliminated redundant aggregation layers in UNION ALL queries with GROUP BY on randomly distributed tables.

@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Apr 8, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@feiniaofeiafei feiniaofeiafei changed the title Test PR [fix](fe) Fix redundant aggregation in agg-union query plan Apr 8, 2026
@feiniaofeiafei feiniaofeiafei marked this pull request as draft April 8, 2026 10:40
### What problem does this PR solve?

Issue Number: close #N/A

Problem Summary: When multiple grouped queries are combined via UNION ALL on randomly distributed tables without PhysicalDistribute operators, Doris was generating redundant local aggregation and global aggregation layers without a distribute operator between them.

The issue was in the `banAggUnionAll` method in ChildrenPropertiesRegulator, which was too aggressively forbidding one-phase aggregation for all agg-union patterns, regardless of whether the union inputs had PhysicalDistribute operators.

### Release note

Improved query optimization: Eliminated redundant aggregation layers in UNION ALL queries with GROUP BY on randomly distributed tables.

### Check List (For Author)

- Test: FE UT and Regression test added
    - FE UT: Added AggregateUnionPlanTest to verify correct behavior
    - Regression test: Added agg_union_group_by test case to verify execution
- Behavior changed: Yes - query plans are now more optimal with fewer redundant aggregation layers
- Does this need documentation: No

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@feiniaofeiafei
Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 85.71% (12/14) 🎉
Increment coverage report
Complete coverage report

@feiniaofeiafei
Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 85.71% (12/14) 🎉
Increment coverage report
Complete coverage report

@feiniaofeiafei
Copy link
Copy Markdown
Contributor Author

run feut

@feiniaofeiafei feiniaofeiafei marked this pull request as ready for review April 10, 2026 06:00
@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@924060929 924060929 merged commit 000395d into apache:master Apr 10, 2026
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/4.1.x dev/4.1.x-conflict reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants