morrySnow commented on code in PR #65368:
URL: https://github.com/apache/doris/pull/65368#discussion_r3542829267
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -147,6 +151,30 @@ public static boolean
containsCountDistinctMultiExpr(LogicalAggregate<? extends
expr instanceof Count && ((Count) expr).isDistinct() &&
expr.arity() > 1);
}
+ /**
+ * Collect distinct aggregate functions by their distinct argument set.
+ * The order of arguments does not distinguish groups here, so
count(distinct a, b)
+ * and count(distinct b, a) belong to the same group.
Review Comment:
返回类型 `Map<Set<Expression>, List<AggregateFunction>>` 过于厚重 — 所有 4
个调用点（`SplitAggMultiPhase`、`SplitAggMultiPhaseWithoutGbyKey`、`DistinctAggStrategySelector`、`CheckMultiDistinct`）都只调用了
`.size()`。`List<AggregateFunction>` 中的值从未被读取，每个 group 的 `ArrayList` 分配都被浪费了。
建议：改为仅返回 `Set<Set<Expression>>`，或一个简单的 `int countDistinctArgumentGroups()`
方法，甚至可以添加短路逻辑（例如当发现 2 个 group 时立即停止）。由于该方法在 `.when()` 规则条件中被调用（Cascades
优化器的热路径），减少分配对性能很重要。
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -147,6 +151,30 @@ public static boolean
containsCountDistinctMultiExpr(LogicalAggregate<? extends
expr instanceof Count && ((Count) expr).isDistinct() &&
expr.arity() > 1);
}
+ /**
+ * Collect distinct aggregate functions by their distinct argument set.
+ * The order of arguments does not distinguish groups here, so
count(distinct a, b)
+ * and count(distinct b, a) belong to the same group.
+ */
+ public static Map<Set<Expression>, List<AggregateFunction>>
collectDistinctArgumentGroups(
+ Aggregate<? extends Plan> aggregate) {
+ Map<Set<Expression>, List<AggregateFunction>> distinctGroups = new
LinkedHashMap<>();
+ for (NamedExpression outputExpression :
aggregate.getOutputExpressions()) {
+ outputExpression.foreach(expression -> {
+ if (expression instanceof AggregateFunction) {
+ AggregateFunction aggregateFunction = (AggregateFunction)
expression;
Review Comment:
`ImmutableSet.copyOf(aggregateFunction.getDistinctArguments())` 这个分组键计算与
`SplitMultiDistinctStrategy.collectDistinctAndNonDistinctFunctions()`（第 160
行：`ImmutableSet.copyOf(aggFunc.getDistinctArguments())`）完全相同。
建议让 `SplitMultiDistinctStrategy` 也复用这个新的工具方法，避免逻辑重复。
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -147,6 +151,30 @@ public static boolean
containsCountDistinctMultiExpr(LogicalAggregate<? extends
expr instanceof Count && ((Count) expr).isDistinct() &&
expr.arity() > 1);
}
+ /**
+ * Collect distinct aggregate functions by their distinct argument set.
+ * The order of arguments does not distinguish groups here, so
count(distinct a, b)
+ * and count(distinct b, a) belong to the same group.
+ */
+ public static Map<Set<Expression>, List<AggregateFunction>>
collectDistinctArgumentGroups(
+ Aggregate<? extends Plan> aggregate) {
Review Comment:
这里手动遍历 `getOutputExpressions()` 并通过 `outputExpression.foreach(...)` 进行树遍历，而
`Aggregate.getAggregateFunctions()`（Aggregate.java:67）已经提供了相同的功能。
建议改为迭代 `aggregate.getAggregateFunctions()`，然后过滤并分组返回的 `AggregateFunction`
实例。这可以避免重复遍历逻辑，并与其他方法（如
`hasDistinctFunc()`、`distinctFuncNum()`、`hasSkewHint()`）保持一致的代码模式。
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckMultiDistinct.java:
##########
@@ -42,43 +40,10 @@ 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) {
- throw new AnalysisException(
- "The query contains multi count distinct or sum distinct,
each can't have multi columns");
+ Map<Set<Expression>, List<AggregateFunction>> distinctGroups =
+ AggregateUtils.collectDistinctArgumentGroups(aggregate);
+ if (distinctGroups.size() > 1) {
+ throw new AnalysisException("Multiple distinct argument groups
remain after aggregate rewrite");
Review Comment:
新的错误信息 "Multiple distinct argument groups remain after aggregate rewrite"
面向的是内部实现概念，对用户来说不够友好。
旧代码的错误信息明确指出了具体的函数名（例如 `"ndv(distinct a) can't support multi
distinct."`），对调试更有帮助。虽然没有正确性问题（新旧代码都会在所有实际场景中抛出错误），但信息质量的下降值得注意。
--
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]
