924060929 commented on code in PR #64633:
URL: https://github.com/apache/doris/pull/64633#discussion_r3480188257
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SplitLimit.java:
##########
@@ -38,8 +40,13 @@ public Rule build() {
.then(limit -> {
long l = limit.getLimit();
long o = limit.getOffset();
+ if (Utils.addOverflows(l, o)) {
+ throw new AnalysisException(
+ "limit + offset overflows the long range in
two-phase limit");
Review Comment:
We tested returning `null` (skip the rule) in an earlier iteration. The
problem is that the translator (`visitPhysicalLimit`) only handles `LOCAL` and
`GLOBAL` phases — it does **not** handle the unsplit `ORIGIN` phase:
```java
// PhysicalPlanTranslator.visitPhysicalLimit
if (physicalLimit.getPhase().isLocal()) {
// merge limit into child
} else if (physicalLimit.getPhase().isGlobal()) {
// create or merge into ExchangeNode
}
// ORIGIN: falls through — limit is silently dropped
```
So if SplitLimit returns `null` and the limit stays as `ORIGIN`, the
translator silently drops it — the query returns all rows instead of applying
`LIMIT/OFFSET`. That's a correctness bug worse than an error.
We also tried the single-phase approach (only emit `GLOBAL` without
`LOCAL`), but that hits the same `ChildrenPropertiesRegulator` constraint as
`twoPhaseSort`: the optimizer can't satisfy `GATHER` distribution without the
two-phase structure for distributed queries.
Since the two-phase `LOCAL` limit requires `limit + offset` (which
overflows), and MAX is not truly "unlimited" in the legacy `PlanNode` layer
(`hasLimit()` = `limit > -1`, no MAX→-1 conversion), throwing is the only
option that doesn't silently produce wrong results.
--
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]
