[fix](preagg) Enable pre-aggregation for multi-argument aggregate functions with all-key distinct inputs#65846
Open
starocean999 wants to merge 4 commits into
Open
[fix](preagg) Enable pre-aggregation for multi-argument aggregate functions with all-key distinct inputs#65846starocean999 wants to merge 4 commits into
starocean999 wants to merge 4 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29606 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary
Requested changes for one P2 performance regression: the new blanket multi-child gate fixes the unsafe mixed key/value COUNT DISTINCT case, but it also disables valid partial/parallel scanning for all-key multi-column COUNT DISTINCT. The inline comment contains the concrete plan, impact, and fix/test boundary.
Critical checkpoint conclusions:
- Goal and correctness:
count(distinct k6, v7)on an aggregate-key table now correctly gets PREAGGREGATION OFF, preventing partial SUM states from changing the DISTINCT tuples. The implementation is broader than required and regresses the safecount(distinct k5, k6)plan. - Scope and focus: the two-file patch is mechanically small and readable, but
children().size() > 1is not the smallest semantic predicate; supported multi-child COUNT DISTINCT arguments need per-scan validation. - Parallel paths and performance: both Nereids
SET_PREAGG_STATUSregistrations use this rule. For AGG_KEYS/UNIQUE-MOR scans, the unnecessary OFF decision requires a full storage merge and can remove an otherwise eligible scan from the BE parallel-scan path. DUP_KEYS and UNIQUE-MOW/MOR-as-DUP bypasses are unchanged. - Testing: the added EXPLAIN is a valid before-fail/after-pass trigger for the mixed key/value bug, but
notContains ... PREAGGREGATION: ONdoes not prove the intended OFF reason and there is no positive all-key boundary. No result file is involved. - Compatibility and FE/BE propagation: no function symbol, storage format, configuration, or thrift field changes are introduced; the existing preaggregation flag is forwarded unchanged.
- Concurrency, lifecycle, persistence, transactions, and writes: none are introduced or modified by this patch.
- Error handling and observability: the new branch fails closed and EXPLAIN exposes the OFF reason; no additional log or metric is needed.
- Other paths: aliases, nested aggregates, joins/multiple scans, nullable/duplicate keys, expression shapes, and later DISTINCT splitting were checked. No additional independent issue was substantiated.
User focus: no additional review focus was provided.
Validation: static review only. The review environment explicitly prohibits builds and test execution, so the added regression case was inspected but not run.
Review completion: every changed file and candidate was swept, deduplicated, and resolved; normal full-review and risk-focused agents converged within the three-round cap.
Contributor
TPC-DS: Total hot run time: 178944 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
starocean999
marked this pull request as ready for review
July 23, 2026 01:45
starocean999
requested review from
924060929,
englefly and
morrySnow
as code owners
July 23, 2026 01:45
starocean999
force-pushed
the
master_0721
branch
from
July 24, 2026 02:34
6a34cab to
0d65540
Compare
starocean999
changed the title
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary
Requested changes for three P1 correctness regressions in aggregate-key preaggregation classification. The direct count(DISTINCT k6, v7) case is now OFF and direct all-key count(DISTINCT k6, k5) is ON, but adjacent legal plans can still receive ON and observe partial storage rows.
Critical checkpoint conclusions:
- Goal and correctness: not fully achieved; three wrong-result paths are described inline.
- Scope and minimality: the patch is small and readable, but the input-slot-set criterion is broader than the supported argument semantics and assumes every slot was classified.
- Parallel paths and performance: both
SetPreAggStatusregistrations are affected. The safe deterministic all-key optimization is preserved; the unsafe ON decisions change correctness rather than only performance. Planning cost is negligible. - Tests and results: the added EXPLAIN checks cover the direct mixed/all-key boundaries, but not an IF/CASE first argument with a later value column, a volatile key expression, or a multi-project derived alias. The weak OFF-reason assertion is already covered by the existing thread. No local build or test was run because the review contract prohibits it; current style, license, and basic checks are green, with no targeted regression result at this head.
- Compatibility, persistence, transactions, writes, FE/BE protocol, concurrency, lifecycle, configuration, and memory: no independent issue applies.
- Error handling and observability: the existing EXPLAIN OFF reason is sufficient; no new log or metric is needed.
- Existing review context: the direct all-key discussion was deduplicated.
- User focus: none was specified.
Review completion: every changed file and candidate was swept; three full/risk-focused rounds converged with NO_NEW_VALUABLE_FINDINGS on this final ledger and comment set.
Validation: static review only; builds and tests were prohibited by the governing review prompt.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes
The all-key multi-argument DISTINCT path and the current-head fixes for the existing threads are directionally correct, but this review found four additional issues: three correctness/planning failures and one planner-complexity regression. Because M-004 was discovered in the third and final allowed round, the mandated convergence protocol ended without every reviewer returning NO_NEW_VALUABLE_FINDINGS; this review is therefore incomplete under the automation contract, although every candidate discovered so far has a verified disposition and is included below.
Critical checkpoint conclusions:
- Goal and data correctness: the direct all-key multi-column DISTINCT case is enabled and the mixed-value cases are rejected, but volatile expressions can still observe partial-row multiplicity (M-002), and mixed validation can enable one scan using another scan's SUM value (M-003).
- Scope and clarity: the patch remains within preaggregation classification and its regression suite, but eager alias composition introduces both redundant map copying (M-001) and an uncaught expression-limit failure (M-004).
- Concurrency and lifecycle: no concurrent shared state, lock ordering, asynchronous lifecycle, or static-initialization concern is introduced; the context is per rewrite.
- Configuration and compatibility: no new configuration, FE/BE protocol, storage-format, rolling-upgrade, or persisted-state change is present. M-004 incorrectly bypasses the existing expression width/depth limits rather than introducing a new setting.
- Parallel paths and conditions: value-only, key-only, mixed IF/CASE, multi-argument DISTINCT, sibling projects, and join paths were traced. The filter/join/grouping and other-table volatile paths are not covered by the new local guard, and mixed key/value ownership is not scan-relative.
- Tests and results: the added EXPLAIN cases cover direct all-key/mixed DISTINCT, local volatile aggregate arguments/conditions, and the repaired sibling alias case. Coverage is still missing for volatile filter/join/grouping and other-table aggregates, cross-scan foreign-SUM multiplicity, and over-limit nested project composition. This runner's review contract prohibited builds/tests; validation was static only, and
thirdparty/installed/bin/protocis absent. - Performance: M-001 adds O(N^2) prefix copying across projected branches; no other material hot-path regression was substantiated.
- Observability: existing PREAGGREGATION ON/OFF reasons are sufficient for this rule; no new metric or logging requirement was identified.
- Transactions, persistence, data writes, and FE/BE variable propagation: not applicable to these planner-only changes.
- User focus: no additional user-provided focus was supplied.
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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
2 participants
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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #48502
Problem Summary:
When an aggregate function has more than one child expression (e.g., count(distinct k6, v7)), the pre-aggregation status check previously only handled the cases of zero children or exactly one child. Multi-child aggregate functions fell through to the else branch that attempted to process them via checkAggWithKeyAndValueSlots, which is designed for single aggregate functions with complex expressions like IF/CASE WHEN — not for genuinely multi-parameter aggregate functions. This could lead to incorrect pre-aggregation decisions for functions like COUNT(DISTINCT col1, col2).
This fix explicitly only enable pre-aggregation for multi-argument aggregate functions with all-key distinct inputs, and reject other multi-argument aggregate functions.
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)