Uh oh!
There was an error while loading. Please reload this page.
[fix](agg) Normalize equivalent multi-column distinct counts - #65206
Conversation
hello-stephen
commented
Jul 3, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
feiniaofeiafei
commented
Jul 6, 2026
run buildall |
hello-stephen
commented
Jul 6, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 6, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
Jul 7, 2026
run nonConcurrent |
ac83640 to
7812eb6Comparefeiniaofeiafei
commented
Jul 7, 2026
run buildall |
hello-stephen
commented
Jul 7, 2026
FE UT Coverage ReportIncrement line coverage |
| public static Expression countDistinctMultiExprToCountIf(Count count) { | ||
| Set<Expression> arguments = ImmutableSet.copyOf(count.getArguments()); | ||
| Expression countExpr = count.getArgument(arguments.size() - 1); | ||
| List<Expression> arguments = ImmutableList.copyOf(ImmutableSet.copyOf(count.getArguments())); |
7812eb6 to
a728675Comparefeiniaofeiafei
commented
Jul 14, 2026
run buildall |
hello-stephen
commented
Jul 14, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 14, 2026
TPC-H: Total hot run time: 29615 ms |
hello-stephen
commented
Jul 14, 2026
TPC-DS: Total hot run time: 178239 ms |
hello-stephen
commented
Jul 14, 2026
ClickBench: Total hot run time: 25.07 s |
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: NormalizeAggregate treated reordered equivalent multi-column COUNT DISTINCT expressions as different aggregate functions, which could make the multi-distinct strategy construct a join from only one aggregate and fail with an index-out-of-bounds exception. It also retained duplicate arguments even though downstream COUNT DISTINCT processing treats them as a set, and the CountIf conversion indexed the original argument list using the deduplicated size. Canonicalize multi-column distinct arguments as an ordered set during normalization and make CountIf consume that deduplicated list directly. ### Release note Fix planning and execution of equivalent multi-column COUNT DISTINCT expressions with reordered or duplicate arguments. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.rules.analysis.NormalizeMultiColumnDistinctCountTest - Behavior changed: Yes, reordered and duplicate multi-column COUNT DISTINCT arguments now share the correct aggregate computation - Does this need documentation: No
a728675 to
2048602Comparefeiniaofeiafei
commented
Jul 14, 2026
run buildall |
hello-stephen
commented
Jul 14, 2026
TPC-H: Total hot run time: 29457 ms |
hello-stephen
commented
Jul 14, 2026
TPC-DS: Total hot run time: 177435 ms |
hello-stephen
commented
Jul 14, 2026
ClickBench: Total hot run time: 24.88 s |
hello-stephen
commented
Jul 14, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
Jul 15, 2026
/review |
feiniaofeiafei
commented
Jul 17, 2026
run buildall |
hello-stephen
commented
Jul 17, 2026
FE UT Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
Jul 17, 2026
run performance |
englefly
commented
Jul 17, 2026
/review |
hello-stephen
commented
Jul 17, 2026
TPC-H: Total hot run time: 29828 ms |
hello-stephen
commented
Jul 17, 2026
TPC-DS: Total hot run time: 176210 ms |
hello-stephen
commented
Jul 17, 2026
ClickBench: Total hot run time: 25.08 s |
There was a problem hiding this comment.
Automated code review: request changes.
The intended scalar canonicalization is reasonable for supported ordinary grouped/global paths, but four blocking correctness or error-contract regressions remain:
- Duplicate BITMAP/HLL arguments can become a unary union aggregate instead of preserving the unsupported multi-argument error.
- Traversal through WindowExpression can make an unrelated grouped aggregate bypass the documented multi-argument analytic COUNT error.
- Sharing reordered Counts newly enables a hint-supported join pushdown that assumes a sole aggregate is unary and builds an invalid Count/slot set.
- The same sharing can make AGG_KEYS scan preaggregation depend on the retained argument order and expose partial aggregate-value rows to tuple-distinct counting.
Review convergence: incomplete under the prescribed three-round limit. Round 3 found the fourth issue; an independent downstream pass confirmed it and found no fifth issue, but a new valuable round-3 candidate means the required all-reviewer NO_NEW_VALUABLE_FINDINGS state was not reached. All four findings were independently validated and deduplicated before submission.
Critical checkpoints:
- Goal, scope, and tests: the four-file change is focused and the normal duplicate/reordered scalar paths are covered, but the new traversal, arity changes, and structural sharing escape into unsupported type, window, join-pushdown, and storage-preaggregation paths that lack regression coverage.
- Compatibility and error behavior: two established errors become accepted but semantically different operations; the other two paths can build an invalid plan or return a wrong distinct cardinality.
- Parallel paths: ordinary grouped/global and forced aggregate-phase paths use compatible canonical arguments; the window, BITMAP/HLL, hinted join, and AGG_KEYS preaggregation paths do not.
- Concurrency, lifecycle, configuration, persistence, data writes, and protocol compatibility: not involved by this FE rewrite change.
- Performance and observability: the local map/set work is bounded by aggregate output size and raises no standalone concern; no new observability surface is involved.
- User focus: no additional review focus was provided, so the full authoritative patch was reviewed.
Validation was static only because the review contract prohibited local builds/tests. At the reviewed head, COMPILE, FE UT, BE UT, Cloud UT, external/nonconcurrent/cloud/vault regressions, formatter/license, and performance are successful. P0 regression remains pending. check_coverage_fe is failed, but its TeamCity log is not accessible here, so no code-level cause could be verified.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
feiniaofeiafei
commented
Jul 21, 2026
/review |
feiniaofeiafei
commented
Jul 21, 2026
run buildall |
hello-stephen
commented
Jul 21, 2026
TPC-H: Total hot run time: 29285 ms |
hello-stephen
commented
Jul 21, 2026
TPC-DS: Total hot run time: 177283 ms |
hello-stephen
commented
Jul 21, 2026
ClickBench: Total hot run time: 25.02 s |
924060929
commented
Jul 22, 2026
/review |
There was a problem hiding this comment.
Automated code review: request changes.
The current head fixes the previously reported join-pushdown invalid-arity path by requiring unary aggregates, but three blocking correctness/error-contract issues in the existing inline threads remain unresolved:
- Duplicate BITMAP/HLL arguments can become unary union aggregates instead of preserving the unsupported multi-argument error.
- Recursive normalization through
WindowExpressioncan bypass the documented multi-argument analyticCOUNTerror. - Sharing reordered Counts can make AGG_KEYS preaggregation validate only the retained first child and expose partial aggregate-value rows, producing a wrong distinct cardinality.
This review also adds one new inline test-coverage issue: the changed success regressions are bare sql statements and never assert the returned counts or nullable CountIf behavior.
Critical checkpoints:
- Goal and data correctness: ordinary deterministic grouped/global split paths share equivalent Counts and preserve the unique argument set and null semantics, but the three unresolved special paths above mean the PR is not yet safe.
- Scope and clarity: the five-file change is focused; the local map/set work is linear in small aggregate outputs and reduces redundant aggregate state.
- Parallel paths and compatibility: join pushdown is now safely unary; logical/physical distinct splitting and phases 1-4 use the full deduplicated set; the window, BITMAP/HLL, and AGG_KEYS preaggregation paths remain incompatible or incorrect.
- Tests: FE UT and P0 regression are green, but the new Groovy cases have no result oracle, and the unresolved window/type/preaggregation cases lack protecting regressions.
check_coverage_feis currently failing. - Concurrency, lifecycle, configuration, persistence, transactions, data writes, protocol compatibility, FE/BE variable passing, and observability: not involved.
- User focus: no additional focus was provided; the full authoritative patch was reviewed.
Validation was static only because the review contract prohibits local builds/tests; .worktree_initialized and thirdparty/installed/bin/protoc are absent. Current GitHub checks are otherwise broadly green, including compile, FE/BE/Cloud unit tests, P0/external/cloud/vault regressions, style/license, and performance.
Review convergence: all normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS for the final current head, ledger, inline comment, and summary after the last accepted candidate and wording/anchor change.
| group_concat(distinct cast(number as varchar) order by number + 1) from numbers('number'='10')""" | ||
| exception "Unsupported query" | ||
| } | ||
| sql """select number % 2, count(distinct cast(number as varchar), cast(number as varchar)), |
There was a problem hiding this comment.
Make these result-checked regression cases. Both new statements are bare sql, so the suite passes for any non-error output even though this PR changes duplicate removal and the CountIf null-filtering expression. The Java tests only inspect plan shape/input slots. Please use qt_/order_qt_ with generated expected counts, and include reordered/partially duplicated nullable arguments so the grouped/global execution result is actually verified.
Uh oh!
There was an error while loading. Please reload this page.
…65206) ### What problem does this PR solve? Related PR: apache#54079 Problem Summary: NormalizeAggregate treated reordered equivalent multi-column COUNT DISTINCT expressions as different aggregate functions, which could make the multi-distinct strategy construct a join from only one aggregate and fail with an index-out-of-bounds exception. It also retained duplicate arguments even though downstream COUNT DISTINCT processing treats them as a set, and the CountIf conversion indexed the original argument list using the deduplicated size. Canonicalize multi-column distinct arguments as an ordered set during normalization and make CountIf consume that deduplicated list directly. ### Release note Fix planning and execution of equivalent multi-column COUNT DISTINCT expressions with reordered or duplicate arguments.
…65206) ### What problem does this PR solve? Related PR: apache#54079 Problem Summary: NormalizeAggregate treated reordered equivalent multi-column COUNT DISTINCT expressions as different aggregate functions, which could make the multi-distinct strategy construct a join from only one aggregate and fail with an index-out-of-bounds exception. It also retained duplicate arguments even though downstream COUNT DISTINCT processing treats them as a set, and the CountIf conversion indexed the original argument list using the deduplicated size. Canonicalize multi-column distinct arguments as an ordered set during normalization and make CountIf consume that deduplicated list directly. ### Release note Fix planning and execution of equivalent multi-column COUNT DISTINCT expressions with reordered or duplicate arguments.
What problem does this PR solve?
Issue Number: None
Related PR: #54079
Problem Summary: NormalizeAggregate treated reordered equivalent multi-column COUNT DISTINCT expressions as different aggregate functions, which could make the multi-distinct strategy construct a join from only one aggregate and fail with an index-out-of-bounds exception. It also retained duplicate arguments even though downstream COUNT DISTINCT processing treats them as a set, and the CountIf conversion indexed the original argument list using the deduplicated size. Canonicalize multi-column distinct arguments as an ordered set during normalization and make CountIf consume that deduplicated list directly.
Release note
Fix planning and execution of equivalent multi-column COUNT DISTINCT expressions with reordered or duplicate arguments.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)