github-actions[bot] commented on code in PR #68186:
URL: https://github.com/apache/doris/pull/68186#discussion_r4045895789
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownScoreTopNIntoOlapScan.java:
##########
@@ -189,10 +190,9 @@ private Plan pushDown(
}
// When limit + offset overflows the long range, the pushed scan limit
would wrap to a
- // negative value. Fail with the same error as ordinary TopN instead
of leaving score()
- // unmaterialized and reporting an unrelated score() usage error.
- if (topN.getLimit() > Long.MAX_VALUE - topN.getOffset()) {
- throw new AnalysisException("limit + offset overflows the long
range");
+ // negative value; skip the push-down and let the TopN above the scan
apply limit/offset.
Review Comment:
[P2] Preserve the overflow error for score() queries
Returning null here leaves score() unmaterialized. The same rewrite batch
immediately runs CheckScoreUsage, so a query that has MATCH, ORDER BY, and
LIMIT now fails with the unrelated "score() function requires ..." message
before LogicalTopNToPhysicalTopN can report the overflow. This also contradicts
the existing regression at test_search_score_topn_predicates.groovy:191-201,
and the base version of this hunk explicitly threw the standard overflow error
to avoid exactly this path. Please keep the fail-fast AnalysisException here
(or validate overflow before CheckScoreUsage).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeTopNs.java:
##########
@@ -58,6 +60,10 @@ public Rule build() {
long limit = topN.getLimit();
long childOffset = childTopN.getOffset();
long childLimit = childTopN.getLimit();
Review Comment:
[P2] Keep individually valid nested TopNs executable
Compatible nested TopNs can hit this guard even when each node is valid. For
example, two TopNs with limit 1 and offset 9223372036854775806 each have limit
+ offset == Long.MAX_VALUE; the outer one must return zero rows, but their
combined offsets overflow and this code throws before lines 68-73 derive
newLimit = 0. The ordered derived-table shape is already known to reach
MERGE_TOP_N in test_merge_topn_offset.groovy. Please canonicalize the empty
result with a representable offset before this check, and add a boundary case
for it.
##########
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.
+ */
public static long mergeOffset(long upperOffset, long bottomOffset) {
Review Comment:
[P2] Preserve valid nested LIMITs when only the merged offset overflows
Two nested LIMIT 1 OFFSET 9223372036854775806 nodes are individually
representable because each limit + offset equals Long.MAX_VALUE, and the outer
node necessarily returns zero rows because its offset exceeds the child's
one-row output. This guard nevertheless turns that valid query into an analysis
error even though mergeLimit already derives a zero limit. Please canonicalize
this case to a representable empty limit; PhysicalPlanTranslator's Exchange
fold at lines 2263-2265 needs the same zero-limit handling so it does not call
mergeOffset after deriving zero.
--
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]
