github-actions[bot] commented on code in PR #61248:
URL: https://github.com/apache/doris/pull/61248#discussion_r3272369619
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctAggregateRewriter.java:
##########
@@ -110,6 +119,9 @@ boolean shouldUseMultiDistinct(LogicalAggregate<? extends
Plan> aggregate) {
Statistics aggStats = aggregate.getStats();
Statistics aggChildStats = aggregate.child().getStats();
Set<Expression> dstArgs = aggregate.getDistinctArguments();
+ if (isDistinctKeySatisfyDistribution(aggregate)) {
+ return false;
Review Comment:
This shortcut can force the split strategy before we know the aggregate is
actually splittable. For example, on a table distributed by `a`, `select b,
count(distinct a), group_concat(c order by d) ... group by b` with
`use_one_phase_agg_for_group_concat_with_order=true` reaches this branch
because the distinct key satisfies distribution. `rewrite()` then calls
`splitDistinctAgg()`, but that returns `null` because non-distinct
`GroupConcat` is not in `supportSplitOtherFunctions`. The original distinct
aggregate is left for physical splitting, where `SplitAggMultiPhase` only
produces multi-phase alternatives and `GroupConcat` with order rejects every
phase except `ONE`, so the query can lose its valid implementation. Please only
apply the distribution override when `splitDistinctAgg` can handle the
aggregate, or otherwise fall back to the previous multi_distinct decision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
