github-actions[bot] commented on code in PR #65099:
URL: https://github.com/apache/doris/pull/65099#discussion_r3662607893
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -129,15 +131,25 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
boolean hasDecomposedAggIf = false;
boolean containsNullToNonNull = false;
Map<NamedExpression, List<AggregateFunction>>
aggFunctionsForOutputExpressions = Maps.newHashMap();
- for (NamedExpression aggOutput : agg.getOutputExpressions()) {
- List<AggregateFunction> funcs = Lists.newArrayList();
- aggFunctionsForOutputExpressions.put(aggOutput, funcs);
- for (Object obj :
aggOutput.collect(AggregateFunction.class::isInstance)) {
- AggregateFunction aggFunction = (AggregateFunction) obj;
- if (aggFunction.isDistinct()) {
- return agg;
- }
- if (pushDownAggFunctionSet.contains(aggFunction.getClass())) {
+ Set<AggregateFunction> allAggFunctions = agg.getAggregateFunctions();
+ boolean hasDistinctAgg =
allAggFunctions.stream().anyMatch(AggregateFunction::isDistinct);
+ if (hasDistinctAgg) {
Review Comment:
This new DISTINCT branch exposes a semantic project-pruning bug. A reachable
normalized tree is:
```text
Aggregate(count(DISTINCT id#1))
Project(id#1, assert_true(name#2 <> 'bad', 'bad') AS guard#9)
InnerJoin(id#1 = id#3)
Scan t1
Scan t2
```
Column pruning runs before eager aggregation and deliberately retains
`guard#9` because `assert_true` is a `NoneMovableFunction`. After this branch
adds `id#1` to the dedup group keys, the eager walker changes the join
children; `visitLogicalProject` then reconstructs the project only from context
aggregate outputs and group keys, so it emits `id#1` and silently drops
`guard#9`. A joined row with `name = 'bad'` now returns a count instead of
raising the required error. Previously DISTINCT caused an immediate return, so
this is newly reachable. Please either refuse this DISTINCT pushdown through a
project containing a non-movable expression, or carry the non-movable
expression and every required input slot through the lower grouping/output
context before rebuilding it; add an `assert_true` regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -129,15 +131,25 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
boolean hasDecomposedAggIf = false;
boolean containsNullToNonNull = false;
Map<NamedExpression, List<AggregateFunction>>
aggFunctionsForOutputExpressions = Maps.newHashMap();
- for (NamedExpression aggOutput : agg.getOutputExpressions()) {
- List<AggregateFunction> funcs = Lists.newArrayList();
- aggFunctionsForOutputExpressions.put(aggOutput, funcs);
- for (Object obj :
aggOutput.collect(AggregateFunction.class::isInstance)) {
- AggregateFunction aggFunction = (AggregateFunction) obj;
- if (aggFunction.isDistinct()) {
- return agg;
- }
- if (pushDownAggFunctionSet.contains(aggFunction.getClass())) {
+ Set<AggregateFunction> allAggFunctions = agg.getAggregateFunctions();
+ boolean hasDistinctAgg =
allAggFunctions.stream().anyMatch(AggregateFunction::isDistinct);
+ if (hasDistinctAgg) {
+ // Keep the distinct aggregate functions unchanged at the upper
aggregate. The common distinct keys
+ // become extra group keys for duplicate elimination below joins.
+ Optional<Set<SlotReference>> distinctKeys =
getCommonDistinctKeys(allAggFunctions);
+ if (!distinctKeys.isPresent()) {
+ return agg;
+ }
+ groupKeys.addAll(distinctKeys.get());
Review Comment:
The distinct-only path drops the aggregate functions from
`PushDownAggContext`, which also drops their `force_eager_agg_hint` actions.
For example:
```text
Aggregate(count(DISTINCT t1.score))
InnerJoin(t1.id = t2.id)
Scan t1
Scan t2
```
With `eager_aggregation_mode=1` and
`force_eager_agg_hint='count:t1.score=nopush'`, `getCommonDistinctKeys` adds
`t1.score` to `groupKeys`, but this constructor receives an empty
`aggFunctions` list. `PushDownAggContext` therefore never calls
`EagerAggHints.decide`, and `genAggregate` sees no `NOPUSH` action and still
creates the lower dedup aggregates. The inverse `push` action is also
ineffective in statistics mode. This contradicts the session variable's
documented branch-level `push`/`nopush` behavior. Please retain/register the
original DISTINCT functions as control metadata even though the pushed
aggregate payload is empty, and add plan-shape tests for both actions (the new
`with_hint_` block currently contains no hints).
##########
regression-test/suites/query_p0/eager_agg/distinct_agg_func_push_down.groovy:
##########
@@ -0,0 +1,256 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("push_down_count_distinct_through_join_one_side") {
+ sql "set runtime_filter_mode=OFF"
+ sql "set eager_aggregation_mode=1;"
+ sql """
+ DROP TABLE IF EXISTS count_with_distinct_t;
+ """
+
+ sql """
+ CREATE TABLE IF NOT EXISTS count_with_distinct_t(
+ `id` int(32),
+ `score` int(64) NULL,
+ `name` varchar(64) NULL
+ ) ENGINE = OLAP
+ DISTRIBUTED BY HASH(id) BUCKETS 4
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ );
+ """
+
+ sql "insert into count_with_distinct_t values (1, 1, 'a')"
+ sql "insert into count_with_distinct_t values (2, null, 'a')"
+ sql "insert into count_with_distinct_t values (3, 1, null)"
+ sql "insert into count_with_distinct_t values (4, 2, 'b')"
+ sql "insert into count_with_distinct_t values (5, null, 'b')"
+ sql "insert into count_with_distinct_t values (6, 2, null)"
+ sql "insert into count_with_distinct_t values (7, 3, 'c')"
+ sql "insert into count_with_distinct_t values (8, null, 'c')"
+ sql "insert into count_with_distinct_t values (9, 3, null)"
+ sql "insert into count_with_distinct_t values (10, null, null)"
+ sql "analyze table count_with_distinct_t with full with sync;"
+
+ order_qt_groupby_pushdown_basic """
+ select count(distinct t1.score) from count_with_distinct_t t1,
count_with_distinct_t t2 where t1.id = t2.id group by t1.name;
+ """
+
+ order_qt_groupby_pushdown_left_join """
+ select count(distinct t1.score) from count_with_distinct_t t1 left
join count_with_distinct_t t2 on t1.id = t2.id group by t1.name;
+ """
+
+ order_qt_groupby_pushdown_right_join """
+ select count(distinct t1.score) from count_with_distinct_t t1 right
join count_with_distinct_t t2 on t1.id = t2.id group by t1.name;
+ """
+
+ order_qt_groupby_pushdown_full_join """
+ select count(distinct t1.score) from count_with_distinct_t t1 full
join count_with_distinct_t t2 on t1.id = t2.id group by t1.name;
+ """
+
+ order_qt_groupby_pushdown_left_semi_join """
+ select count(distinct t1.score) from count_with_distinct_t t1 inner
join count_with_distinct_t t2 on t1.id = t2.id group by t1.name;
Review Comment:
This case is named `left_semi_join`, but the SQL uses an `inner join`, so
the semi-join branch is never exercised. The surrounding outer/anti cases also
self-join the same fully matching `id` domain, which makes every outer result
inner-like and the anti oracle empty. Because the new distinct-only path
explicitly deduplicates eligible semi/anti/outer branches, please use an actual
left semi join and inputs with left-only/right-only keys (including a nullable
distinct key) so retained-side and null-extension errors would change the
result; a lower-aggregate plan assertion would also prove the intended rewrite
ran.
--
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]
