zhuqi-lucas opened a new pull request, #26215:
URL: https://github.com/apache/datafusion/pull/26215
## Which issue does this PR close?
- Part of #23840. Second of the three units that umbrella PR was split into;
#23990 (the `RowValues` cache) was the first.
## Rationale for this change
`ArrayValues` wraps every sort-key cursor to add null handling, so its
`compare` / `eq` / `eq_to_previous` sit in the merge's inner loop. Each one
opens with the same shape:
```rust
match (l.is_null(l_idx), r.is_null(r_idx)) {
(true, true) => Ordering::Equal,
(true, false) => ...,
(false, true) => ...,
(false, false) => T::compare(...),
}
```
That is two comparisons against `null_threshold` plus the match, evaluated
once per comparison the loser tree makes.
Sort keys are usually null-free, and whether an array has any nulls is known
when the cursor is built (`array.null_count()`, already read there to compute
`null_threshold`). When neither side has nulls, the match above picks its
`(false, false)` arm every single time.
## What changes are included in this PR?
- `ArrayValues` records `has_nulls: bool` at construction, from the
`null_count` it already reads.
- `compare`, `eq` and `eq_to_previous` take a direct path when no cursor
involved has nulls, skipping the `is_null` checks.
`has_nulls` is purely a shortcut around checks whose outcome is already
determined; `is_null` still decides every case where a null can occur, so
behaviour is unchanged.
Not touched: `get_value` and `eq_to_single_row_value` also call `is_null`,
but they are not in the merge's inner loop, so they keep the plain path rather
than growing another branch.
## Are these changes tested?
Yes. `test_array_values_has_nulls_fast_path_agrees_with_slow_path` asserts
the fast path is actually taken (`!values.has_nulls` on both cursors) and that
it orders the same as the null-aware path, then forces the slow path with a
null-containing cursor and checks the two agree.
Red/green verified: making the fast path ignore `options.descending` fails
the test.
`cargo test -p datafusion-physical-plan --lib sorts::` — 145 passed. `cargo
clippy --all-targets -p datafusion-physical-plan -- -D warnings` clean.
## Benchmark
Measured as part of the full #23840 stack on sort-tpch SF1
(`release-nonlto`, 3 iterations), where this unit is what recovered Q11:
StringView cursor caching alone regressed Q11 by ~3%, and adding this fast path
moved it to -7.3%. Q11 sorts 6M rows by `l_shipmode` — cardinality 7, so it is
almost entirely tied comparisons, which is exactly the inner loop this touches.
Requesting the benchmark bot below for numbers on this change in isolation.
## Are there any user-facing changes?
No. No API change; `ArrayValues` is internal to the sort-merge cursors.
--
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]
