rohityadav1993 opened a new pull request, #19724:
URL: https://github.com/apache/pinot/pull/19724
`MinMaxValueBasedSelectionOrderByCombineOperator` skips segments based on
the column min/max value. With null
handling enabled, that value cannot bound the null rows, because a null is
stored as the column's default null value
and the metadata has no null semantics. This causes two bugs in selection
queries whose first `ORDER BY` expression is
a column and that have a `LIMIT` (the queries that use this operator, shown
as `COMBINE_SELECT_ORDERBY_MINMAX` in
`EXPLAIN`).
### Bug 1: wrong rows when nulls sort first
When nulls sort first (the default for `DESC`, or explicit `NULLS FIRST`), a
segment whose null rows belong in the
top K can be skipped. The query returns wrong rows without any error.
Two segments on one server, `enableNullHandling=true`:
| Segment | `intCol` values |
|---|---|
| A | 100, 101, 102 |
| B | 5, null, null |
`SELECT intCol FROM t ORDER BY intCol DESC LIMIT 3`
- Expected: `null, null, 102`
- Actual: `102, 101, 100`. Segment A sets the boundary to 100; segment B's
max value is 5, so it is skipped with its
nulls.
The same happens with `ASC NULLS FIRST` when the column has a custom
`defaultNullValue` above the other segments'
values, because the null rows then raise the segment's min value.
Reachable since 1.0.0, when `DESC` started defaulting to `NULLS FIRST`
(#10805, #10817).
### Bug 2: `NullPointerException` when a segment's last kept row is null
If a segment's kept rows end in a null, that null becomes the boundary value
and the next `compareTo` throws. This
also happens when nulls sort last.
| Segment | `intCol` values |
|---|---|
| A | 100, 90, 80 |
| B | 95, null, null |
`SELECT intCol FROM t ORDER BY intCol DESC NULLS LAST LIMIT 3` fails with:
```
Cannot invoke "java.lang.Comparable.compareTo(Object)" because
"segmentBoundaryValue" is null
```
Reachable since 0.11.0, when selection rows started carrying nulls (#8927).
### Fix
All changes are in `MinMaxValueBasedSelectionOrderByCombineOperator`;
operator selection in `CombinePlanNode` is
unchanged.
- **Segments that may have nulls are never skipped when nulls sort first.**
Such a segment is treated like a segment
without a min/max value, which the operator already sorts to the front and
never skips. Keeping these segments at
the front matters because a skip also drops every segment after it.
- **The per-segment signal is the column's null value vector**
(`DataSource.getNullValueVector()`):
- Immutable segments only have one when the column contains nulls.
- Mutable (consuming) segments always have one when null handling is
enabled, so they are conservatively never
skipped when nulls sort first.
- Non-nullable columns (column-based null handling) and tables without
null value vectors have none, so they keep
skipping.
- This is the same reader that produces the null cells in the results
(`ProjectionBlockValSet.getNullBitmap()`), so
"no null value vector" means the segment cannot return null rows.
Checking it is a reference check on a reader
built at segment load; it does no I/O and needs no `acquire()`.
- **Segment skipping is unchanged when nulls sort last.** A null row ranks
below any non-null boundary, and the stored
default null value can only widen the min/max range, which is conservative.
- **A null row value is never used as the boundary value**, per segment or
for the merged result; the previous
boundary is kept. This fixes bug 2.
#### Why not disable the operator under null handling
#18692 turned off `SelectionQuerySegmentPruner` whenever null handling is
active. Doing the same here would give up
segment skipping for every null-handling query, including nulls-last orders,
segments without nulls and non-nullable
columns. The per-segment check keeps skipping in all of those cases and only
processes segments that can actually
return null rows ahead of the boundary.
I also considered `ColumnMetadata.isNonNull()`. It is `false` for segments
created before the flag existed and for
columns that were never null-handled, so it would turn off skipping for
segments that cannot return nulls. It is also
not available for mutable segments.
### Performance
- No change without null handling, or when nulls sort last.
- When nulls sort first, segments that contain nulls, and consuming
segments, are always processed. In the worst case
(every segment has nulls) the operator does about the same work as
`SelectionOrderByCombineOperator`.
### Tests
- `NullQueriesFluentTest`:
- `DESC` and `DESC NULLS FIRST`;
- column-based null handling on a nullable column;
- `ASC NULLS FIRST` with a custom `defaultNullValue`;
- the `NullPointerException` case;
- nulls-last controls.
- `SelectionCombineOperatorTest` checks both the rows and
`numSegmentsMatched`:
- a segment with nulls is processed when nulls sort first;
- skipping is kept when nulls sort last, for segments without nulls, and
for non-nullable columns.
These fail if the fix is replaced by turning skipping off.
The new tests fail on master (4 in `NullQueriesFluentTest`, 1 in
`SelectionCombineOperatorTest`) and pass with the
fix. Removing each part of the fix makes the expected tests fail, except the
null guard on the merged boundary. That
guard only preserves skipping across threads, so its effect depends on
thread timing.
Also run: `NullHandlingEnabledQueriesTest`, `NullEnabledQueriesTest`,
`AllNullQueriesTest`,
`BooleanNullEnabledQueriesTest`, `SelectionQuerySegmentPrunerTest`,
`CombineSlowOperatorsTest`,
`TextMatchTransformFunctionTest`, `CombinePlanNodeTest`, and the selection
`ORDER BY` suites (505 tests, all pass).
### Compatibility
Server-local change to query execution. No wire format, config or API change.
--
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]
