xiangfu0 commented on code in PR #19121:
URL: https://github.com/apache/pinot/pull/19121#discussion_r4506371116
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestVisitor.java:
##########
@@ -337,6 +352,41 @@ public Void visitUnnest(UnnestNode node,
ServerPlanRequestContext context) {
return null;
}
+ /// An unbounded sort can be folded into the V1 request only when the leaf
runs exactly one physical request: a V1
+ /// ORDER BY then yields one sorted run per worker, whichever combine V1
picks. `sortedSelectionMergeMode` does not
+ /// affect that ordering; it is the opt-in, and when it streams (ON, or AUTO
resolving to ON over sorted segments) the
+ /// leaf no longer materializes every row. The sort itself must have no
offset, every sort key must be a plain column
+ /// (so the leaf's conversion to the MSE type cannot reorder V1's output),
and nothing below the sort may already have
+ /// set an ORDER BY, OFFSET or a narrower LIMIT, which the pushed sort would
overwrite.
+ private static boolean canPushUnboundedSort(SortNode node, PinotQuery
pinotQuery,
+ ServerPlanRequestContext context) {
+ if (!context.isSinglePhysicalRequest() ||
!context.isSortedSelectionMergeRequested() || node.getOffset() > 0
Review Comment:
**Measured slowdown: AUTO over unsorted segments.** When AUTO resolves to
OFF, V1 uses `MinMaxValueBasedSelectionOrderByCombineOperator`, which merges
each segment block into one growing list
(`SelectionOperatorUtils.mergeWithOrdering`). With `limit = Integer.MAX_VALUE`
it never stops early, so the cost grows with segments × rows. Locally (2M rows,
2 servers), head AUTO vs the parent commit: **2104 vs 1118 ms at 64 segments
per server, 7916 vs 1111 ms at 256**. ON stays around 0.9 s. Numbers are in the
review summary.
The enum docs say ON is for benchmarking only, so AUTO is the mode people
will actually run, and unsorted segments are exactly the case AUTO is meant to
detect. Could the push be limited to cases where V1 will really stream? For
example, apply AUTO's sorted-ratio check before deciding, or push only for ON
until AUTO is measured. A k-way merge in the V1 combine for unbounded limits
would also remove the quadratic merge.
---
_AI-generated review via IntentLab · Model: Claude Opus 5.5
(claude-opus-5-5)._
--
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]
