github-actions[bot] commented on code in PR #66117:
URL: https://github.com/apache/doris/pull/66117#discussion_r3670714590
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -1292,6 +1338,21 @@ private boolean checkStats(Plan plan, PushDownAggContext
context) {
cards[groupByCardinality(colStats,
stats.getRowCount())].add(colStats);
}
+ if (!extremelyHigh.isEmpty() && context.getGroupKeys().size() > 1) {
Review Comment:
[P1] Remove the dead guard and cover reachable statistics gates
These branches introduce exact negative planner decisions, but this PR only
updates constructor calls in `EagerAggRewriterTest`. The 102 shape snapshots
show broad positive plan movement; none isolates the one-key-vs-multi-key 10x
boundary, the 1000x equality/rejection boundary, or the new session toggle. On
the reviewed head, `check_coverage_fe (Coverage)` fails at 65.45% (36/55):
these lines and the new `groupByCardinality` branch are entirely uncovered by
FE UT, and the successful early-return path is also uncovered. Lines 1347-1349
are unreachable because the same statistic is rejected at lines 1331-1333
before it is added to `groupKeysStats`; remove that duplicate guard rather than
trying to cover it. Then add controlled-statistics tests for both sides of the
reachable 10x/1000x boundaries, plus enabled/disabled nested-broadcast and
Aggregate/Intersect barrier+sibling cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -309,17 +345,25 @@ private Pair<Boolean, Boolean>
adjustPushSideForNullable(LogicalJoin<? extends P
return Pair.of(toLeft, toRight);
}
- private boolean isPassThroughBigJoin(LogicalJoin<? extends Plan, ? extends
Plan> join,
- PushDownAggContext context) {
- if (context.isPassThroughBigJoin()) {
- return true;
- } else {
- Statistics stats = join.right().getStats();
- if (stats == null) {
- stats = join.right().accept(derive, new
StatsDerive.DeriveContext());
- }
- return stats.getRowCount() > BIG_JOIN_BUILD_SIZE ||
SessionVariable.getEagerAggregationMode() > 0;
+ private boolean isSmallBroadcastJoin(LogicalJoin<? extends Plan, ? extends
Plan> join,
+ ConnectContext context) {
+ if (!JoinUtils.couldBroadcast(join)) {
+ return false;
}
+ SessionVariable sessionVariable = context.getSessionVariable();
+ Statistics stats = join.right().getStats();
+ if (stats == null) {
+ stats = join.right().accept(derive, new
StatsDerive.DeriveContext());
+ }
+ return stats.getRowCount() <=
sessionVariable.getBroadcastRowCountLimit()
Review Comment:
[P1] Match the physical broadcast-eligibility gate
`JoinUtils.couldBroadcast` only checks the join type, so this can mark a
join as “small broadcast” even when physical planning must shuffle it. For
example:
```text
Aggregate(sum(F.v), group by F.g, D.c1...D.c10)
Join(J, H > 1M rows)
J: Join(F, D = 200K rows with 11 required BIGINT outputs)
```
With `exec_mem_limit=64MiB`, the default 20% broadcast fraction allows
12.8MB, while `D` is about 17.6MB. `RequestPropertyDeriver` therefore rejects
broadcast via `checkBroadcastJoinStats`, but this method still sets the
small-broadcast marker. A 100M-row probe grouped to 1M keys is accepted by the
general check (roughly 100x reduction) and then rejected by the new 1000x gate,
so roughly 100x more rows are shuffled. The same mismatch exists for an
explicit `[shuffle]` hint on a shuffle-capable join. Please preserve forced
`BROADCAST_RIGHT` and broadcast/replication-only join types. When
`JoinUtils.couldShuffle(join)` is true, return false for `SHUFFLE_RIGHT`; for
an unhinted join in that same class, also require
`stats.computeSize(join.right().getOutput())` to fit the physical
broadcast-memory cap.
--
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]
