924060929 commented on code in PR #64633:
URL: https://github.com/apache/doris/pull/64633#discussion_r3480186488
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalTopNToPhysicalTopN.java:
##########
@@ -46,13 +48,30 @@ public Rule build() {
* mergeTopN(limit, off, require gather) -> localTopN(off+limit, 0,
require any)
*/
private List<PhysicalTopN<? extends Plan>> twoPhaseSort(LogicalTopN<?
extends Plan> logicalTopN) {
- PhysicalTopN<Plan> localSort = new
PhysicalTopN<>(logicalTopN.getOrderKeys(),
- logicalTopN.getLimit() + logicalTopN.getOffset(), 0,
SortPhase.LOCAL_SORT,
- logicalTopN.getLogicalProperties(), logicalTopN.child(0));
int sortPhaseNum = 0;
if (ConnectContext.get() != null) {
sortPhaseNum =
ConnectContext.get().getSessionVariable().sortPhaseNum;
}
+ // The two-phase local sort keeps limit + offset rows. When that
overflows the long range
+ // (e.g. LIMIT and OFFSET both BIGINT_MAX), only the single-phase
gather sort is safe: it
+ // applies limit and offset separately and cannot overflow. The
two-phase MERGE_SORT cannot be
+ // used because ChildrenPropertiesRegulator forbids inserting a gather
Exchange under
+ // GATHER_SORT, so the optimizer cannot produce a valid distributed
plan with only one-phase.
+ // If sort_phase_num != 1 (i.e. the user did not explicitly request
single-phase), report the
Review Comment:
Thanks for the review! We tried single-phase (GATHER_SORT only) in an
earlier iteration and it failed in CI with:
```
lowestCostPlans with physicalProperties(GATHER) doesn't exist in root group
```
Root cause: `ChildrenPropertiesRegulator.visitPhysicalTopN` (line ~740-760)
explicitly **forbids** inserting a gather Exchange under GATHER_SORT:
```java
if (topN.getSortPhase() == SortPhase.GATHER_SORT
&& children.get(0).getPlan() instanceof PhysicalDistribute) {
return ImmutableList.of(); // discard this plan
}
```
So GATHER_SORT can only work when the child already provides GATHER (e.g.
single-instance). For distributed queries (like `count(*)` over `UNION ALL`),
the two-phase MERGE_SORT is the only way to provide GATHER distribution. And
the two-phase local sort requires `limit + offset` which overflows.
We considered using `Long.MAX_VALUE` for the local sort limit ("keep all
rows locally"), but verified that MAX is **not** treated as "unlimited" —
legacy `PlanNode` uses `-1` for unlimited (`hasLimit()` returns `limit > -1`),
and there is no MAX → -1 conversion in the translator. So MAX would be sent to
BE as a real huge limit (triggering TopN heap allocation etc.).
Given these constraints, throwing with a clear message pointing to
`sort_phase_num = 1` seems like the honest choice for this extreme edge case
(LIMIT and OFFSET both at BIGINT_MAX).
--
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]
