github-actions[bot] commented on code in PR #65368:
URL: https://github.com/apache/doris/pull/65368#discussion_r3575673126
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckMultiDistinct.java:
##########
@@ -42,43 +34,9 @@ public Rule build() {
}
private LogicalAggregate checkDistinct(LogicalAggregate<? extends Plan>
aggregate) {
- if (aggregate.getDistinctArguments().size() > 1) {
-
- for (AggregateFunction func : aggregate.getAggregateFunctions()) {
- if (func.isDistinct() && !(func instanceof
SupportMultiDistinct)) {
- throw new AnalysisException(func.toString() + " can't
support multi distinct.");
- }
- }
- }
-
- boolean distinctMultiColumns = false;
- for (AggregateFunction func : aggregate.getAggregateFunctions()) {
- if (!func.isDistinct()) {
- continue;
- }
- if (func.arity() <= 1) {
- continue;
- }
- for (int i = 1; i < func.arity(); i++) {
- if (!func.child(i).getInputSlots().isEmpty() &&
!(func.child(i) instanceof OrderExpression)) {
- // think about group_concat(distinct col_1, ',')
- distinctMultiColumns = true;
- break;
- }
- }
- if (distinctMultiColumns) {
- break;
- }
- }
-
- long distinctFunctionNum = 0;
- for (AggregateFunction aggregateFunction :
aggregate.getAggregateFunctions()) {
- distinctFunctionNum += aggregateFunction.isDistinct() ? 1 : 0;
- }
-
- if (distinctMultiColumns && distinctFunctionNum > 1) {
+ if (AggregateUtils.distinctArgumentGroupCountUpToTwo(aggregate) > 1) {
Review Comment:
This drops the old support boundary for multi-argument DISTINCT aggregates
that do not implement `SupportMultiDistinct`. For example, `corr(distinct a,
b)` binds to `Corr` with `distinct=true` and two arguments, but `Corr` is not
`SupportMultiDistinct` and has no legality check rejecting DISTINCT. With the
new condition, `distinctArgumentGroupCountUpToTwo()` returns `1` for that
single aggregate, so the final check passes. In paths that choose the
multi-distinct rewrite, `MultiDistinctFunctionStrategy` only converts
`SupportMultiDistinct` functions and otherwise leaves the aggregate unchanged,
so a plan can still contain `corr(DISTINCT a, b)` instead of being rejected.
The old code rejected this because the aggregate-wide distinct argument set had
size greater than one and the function was not `SupportMultiDistinct`. Please
keep an explicit support/legality guard for non-whitelisted multi-argument
DISTINCT functions, or add intentional support plus regression coverage for a
two-ar
gument aggregate such as `corr(distinct a,b)` with and without `GROUP BY`.
--
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]
