rohityadav1993 opened a new issue, #19725:
URL: https://github.com/apache/pinot/issues/19725
With null handling enabled, a selection query whose first `ORDER BY`
expression is a column and that has a `LIMIT`
can return wrong rows without any error, or fail with a
`NullPointerException`. Both come from
`MinMaxValueBasedSelectionOrderByCombineOperator`
(`COMBINE_SELECT_ORDERBY_MINMAX` in `EXPLAIN`), which skips segments
based on the column min/max value. That value cannot bound null rows: a null
is stored as the column's default null
value, and the metadata has no null semantics.
Found while testing for regressions in #19120.
### Cases
All examples use two segments on one server and `SET
enableNullHandling=true`.
**1. Wrong rows when nulls sort first (default for `DESC`, or explicit
`NULLS FIRST`).** Reachable since 1.0.0, when
`DESC` started defaulting to `NULLS FIRST` (#10805, #10817).
| Segment | `intCol` values |
|---|---|
| A | 100, 101, 102 |
| B | 5, null, null |
| Query | Expected | Actual |
|---|---|---|
| `SELECT intCol FROM t ORDER BY intCol DESC LIMIT 3` | null, null, 102 |
102, 101, 100 |
| `SELECT intCol FROM t ORDER BY intCol DESC NULLS FIRST LIMIT 3` | null,
null, 102 | 102, 101, 100 |
Segment A sets the boundary to 100. Segment B's max value is 5, so it is
skipped together with its null rows, which
should have come first. This also happens with column-based null handling on
a nullable column.
**2. Wrong rows with `ASC NULLS FIRST` and a custom default null value.**
The null rows are stored as the default
null value, which raises the segment's min value. Column `intCol` with
`defaultNullValue` 1000:
| Segment | `intCol` values |
|---|---|
| A | 1, 2, 3 |
| B | 500, null, null |
| Query | Expected | Actual |
|---|---|---|
| `SELECT intCol FROM t ORDER BY intCol ASC NULLS FIRST LIMIT 3` | null,
null, 1 | 1, 2, 3 |
With the usual default for an INT dimension (`Integer.MIN_VALUE`), the null
rows lower the min value instead, and the
result is correct.
**3. `NullPointerException` when a segment's kept rows end in a null.** This
also happens when nulls sort last.
Reachable since 0.11.0, when selection rows started carrying nulls (#8927).
| Segment | `intCol` values |
|---|---|
| A | 100, 90, 80 |
| B | 95, null, null |
`SELECT intCol FROM t ORDER BY intCol DESC NULLS LAST LIMIT 3` should return
`100, 95, 90` but fails with (error code
200):
```
Cannot invoke "java.lang.Comparable.compareTo(Object)" because
"segmentBoundaryValue" is null
```
Segment A sets the boundary to 80. Segment B's max value is 95, so it is
processed, and its last kept row (null)
becomes the boundary for the next `compareTo`.
**Not affected:** apart from the exception in case 3, `DESC NULLS LAST`,
`ASC` and `ASC NULLS LAST` return the
correct rows. Queries without null handling, and queries whose first `ORDER
BY` expression is not a plain column (they
use `SelectionOrderByCombineOperator`), are not affected.
### Why existing tests miss it
The null-handling query tests (`NullHandlingEnabledQueriesTest`,
`NullEnabledQueriesTest`, `AllNullQueriesTest`,
`BooleanNullEnabledQueriesTest`) register one segment twice, so both
segments have the same min/max value and nothing
is ever skipped. Most of their selection `ORDER BY` queries also have no
`LIMIT`.
### Unit tests that reproduce it
These tests are added in the fix PR #19724; on current master they fail as
shown.
`NullQueriesFluentTest` (two segments on one instance,
`maxExecutionThreads=1`):
| Test | Case | Failure on master |
|---|---|---|
| `testMinMaxCombineOrderByDescKeepsNullsFirst` | 1 (`DESC`, `DESC NULLS
FIRST`) | `expected [null] but found [102]` |
| `testMinMaxCombineOrderByDescKeepsNullsFirstColumnBasedNullHandling` | 1,
column-based null handling | `expected [null] but found [102]` |
| `testMinMaxCombineOrderByAscNullsFirstWithCustomDefaultNullValue` | 2 |
`expected [null] but found [1]` |
| `testMinMaxCombineNullBoundaryValue` | 3 | `Cannot invoke
"java.lang.Comparable.compareTo(Object)" because "segmentBoundaryValue" is
null` |
| `testMinMaxCombineNullsLast` | control: nulls last | passes |
For example, case 1:
```java
@Test
public void testMinMaxCombineOrderByDescKeepsNullsFirst() {
FluentQueryTest.withBaseDir(_baseDir)
.withExtraQueryOptions(Map.of("maxExecutionThreads", "1"))
.withNullHandling(true)
.givenTable(INT_SCHEMA, NULL_HANDLING_TABLE_CONFIG)
.onFirstInstance(new Object[]{100}, new Object[]{101}, new
Object[]{102})
.andSegment(new Object[]{5}, new Object[]{null}, new Object[]{null})
.whenQuery("select intCol from testTable order by intCol desc limit 3")
// The harness serves the instance as two servers, so the broker
merges two copies of [null, null, 102]
.thenResultIs(new Object[]{null}, new Object[]{null}, new
Object[]{null});
}
```
`SelectionCombineOperatorTest` runs the combine operator directly with one
thread:
| Test | Checks | Result on master |
|---|---|---|
| `selectionOrderByMinMaxProcessesSegmentWithNullsWhenNullsSortFirst` | case
1: rows `[null, null, 102]`, both segments processed | fails: `Lists differ at
element [0]: null != 102` |
| `selectionOrderByMinMaxSkipsSegmentWithoutNullsUnderNullHandling` | a
segment without nulls is still skipped under null handling | passes |
| `selectionOrderByMinMaxSkipsSegmentWithNullsWhenNullsSortLast` | a segment
with nulls is still skipped when nulls sort last | passes |
| `selectionOrderByMinMaxSkipsSegmentForNonNullableColumn` | a non-nullable
column is still skipped | passes |
The last three guard that a fix keeps segment skipping where it is safe
(`numSegmentsMatched == 1`).
Run:
```
./mvnw -pl pinot-core test
-Dtest='NullQueriesFluentTest,SelectionCombineOperatorTest'
```
### Workaround
Order by an expression instead of the plain column (for example `ORDER BY
intCol + 0 DESC`), which uses
`SelectionOrderByCombineOperator` and returns the correct rows.
--
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]
