github-actions[bot] commented on code in PR #64633:
URL: https://github.com/apache/doris/pull/64633#discussion_r3465800781
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeLimits.java:
##########
@@ -54,7 +56,16 @@ public Rule build() {
}).toRule(RuleType.MERGE_LIMITS);
}
+ /**
+ * Merge two consecutive limits' offsets into a single offset. Consecutive
limits must be merged,
+ * and an overflowing combined offset cannot be represented as a single
offset, so fail fast
+ * instead of wrapping to a negative offset.
+ */
Review Comment:
This throws for a valid nested `LIMIT/OFFSET` shape instead of preserving
the empty result. For example, `Limit(limit=1, offset=1) ->
Limit(limit=Long.MAX_VALUE, offset=Long.MAX_VALUE) -> Scan` is reachable from
nested subqueries; the inner offset already skips every possible row because
Doris assumes no relation has more than `Long.MAX_VALUE` rows. Merging those
offsets overflows here and turns the query into an analysis error. Please keep
the original empty-result semantics, e.g. rewrite this overflow case to `LIMIT
0`/empty relation or skip the merge, and add a focused nested-limit test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalTopN.java:
##########
@@ -61,6 +61,11 @@ public LogicalTopN(List<OrderKey> orderKeys, long limit,
long offset, Optional<G
Optional<LogicalProperties> logicalProperties, CHILD_TYPE child) {
super(PlanType.LOGICAL_TOP_N, groupExpression, logicalProperties,
child);
this.orderKeys =
ImmutableList.copyOf(Objects.requireNonNull(orderKeys, "orderKeys can not be
null"));
+ // limit/offset are always non-negative ("no limit" is represented by
Long.MAX_VALUE). A
+ // negative value here means limit + offset overflowed somewhere
upstream and would produce an
+ // illegal plan that hangs in BE; fail fast instead.
+ Preconditions.checkArgument(limit >= 0 && offset >= 0,
Review Comment:
This new invariant is useful, but there is still a reachable raw offset
merge that now trips it. `MergeTopNs` computes `offset + childOffset` without
an overflow check before calling `withLimitChild`; for a nested TopN shape such
as `TopN(limit=1, offset=1) -> TopN(limit=Long.MAX_VALUE,
offset=Long.MAX_VALUE) -> Scan`, that sum wraps negative and this constructor
rejects a query that should simply return zero rows. Please guard the
`MergeTopNs` offset merge as well, preserving empty-result semantics, and add a
focused nested-TopN test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownLimitDistinctThroughJoin.java:
##########
@@ -64,6 +66,12 @@ public List<Rule> buildRules() {
logicalLimit(logicalAggregate(logicalProject(logicalJoin()).when(LogicalProject::isAllSlots))
.when(LogicalAggregate::isDistinct))
.then(limit -> {
+ // limit + offset overflowing means no child can
hold that many rows, so the
+ // push-down cannot reduce anything; skip it. (The
direct branch is gated the
Review Comment:
This project branch still skips the same `ConnectContext` and
`topn_opt_limit_threshold` gate that protects the direct `LIMIT -> DISTINCT ->
JOIN` branch above. A plan like `Limit(limit=2000, offset=0) ->
Aggregate(distinct) -> Project(all slots) -> LeftOuterJoin` is refused by the
direct branch with the default threshold of 1024, but adding the all-slot
project reaches this path and pushes the large child limits anyway. Please
apply the same session/threshold predicate here, or centralize the guard in
`pushLimitThroughJoin` before constructing child limits.
--
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]
