github-actions[bot] commented on code in PR #65206:
URL: https://github.com/apache/doris/pull/65206#discussion_r3602141363
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:
##########
@@ -447,6 +449,28 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan>
aggregate, Optional<Logi
having.get().withChildren(new
LogicalProject<>(bottomProjectsBuilder.build(), newAggregate)));
}
+ private List<NamedExpression>
normalizeMultiColumnDistinctCount(List<NamedExpression> aggregateOutput) {
+ // Multi-column distinct counts treat arguments as a set. Remove
duplicates and canonicalize equivalent
+ // counts to the first argument order so the structural equality used
below can share one aggregate result.
+ Map<ImmutableSet<Expression>, Count> distinctArgumentsToCount = new
HashMap<>();
+ return ExpressionUtils.rewriteDownShortCircuit(aggregateOutput,
expression -> {
+ if (!(expression instanceof Count)) {
+ return expression;
+ }
+ Count count = (Count) expression;
+ if (!count.isDistinct() || count.arity() <= 1) {
+ return count;
+ }
+ ImmutableSet<Expression> distinctArguments =
ImmutableSet.copyOf(count.getDistinctArguments());
+ Count normalizedCount =
distinctArgumentsToCount.get(distinctArguments);
+ if (normalizedCount == null) {
+ normalizedCount = count.withDistinctAndChildren(true,
ImmutableList.copyOf(distinctArguments));
Review Comment:
Preserve the original multi-argument legality before collapsing this to one
child. For example, `count(DISTINCT bitmap_col, bitmap_col)` currently reaches
this code as a legal-to-bind two-argument `Count`; after this assignment it is
unary, so the later `CountDistinctRewrite` converts it to `bitmap_union_count`
instead of `Count.checkLegalityAfterRewrite` rejecting the unsupported tuple.
Those operations are not equivalent: for bitmap rows `{1,2}` and `{2,3}`, union
count is 3, while there are 2 distinct `(bitmap_col, bitmap_col)` tuples. HLL
has the same unary-rewrite problem. Please ensure only a genuinely unary source
`Count` can enter those special rewrites (and add duplicate BITMAP/HLL negative
tests).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:
##########
@@ -447,6 +449,28 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan>
aggregate, Optional<Logi
having.get().withChildren(new
LogicalProject<>(bottomProjectsBuilder.build(), newAggregate)));
}
+ private List<NamedExpression>
normalizeMultiColumnDistinctCount(List<NamedExpression> aggregateOutput) {
+ // Multi-column distinct counts treat arguments as a set. Remove
duplicates and canonicalize equivalent
+ // counts to the first argument order so the structural equality used
below can share one aggregate result.
+ Map<ImmutableSet<Expression>, Count> distinctArgumentsToCount = new
HashMap<>();
+ return ExpressionUtils.rewriteDownShortCircuit(aggregateOutput,
expression -> {
+ if (!(expression instanceof Count)) {
+ return expression;
+ }
+ Count count = (Count) expression;
+ if (!count.isDistinct() || count.arity() <= 1) {
+ return count;
+ }
+ ImmutableSet<Expression> distinctArguments =
ImmutableSet.copyOf(count.getDistinctArguments());
+ Count normalizedCount =
distinctArgumentsToCount.get(distinctArguments);
+ if (normalizedCount == null) {
+ normalizedCount = count.withDistinctAndChildren(true,
ImmutableList.copyOf(distinctArguments));
+ distinctArgumentsToCount.put(distinctArguments,
normalizedCount);
+ }
+ return count.withDistinctAndChildren(true,
normalizedCount.children());
Review Comment:
Preserve the conservative preaggregation decision for every retained
argument. On an AGG_KEYS table `t(k INT, v BIGINT SUM) AGGREGATE KEY(k)`,
`count(DISTINCT k,v), count(DISTINCT v,k)` previously left both ordered Counts,
so the value-first Count made `SetPreAggStatus` choose PREAGG OFF. Sharing both
as the first `[k,v]` ordering here leaves one key-first multi-child Count.
`SetPreAggStatus`'s multi-child path validates only `child(0)`, chooses ON, and
ignores the retained `v`. BE documents that ON may return partial aggregate
rows: two rowsets with `(1,1)` and `(1,2)` can then be counted as two tuples,
while OFF fully combines them to `(1,3)` and counts one. Please validate every
Count child or disable preaggregation for this shape, and add an AGG_KEYS
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:
##########
@@ -447,6 +449,28 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan>
aggregate, Optional<Logi
having.get().withChildren(new
LogicalProject<>(bottomProjectsBuilder.build(), newAggregate)));
}
+ private List<NamedExpression>
normalizeMultiColumnDistinctCount(List<NamedExpression> aggregateOutput) {
+ // Multi-column distinct counts treat arguments as a set. Remove
duplicates and canonicalize equivalent
+ // counts to the first argument order so the structural equality used
below can share one aggregate result.
+ Map<ImmutableSet<Expression>, Count> distinctArgumentsToCount = new
HashMap<>();
+ return ExpressionUtils.rewriteDownShortCircuit(aggregateOutput,
expression -> {
+ if (!(expression instanceof Count)) {
+ return expression;
+ }
+ Count count = (Count) expression;
+ if (!count.isDistinct() || count.arity() <= 1) {
+ return count;
+ }
+ ImmutableSet<Expression> distinctArguments =
ImmutableSet.copyOf(count.getDistinctArguments());
+ Count normalizedCount =
distinctArgumentsToCount.get(distinctArguments);
+ if (normalizedCount == null) {
+ normalizedCount = count.withDistinctAndChildren(true,
ImmutableList.copyOf(distinctArguments));
+ distinctArgumentsToCount.put(distinctArguments,
normalizedCount);
+ }
+ return count.withDistinctAndChildren(true,
normalizedCount.children());
Review Comment:
This sharing newly makes the target two-output query eligible for a
downstream rule that cannot handle a multi-column Count. For example:
```sql
SELECT /*+ USE_CBO_RULE(PUSH_DOWN_AGG_WITH_DISTINCT_THROUGH_JOIN_ONE_SIDE) */
count(DISTINCT l.a, l.b), count(DISTINCT l.b, l.a)
FROM l JOIN r ON l.k = r.k;
```
Before this change the normalized aggregate has two ordered `Count`
functions, so `PushDownAggWithDistinctThroughJoinOneSide` rejects `funcs.size()
> 1`. After both outputs share this one two-child Count, that gate passes; the
rule inspects/groups only `child(0)` and then removes DISTINCT while retaining
both children, producing invalid `count(l.a,l.b)` (and potentially dropping
`l.b` from the pushed-side output). BE `count` only accepts arity 0–1. Please
make that downstream rule require unary functions or preserve/group every
distinct argument, and add this hinted reordered-count join case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:
##########
@@ -447,6 +449,28 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan>
aggregate, Optional<Logi
having.get().withChildren(new
LogicalProject<>(bottomProjectsBuilder.build(), newAggregate)));
}
+ private List<NamedExpression>
normalizeMultiColumnDistinctCount(List<NamedExpression> aggregateOutput) {
+ // Multi-column distinct counts treat arguments as a set. Remove
duplicates and canonicalize equivalent
+ // counts to the first argument order so the structural equality used
below can share one aggregate result.
+ Map<ImmutableSet<Expression>, Count> distinctArgumentsToCount = new
HashMap<>();
+ return ExpressionUtils.rewriteDownShortCircuit(aggregateOutput,
expression -> {
Review Comment:
Avoid descending into `WindowExpression` here. A reduced triggering tree is:
```text
Aggregate(groupBy=[id], outputs=[id, sum(v1), Window(count(DISTINCT id,
id))])
Scan
```
This traversal changes the window function to unary `count(DISTINCT id)`
before `ExtractAndNormalizeWindowExpression` / `DistinctWindowExpression`, so
it is accepted as `MultiDistinctCount`. The same `count(DISTINCT id,id) OVER()`
without an unrelated aggregate still reaches the documented `COUNT with
DISTINCT only support 1 parameter in analytic function` error.
`NormalizeAggregate` is also explicitly expected to leave window expressions
unchanged by `ExtractAndNormalizeWindowExpressionTest`. Please restrict this
canonicalization to non-window aggregate functions (or change the window
contract consistently in its dedicated rule and tests).
--
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]
