github-actions[bot] commented on code in PR #64633:
URL: https://github.com/apache/doris/pull/64633#discussion_r3449585806
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNThroughJoin.java:
##########
@@ -191,19 +192,19 @@ private Plan pushTopNToChild(LogicalTopN<? extends Plan>
topN, Plan child, List<
.filter(childOutputSet::contains)
.collect(Collectors.toList());
if (childRequired.isEmpty()) {
Review Comment:
The direct `TopN -> Join` pattern now uses `topnOptLimitThreshold >=
Utils.saturatedAdd(...)` before it pushes, but the sibling `TopN -> Project ->
Join` pattern has no equivalent guard and still reaches this helper. For a
reachable shape like `TopN(limit=MAX, offset=MAX, order by l.k) -> Project(l.k,
...) -> LeftOuterJoin`, the project branch calls `pushLimitThroughJoin` and
this changed line inserts a child `LogicalTopN(limit=Long.MAX_VALUE, offset=0)`
under the join. With the default `topn_opt_limit_threshold=1024`, the same plan
without the project would be rejected by the direct branch. Please apply the
same threshold/context predicate to the project-over-join branch, or centralize
the saturated-limit refusal inside `pushLimitThroughJoin` before constructing
child TopNs.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownTopNDistinctThroughJoin.java:
##########
@@ -107,7 +108,7 @@ private Plan pushTopNThroughJoin(LogicalTopN<? extends
Plan> topN, LogicalJoin<P
join.left().getOutputSet(), topN.getOrderKeys());
if (!pushedOrderKeys.isEmpty()) {
LogicalTopN<Plan> left = topN.withLimitOrderKeyAndChild(
Review Comment:
The direct distinct join pattern is gated by `topnOptLimitThreshold >=
Utils.saturatedAdd(...)`, but the `TopN -> Aggregate(distinct) ->
Project(allSlots) -> Join` pattern above it has no equivalent guard and still
reaches this changed child TopN construction. With `limit=MAX` and
`offset=MAX`, the direct branch is disabled under the default finite threshold,
while this project branch can push `LogicalTopN(limit=Long.MAX_VALUE,
offset=0)` plus `PlanUtils.distinct(child)` under the join. Please add the same
threshold/context predicate to the project branch, or check it once inside
`pushTopNThroughJoin` before constructing child TopNs.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/util/UtilsTest.java:
##########
@@ -32,4 +32,24 @@ public void containChinese() {
String en = "database123";
Assertions.assertFalse(Utils.containChinese(en));
}
+
+ @Test
+ public void testSaturatedAdd() {
Review Comment:
This unit test proves the arithmetic helper, but it does not prove that the
overflow fix is wired through the changed planner rules. The PR changes
`SplitLimit`, both TopN-through-join rules, distinct-union, and window
pushdown, while the only new SQL overflow case covers plain `UNION ALL` and is
already missing generated `.out` coverage in the existing regression thread.
Please add generated-output regressions or focused planner tests for the
overflow boundary through at least split limit and a session-gated
join/project-join path; otherwise the rule wiring and threshold behavior can
regress while this helper test still passes.
--
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]
