[feature](nereids) support multi_distinct_collect_list and multi_distinct_array_agg#65245
[feature](nereids) support multi_distinct_collect_list and multi_distinct_array_agg#65245Baymine wants to merge 1 commit into
Conversation
…inct_array_agg ### What problem does this PR solve? Issue Number: close apache#65244 Problem Summary: When a single GROUP BY contained more than one DISTINCT aggregate and any of them was collect_list(distinct ...) or array_agg(distinct ...), planning failed with "... can't support multi distinct". CheckMultiDistinct rejects any distinct aggregate that is not a SupportMultiDistinct, and collect_list / array_agg did not implement that interface, so queries such as select g, collect_list(distinct a), collect_list(distinct b) from t group by g select g, array_agg(distinct a), array_agg(distinct b) from t group by g could not run at all. This adds the missing multi-distinct variants and wires them into the existing framework: - FE: CollectList and ArrayAgg now implement SupportMultiDistinct and convert to the new MultiDistinctCollectList / MultiDistinctArrayAgg functions. The new functions implement MultiDistinction, are registered in BuiltinAggregateFunctions, and get visitor hooks in AggregateFunctionVisitor. - BE: registers multi_distinct_collect_list and multi_distinct_array_agg. Both dedup in the aggregate state via the existing Set-backed collect data, because the Nereids multi-distinct plan does not push the distinct argument into the group-by key. ### Release note Support multiple DISTINCT aggregates in one GROUP BY when they include collect_list(distinct ...) or array_agg(distinct ...). ### Check List (For Author) - Test: - [x] Unit Test: MultiDistinctArrayAggTest, MultiDistinctCollectListTest, CheckMultiDistinctTest (28 cases, all pass). - [x] Regression test: added regression-test/suites/nereids_p0/multi_distinct/multi_distinct_collect_list.groovy (verified via CI; local cluster restart was skipped in this environment). - Behavior changed: Yes. Queries that previously failed with "can't support multi distinct" for collect_list/array_agg now plan and run. - Does this need documentation: No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
morrySnow
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds multi_distinct_collect_list and multi_distinct_array_agg support — a solid feature enabling queries with multiple DISTINCT aggregates in one GROUP BY that include collect_list/array_agg. The overall approach (FE: SupportMultiDistinct + MultiDistinction classes; BE: registering new function names) follows existing patterns. Good test coverage.
Below are the issues found, ranked by severity:
1. [HIGH] array_agg distinct path silently drops nulls
See inline comment in aggregate_function_array_agg.cpp. The Set-based dedup data structure cannot represent null elements, and the null-skipping adapter drops them before the inner function sees them. This means array_agg(distinct nullable_col) returns different results when it takes the multi-distinct path vs. the single-distinct path.
2. [MEDIUM] canSkewRewrite can produce nested ARRAY<ARRAY>
Adding SupportMultiDistinct to ArrayAgg/CollectList makes canSkewRewrite() (Aggregate.java:~206) return true. In SplitAggMultiPhase.java:~334, phase 3 calls getAggregateFunction() which falls to the else branch: aggFunc.withDistinctAndChildren(false, children). This wraps the already-array intermediate slot in another array_agg(), producing ARRAY<ARRAY<T>>. A query with a skew hint on these functions would return deeply nested arrays. Consider adding a guard in canSkewRewrite() or handling array concatenation in the skew phases.
3. [MEDIUM] DistinctWindowExpression doesn't handle ArrayAgg/CollectList
DistinctWindowExpression.java:87-101 has hardcoded instanceof checks for Count, Sum, GroupConcat — it never queries SupportMultiDistinct. ARRAY_AGG(DISTINCT col) OVER(...) or COLLECT_LIST(DISTINCT col) OVER(...) would not be converted. Consider refactoring to use the SupportMultiDistinct interface like other rules do.
4. [LOW] get_name() returns "collect_set" for multi_distinct_* functions
aggregate_function_collect.h:410-416: get_name() returns "collect_set" for all Set-based data. Three different functions (collect_set, multi_distinct_collect_list, multi_distinct_array_agg) all report as "collect_set" in EXPLAIN ANALYZE profiles and error messages, making debugging confusing.
5. [LOW] Unused #include
See inline comment in aggregate_function_array_agg.cpp.
6. [LOW] convertToMultiDistinct silently drops limit argument
See inline comment in CollectList.java.
| template <PrimitiveType T> | ||
| AggregateFunctionPtr do_create_agg_function_collect(const DataTypes& argument_types, | ||
| AggregateFunctionPtr do_create_agg_function_collect(bool distinct, const DataTypes& argument_types, | ||
| const bool result_is_nullable, |
There was a problem hiding this comment.
🐛 Null elements are silently dropped in the multi-distinct path
The distinct path creates AggregateFunctionCollect<AggregateFunctionCollectSetData<T, false>, false> via creator_without_type::create. This factory wraps the inner function in AggregateFunctionNullUnaryInline (see aggregate_function_null.h:331-342), which skips null rows and never calls the inner add(). Additionally, AggregateFunctionCollectSetData uses flat_hash_set<ElementType> with no null map — it cannot represent null elements.
In contrast, the non-distinct array_agg path uses AggregateFunctionArrayAggData which explicitly tracks nulls via a null_map.
Concrete failure:
SELECT array_agg(distinct col) FROM (
SELECT CAST(NULL AS INT) AS col UNION ALL SELECT 1 AS col
) tReturns [1] instead of [null, 1].
The behavior change is silent — it only triggers when multiple distinct aggregates cause the multi-distinct rewrite to fire.
Suggested fix: The multi_distinct_array_agg path needs a data structure that combines hash-set dedup with null tracking, similar to what AggregateFunctionArrayAggData does.
|
|
||
| #include "exprs/aggregate/aggregate_function_array_agg.h" | ||
|
|
||
| #include "common/exception.h" |
There was a problem hiding this comment.
🧹 Unused include
#include "common/status.h" is added but Status is never used in this file. Only Exception (from "common/exception.h") is thrown in the new distinct path. Consider removing this include to follow include-what-you-use hygiene.
|
|
||
| @Override | ||
| public AggregateFunction convertToMultiDistinct() { | ||
| Preconditions.checkArgument(distinct, |
There was a problem hiding this comment.
When CollectList is called with 2 children (the collect_list(expr, limit) variant), the limit argument children.get(1) is silently discarded. Only children.get(0) is passed to MultiDistinctCollectList. The limit parameter has no effect in the multi-distinct path.
Consider either:
- Documenting that the limit is intentionally dropped in the multi-distinct path, or
- Simplifying to always use
child(0)since thechildren.size() == 1vs else branch both result in the same effective behavior.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: close #65244
Problem Summary:
When a single GROUP BY contained more than one DISTINCT aggregate and any of
them was collect_list(distinct ...) or array_agg(distinct ...), planning failed
with "... can't support multi distinct". CheckMultiDistinct rejects any distinct
aggregate that is not a SupportMultiDistinct, and collect_list / array_agg did
not implement that interface, so queries such as
could not run at all.
This adds the missing multi-distinct variants and wires them into the existing
framework:
the new MultiDistinctCollectList / MultiDistinctArrayAgg functions. The new
functions implement MultiDistinction, are registered in
BuiltinAggregateFunctions, and get visitor hooks in AggregateFunctionVisitor.
dedup in the aggregate state via the existing Set-backed collect data, because
the Nereids multi-distinct plan does not push the distinct argument into the
group-by key.
Release note
Support multiple DISTINCT aggregates in one GROUP BY when they include
collect_list(distinct ...) or array_agg(distinct ...).
Check List (For Author)