[Enhancement] Push down non-group-by aggregate through union all#73930
Conversation
Overall the rewrite looks correct. The property that makes it safe is the all-or-nothing decomposition through
Both are the conservative-correct choice. On the PR description: it says the whitelist "adds COUNT and HLL_RAW", but the change actually enables the full A few things worth considering: 1. Gate the non-group-by push-down on input size. For the non-group-by path 2. Make the whitelist and the partial→final remap a single source of truth. 3. (Desirable) Extend to Iceberg/MOR tables. Tables with equality deletes are rewritten by What we'd want is the same decomposition this PR already does for plain Landing the partial above a non-inflating anti/semi join is always correct (it aggregates exactly what the branch feeds the union), and the "land above a join" machinery already exists for the small-broadcast-join case. The push-vs-not decision uses the same row-count floor as #1, just read at the join's output rather than a scan — the only caveat being that the anti-join's output cardinality is a weaker estimate. For this PR, a regression test asserting that a non-group-by aggregate over a |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
LGTM |
There was a problem hiding this comment.
Pull request overview
This PR enhances FE optimizer aggregate push-down so non-group-by aggregates over UNION ALL can be partially computed in each branch and merged above the union, reducing raw-row exchange pressure for sampled-statistics style queries.
Changes:
- Adds non-group-by aggregate eligibility/remapping for
COUNTandHLL_RAW, plus self-mergeable aggregate functions. - Blocks unsafe non-group-by push-down through joins and
UNION DISTINCT. - Adds regression tests for union-all push-down, join/distinct guards, filter/project edge cases, and local/global/auto modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
PushDownAggregateCollector.java |
Extends collection/eligibility logic for non-group-by aggregate push-down and adds safety/statistics guards. |
PushDownAggregateRewriter.java |
Rewrites final-stage aggregate functions and handles zero-argument COUNT(*) safely. |
AggregatePushDownTest.java |
Adds optimizer plan tests covering the new push-down behavior and regressions. |
e875534 to
64be926
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff7e0d956
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
eff7e0d to
57d10da
Compare
Signed-off-by: JasonQuCode <88529388+JasonQuCode@users.noreply.github.com>
Signed-off-by: JasonQuCode <88529388+JasonQuCode@users.noreply.github.com>
d3cffd0 to
f868405
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f868405349
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@satanson Submitted to the community edition, PTAL. |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 201 / 224 (89.73%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@Mergifyio backport branch-4.1 |
✅ Backports have been createdDetails
|
Why I'm doing:
Statistics collection queries may aggregate data above UNION ALL, causing a large amount of raw sampled rows to be exchanged and merged before local aggregation. This can increase network transfer, memory usage, and execution time for non-group-by aggregate workloads such as ANALYZE SAMPLE.
This PR pushes local split aggregates into UNION ALL branches when the physical plan is already a safe two-stage aggregate plan, so each branch can aggregate raw rows earlier and UNION ALL only needs to pass intermediate aggregate states.
What I'm doing:
Add a physical plan rewrite rule to push eligible non-group-by local split aggregates into UNION ALL branches, while preserving the upper merge stage to combine intermediate aggregate states.
Also add tests to verify both plan rewriting and result correctness.
Performance Test
Environment
Test environment:
Test table schema:
Test 1: ANALYZE SAMPLE
This test runs sample statistics collection on the 200M-row table:
The result below is the average of 5 runs.
Result:
QueryPeakMemoryUsagereduced by about 24.2%Test 2: Statistics-like SQL
This test uses a statistics-collection-like SQL on the same 200M-row table. The SQL contains multiple
UNION ALLbranches and aggregate functions commonly used by statistics collection, includingCOUNT,SUM,MIN,MAX, andHLL_RAW.Default parallelism settings are used.
The result below is the average of 5 runs.
Result:
QueryPeakMemoryUsagereduced by about 55.8%What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: