xiangfu0 commented on code in PR #19922:
URL: https://github.com/apache/pinot/pull/19922#discussion_r4309004411
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/query/SelectionOrderByOperatorTest.java:
##########
@@ -129,6 +130,20 @@ public void testPartialSortNullWithoutNullHandling() {
assertNotNull(rows.get(0)[1], "Column 'col2' value should not be 'null'
when null handling is disabled");
}
+ /// `LIMIT` plus `OFFSET` can exceed the int range. Wrapping to a negative
number of rows to keep reaches
+ /// `new PriorityQueue<>(negative)`, which fails the query instead of
answering it.
+ @Test
+ public void testLargeLimitPlusOffsetDoesNotOverflow() {
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(
+ "SELECT col1, col2 "
+ + "FROM testTable "
+ + "ORDER BY col1, col2 "
Review Comment:
This constructs `SelectionOrderByOperator` directly, so it misses the
planning path: `SelectionPlanNode.run()` line 88 still computes the raw `limit
+ offset`, and on a segment whose order-by column is physically sorted this
exact query still fails (see review summary). Suggest adding a plan-level
regression test — build a segment with a sorted column and plan through
`SelectionPlanNode` (or run the query end-to-end) with `LIMIT 2147483647 OFFSET
2147483647`, asserting rows are returned. That test fails on the current head
and would have caught the missed site.
Also, the saturation contract of `getLimitPlusOffset()` is only pinned as
side-assertions inside two operator tests (here and
`SelectionOperatorServiceTest`); a direct `QueryContext` unit test would be
more durable.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/selection/SelectionOperatorService.java:
##########
@@ -71,7 +71,7 @@ public SelectionOperatorService(QueryContext queryContext,
DataSchema dataSchema
// Select rows from offset to offset + limit.
Review Comment:
Stale comment: this now annotates a saturated sum, not the exact `offset +
limit`. A reader trusting it may reintroduce the raw add when refactoring
`processSingleDataTable`'s end bound. Suggest dropping it or referencing the
saturation semantics of `getLimitPlusOffset()`.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SortOperator.java:
##########
@@ -109,7 +110,8 @@ static SortOperator create(OpChainExecutionContext context,
MultiStageOperator i
// Setting numRowsToKeep as default maximum on Broker if limit not set.
// TODO: make this default behavior configurable.
int fetch = node.getFetch();
- int numRowsToKeep = fetch > 0 ? fetch + offset : defaultResponseLimit;
+ // Saturate the sum so that a large fetch and offset cannot wrap to a
negative number of rows to keep.
+ int numRowsToKeep = fetch > 0 ? IntMath.saturatedAdd(fetch, offset) :
defaultResponseLimit;
Review Comment:
Two notes while touching this line:
1. `fetch > 0` conflates `LIMIT 0` with "no limit set", while
`ServerPlanRequestVisitor` uses `fetch >= 0` for the same value one layer down.
If a fetch=0 SortNode survives planning, `numRowsToKeep` becomes
`defaultResponseLimit` for a query that asked for zero rows. Worth normalizing
to `fetch >= 0` (pre-existing, non-blocking).
2. `LimitSortOperator` reconstructs the limit as `numRowsToKeep - offset`,
which under-counts once the sum saturates (e.g. fetch=10, offset=2147483640 →
`_rowsToEmit=7`). Near-theoretical, but since `create` knows the real fetch it
could pass it through instead of re-deriving it.
Also note the two planner rules that produce this same sum unsaturated
(`PinotSortExchangeCopyRule`, `SortPushdownRule`) — see the review summary.
--
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]
