gortiz commented on code in PR #19724:
URL: https://github.com/apache/pinot/pull/19724#discussion_r4193036483
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/MinMaxValueBasedSelectionOrderByCombineOperator.java:
##########
@@ -81,10 +90,12 @@ public
MinMaxValueBasedSelectionOrderByCombineOperator(List<Operator> operators,
OrderByExpressionContext firstOrderByExpression =
orderByExpressions.get(0);
assert firstOrderByExpression.getExpression().getType() ==
ExpressionContext.Type.IDENTIFIER;
String firstOrderByColumn =
firstOrderByExpression.getExpression().getIdentifier();
+ boolean nullsFirst = queryContext.isNullHandlingEnabled() &&
!firstOrderByExpression.isNullsLast();
Review Comment:
Nit: this flag means "null handling is enabled and nulls rank first", not
only "nulls first". The `isNullHandlingEnabled()` check is load-bearing,
because `isNullsLast()` returns false for `DESC` even when null handling is
off. A name like `nullsMayRankFirst`, or a short comment, would make that clear
to the next reader.
##########
pinot-core/src/test/java/org/apache/pinot/queries/NullQueriesFluentTest.java:
##########
@@ -109,4 +143,102 @@ public void
testCastStringToTimestampNullHandlingDisabled() {
new Object[]{"2025-09-23 17:38:00.0"}
);
}
+
+ /// The segment with nulls has max 5, below the boundary 100 set by the
first segment, but its nulls sort first.
+ @Test
+ public void testMinMaxCombineOrderByDescKeepsNullsFirst() {
Review Comment:
Optional: because the broker merges two copies of the combine result, these
assertions end up as `[null, null, null]`. They still fail on master, so they
catch the bug. But they can't tell whether the per-server third row was 102 or
something else. With `LIMIT 5` the non-null tail would survive the merge and
the assertion would be stricter. `SelectionCombineOperatorTest` already checks
the exact rows, so this is not a blocker.
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/combine/SelectionCombineOperatorTest.java:
##########
@@ -304,10 +347,78 @@ public void
selectionOrderByDescendingWithLargeLimitAndReverseOrder() {
assertEquals(combineResult.getNumTotalDocs(), NUM_SEGMENTS *
NUM_RECORDS_PER_SEGMENT);
}
+ /// Under null handling, a segment without nulls is still skipped when its
max cannot beat the boundary, even though
+ /// nulls sort first under `DESC`.
+ @Test
+ public void
selectionOrderByMinMaxSkipsSegmentWithoutNullsUnderNullHandling() {
+ SelectionResultsBlock combineResult =
+ getSingleThreadCombineResult(NULL_HANDLING_OPTIONS + "SELECT * FROM
testTable ORDER BY intColumn DESC LIMIT 3",
+ List.of(_highSegment, _lowSegment));
+ assertEquals(getIntColumnValues(combineResult), Arrays.asList(102, 101,
100));
+ assertEquals(combineResult.getNumSegmentsProcessed(), 2);
+ assertEquals(combineResult.getNumSegmentsMatched(), 1);
+ }
+
+ /// When nulls sort last, a segment with nulls is still skipped when its max
cannot beat the boundary: its nulls rank
+ /// below any non-null boundary.
+ @Test
+ public void selectionOrderByMinMaxSkipsSegmentWithNullsWhenNullsSortLast() {
+ SelectionResultsBlock combineResult = getSingleThreadCombineResult(
+ NULL_HANDLING_OPTIONS + "SELECT * FROM testTable ORDER BY intColumn
DESC NULLS LAST LIMIT 3",
+ List.of(_highSegment, _lowSegmentWithNulls));
+ assertEquals(getIntColumnValues(combineResult), Arrays.asList(102, 101,
100));
+ assertEquals(combineResult.getNumSegmentsProcessed(), 2);
+ assertEquals(combineResult.getNumSegmentsMatched(), 1);
+ }
+
+ /// When nulls sort first, a segment with nulls must not be skipped: the
segment min/max does not account for its
+ /// null rows.
+ @Test
+ public void
selectionOrderByMinMaxProcessesSegmentWithNullsWhenNullsSortFirst() {
Review Comment:
Optional: nothing tests the mutable (consuming) segment path.
`MutableSegmentImpl` always creates a `MutableNullValueVector` for nullable
columns, so consuming segments are never skipped when nulls sort first. Right
now only the comments describe that behavior. A small case with a mutable
segment would lock it in.
--
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]
