github-actions[bot] commented on code in PR #66570:
URL: https://github.com/apache/doris/pull/66570#discussion_r3746550178
##########
fe/fe-core/src/test/java/org/apache/doris/qe/LocalExchangePlannerTest.java:
##########
@@ -163,6 +163,61 @@ public void testAggWithoutKeyTwoPhase() throws Exception {
olapScan("t1")))))));
}
+ @Test
+ public void testCountDistinctNoGroupByRequiresHashBeforeAgg() throws
Exception {
+ // count(distinct k2) without group-by: the finalize merge agg emits
per-instance
+ // scalar values that the parent sums
(sum0(multi_distinct_count(...))), so its
+ // input must be hash-partitioned by the distinct key. With the
broadcast-join
+ // probe forced to PASSTHROUGH, rows are scattered below the join —
the agg must
+ // still get a LOCAL_HASH exchange directly beneath it. Pre-fix this
agg received
+ // NoRequire (no hash LE) and the parent double-counted overlapping
keys.
+ // agg_phase=1 forces the multi_distinct_count two-phase shape (the
default
+ // strategy splits into a group-by + count shape which is already
safe).
+ setupLocalShuffleSession(sv -> {
+ sv.enableBroadcastJoinForcePassthrough = true;
+ sv.aggPhase = 1;
+ });
+ assertHasLocalExchangeOfType("select count(distinct a.k2) from test.t1
a "
Review Comment:
[P2] Assert the distinct key, not just the HASH type
assertHasLocalExchangeOfType flattens every local exchange in every fragment
to an enum set, discarding both its plan-tree location and distributeExprLists.
The correctness invariant here is that the exchange directly below the scalar
multi_distinct_count finalize aggregate is keyed by a.k2; a keyless or
wrong-key LOCAL_EXECUTION_HASH_SHUFFLE still passes this assertion (and the new
mock fixture currently accepts a zero-key HASH node). Please use the existing
structural plan-shape traversal, or an equivalent targeted walk, to assert that
aggregate-to-exchange edge and its partition expressions.
##########
fe/fe-core/src/test/java/org/apache/doris/qe/LocalExchangePlannerTest.java:
##########
@@ -163,6 +163,61 @@ public void testAggWithoutKeyTwoPhase() throws Exception {
olapScan("t1")))))));
}
+ @Test
+ public void testCountDistinctNoGroupByRequiresHashBeforeAgg() throws
Exception {
+ // count(distinct k2) without group-by: the finalize merge agg emits
per-instance
+ // scalar values that the parent sums
(sum0(multi_distinct_count(...))), so its
+ // input must be hash-partitioned by the distinct key. With the
broadcast-join
+ // probe forced to PASSTHROUGH, rows are scattered below the join —
the agg must
+ // still get a LOCAL_HASH exchange directly beneath it. Pre-fix this
agg received
+ // NoRequire (no hash LE) and the parent double-counted overlapping
keys.
+ // agg_phase=1 forces the multi_distinct_count two-phase shape (the
default
+ // strategy splits into a group-by + count shape which is already
safe).
+ setupLocalShuffleSession(sv -> {
+ sv.enableBroadcastJoinForcePassthrough = true;
+ sv.aggPhase = 1;
+ });
+ assertHasLocalExchangeOfType("select count(distinct a.k2) from test.t1
a "
+ + "left join [shuffle] test.t2 b on a.k2 = b.k2 "
+ + "left join [broadcast] test.t2 c on b.k1 = c.k1",
+ LocalExchangeType.LOCAL_EXECUTION_HASH_SHUFFLE);
+
+ }
+
+ @Test
+ public void
testCountDistinctNoGroupByRequiresHashWithoutForcePassthrough() throws
Exception {
+ // Same as above without the broadcast-join force-passthrough: the
join output is
+ // hash-partitioned by the probe key, and the satisfy() check lets the
agg keep
+ // that distribution without inserting a redundant LE — the hash
demand must
+ // still be recognized (a plain hash join probe output satisfies it).
+ setupLocalShuffleSession(null);
Review Comment:
[P2] Configure and verify the no-force scenario explicitly
This call resets neither aggPhase nor enableBroadcastJoinForcePassthrough.
Because the test class keeps one ConnectContext, it may inherit aggPhase=1 and
force-passthrough=true from the other new test; in a clean session it instead
uses the default aggregate strategy that the preceding comment says is already
safe. In either case, anyTree(agg()) is true for every successful
count(distinct ...) plan and cannot show that a compatible child hash avoided a
redundant LE. Set aggPhase=1 and force-passthrough=false here (and reset shared
state in setup), then assert the claimed aggregate-child distribution.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/AggregationNode.java:
##########
@@ -335,7 +335,13 @@ public Pair<PlanNode, LocalExchangeType>
enforceAndDeriveLocalExchange(
// early return also catches FIRST_MERGE, dropping the HASH
requirement and
// causing wrong-result (e.g. PASSTHROUGH over serial child breaks
the
// group-by-key invariant — DORIS-25413).
- if (!hasKeys) {
+ if (!hasPartitionRequirement()) {
Review Comment:
[P1] Do not create a zero-key HASH exchange for direct multi-distinct calls
This predicate also matches a directly called scalar
multi_distinct_count(col). That function has isDistinct=false, so
SplitAggWithoutDistinct builds a two-phase LOCAL aggregate with no
partitionExpressions; over a non-hash scan both the child distribution list and
grouping list are empty. With the default LE setting this branch now reaches
requireHash(), and getLocalExchangeDistributeExprs() supplies zero expressions.
BE's hash partitioner initializes every row to hash 0, so the resulting
FE-planned HASH exchange sends the whole input to channel 0, collapsing the
partial aggregate to one task per BE and concentrating its distinct state in
one task. BE's own AggSinkOperatorX sees empty _partition_exprs on this path
and uses the base requirement instead. Please base the LOCAL decision on a
non-empty effective key (while retaining the keyed finalize fix), and add a
direct-function plan case that rejects a zero-expression HASH exchange.
--
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]
