github-actions[bot] commented on code in PR #65368:
URL: https://github.com/apache/doris/pull/65368#discussion_r3575686483
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -169,7 +187,7 @@ public static Set<NamedExpression>
getGroupBySetNamedExpr(LogicalAggregate<? ext
public static Set<NamedExpression> getDistinctNamedExpr(LogicalAggregate<?
extends Plan> aggregate) {
return aggregate.getAggregateFunctions().stream()
.filter(AggregateFunction::isDistinct)
- .flatMap(aggFunc -> aggFunc.getArguments().stream())
+ .flatMap(aggFunc -> aggFunc.getDistinctArguments().stream())
Review Comment:
This helper also needs to preserve legal non-literal constant separators for
`GROUP_CONCAT(DISTINCT ...)`. The new legality check allows any deterministic
constant expression, so `group_concat(distinct a, concat(',', ''))` or
`group_concat(distinct a, cast(',' as varchar))` passes analysis.
`NormalizeAggregate` only skips literal children; non-literal constant children
are pushed into the bottom project and the aggregate is normalized to reference
a separator slot. After this change, `getDistinctNamedExpr()` returns only
`GroupConcat.getDistinctArguments()` (the value argument), so the split lower
aggregate keeps the group keys and value slot but drops the normalized
separator slot. The upper rewrite still uses
`aggFunc.withDistinctAndChildren(false, aggFunc.children())`, leaving
`group_concat(value_slot, separator_slot)` above a child that no longer outputs
`separator_slot`. Please either fold/canonicalize constant separators to
literals before splitting, or carry allowed separat
or slots through the lower aggregate without treating them as DISTINCT keys.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -147,6 +148,23 @@ public static boolean
containsCountDistinctMultiExpr(LogicalAggregate<? extends
expr instanceof Count && ((Count) expr).isDistinct() &&
expr.arity() > 1);
}
+ /** count agg function distinct group, up to 2*/
+ public static int distinctArgumentGroupCountUpToTwo(Aggregate<? extends
Plan> aggregate) {
+ Set<Expression> distinctArgumentGroup = null;
+ for (AggregateFunction aggregateFunction :
aggregate.getAggregateFunctions()) {
+ if (!aggregateFunction.isDistinct()) {
+ continue;
+ }
+ Set<Expression> currentGroup =
ImmutableSet.copyOf(aggregateFunction.getDistinctArguments());
Review Comment:
This same-set grouping still needs to canonicalize duplicate multi-column
COUNT arguments before the Cascades split path. For example, `count(distinct a,
a, b)` and `count(distinct a, b)` now both produce the distinct group `{a,b}`,
so this helper returns `1` and the aggregate can be left for
`SplitAggMultiPhase`. The lower dedup key is also built from the unique
distinct set, but the upper COUNT rewrite still calls
`countDistinctMultiExprToCountIf()`, which computes an `ImmutableSet` and then
uses that set size to index the original positional argument list. With
`count(distinct a, a, b)`, the set size is 2, so it only checks child 0/1 (`a`,
`a`) and never checks nullable `b`; a deduped row `(a=1, b=NULL)` is counted
even though multi-column COUNT DISTINCT should ignore rows with any null
argument. Please either canonicalize/reject duplicate COUNT DISTINCT arguments
before treating functions as the same group, or make
`countDistinctMultiExprToCountIf()` build the null filter from
the ordered unique distinct arguments used by the split.
--
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]
