morrySnow commented on code in PR #65099:
URL: https://github.com/apache/doris/pull/65099#discussion_r3504740974
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -277,6 +289,32 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
return agg;
}
+ private Optional<List<SlotReference>>
getCommonDistinctKeys(Set<AggregateFunction> aggFunctions) {
+ List<SlotReference> commonDistinctKeys = null;
+ for (AggregateFunction aggFunction : aggFunctions) {
+ if (!aggFunction.isDistinct() ||
!pushDownAggFunctionSet.contains(aggFunction.getClass())
+ || aggFunction.containsVolatileExpression()) {
+ return Optional.empty();
+ }
+ List<SlotReference> distinctKeys = new ArrayList<>();
+ for (Expression child : aggFunction.children()) {
+ if (!(child instanceof SlotReference)) {
+ return Optional.empty();
+ }
+ distinctKeys.add((SlotReference) child);
+ }
+ if (distinctKeys.isEmpty()) {
+ return Optional.empty();
+ }
+ if (commonDistinctKeys == null) {
+ commonDistinctKeys = distinctKeys;
+ } else if (!commonDistinctKeys.equals(distinctKeys)) {
Review Comment:
**PLAUSIBLE:** `getCommonDistinctKeys` uses `List.equals()` to compare
distinct keys, which is order-sensitive. For multi-column distinct aggregates
like `COUNT(DISTINCT a, b)` and `SUM(DISTINCT b, a)` in the same query, the two
functions share the same distinct tuple `{a, b}` but the argument lists are in
different orders. `[a, b].equals([b, a])` returns `false`, so the method
returns `Optional.empty()` and the optimization is skipped entirely.
**Impact:** No wrong results — the fallback is to leave the original plan
unchanged. But the dedup-pushdown optimization is missed for this (admittedly
unusual) edge case.
**Suggested fix:** Sort the distinct key list before comparison (e.g., by
`ExprId`), or convert to a `Set<SlotReference>` for order-independent
comparison.
```suggestion
} else if (distinctKeys.size() != commonDistinctKeys.size()
|| !new
HashSet<>(commonDistinctKeys).containsAll(distinctKeys)) {
```
--
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]
