andygrove opened a new pull request, #6824:
URL: https://github.com/apache/datafusion-comet/pull/6824
## Which issue does this PR close?
Closes #4626.
## Rationale for this change
When Comet converts a Spark `UnsafeArray` of primitives to Arrow, as JVM
columnar shuffle does for every array column, it appends a nullable array
element by element, checking the null bitset for each one. #6604 appends a
nullable array without nulls with one `append_slice`, but an array that holds a
null still takes the loop.
This continues #4672 by @sandugood, which replaces that loop with one copy
of the values and a validity buffer built from Spark's null bitset. #4672 has
had no activity since its last review, so this branch keeps its commits, merges
main, and addresses the review feedback. GitHub credits @sandugood as co-author
when the PR is squash merged.
## What changes are included in this PR?
- An aligned nullable array of at least 64 elements that holds a null is
appended with `PrimitiveBuilder::append_array`: one copy of the values plus a
`NullBuffer` made by inverting the bytes of Spark's null bitset. Spark sets the
bit of a null element and Arrow the bit of a valid one. This covers every type
`impl_append_to_builder` generates: the integer and floating-point types,
`Date32`, `Time64` and `Timestamp`.
- Shorter arrays and unaligned ones stay on the per-element loop. Building
the two buffers takes four allocations, which cost as much as appending 40 to
55 elements one by one. `MIN_BULK_NULLABLE_APPEND_ELEMENTS` records the
measurements. This addresses sunchao's review of #4672, which found one-element
arrays 11 times slower on the bulk path.
- `append_array` requires the array's data type to match the builder's, and
Comet's timestamp builders carry the column's timezone. So the timestamp
appender takes the timezone, and the list path in `row.rs` passes it through.
- A nullable array without nulls keeps the `append_slice` path from #6604.
The boolean appender reads its bytes through a slice.
## How are these changes tested?
- `nullable_primitive_arrays_round_trip_on_every_append_path` also covers
arrays of 63 and 64 elements with nulls at both ends, on either side of the new
cutoff, and a timestamp column with a timezone. It runs every element width,
with arrays on and off an 8-byte boundary. Dropping the timezone from the bulk
path makes `append_array` panic.
- The tests from #4672 cover arrays whose last null sits in a partial byte,
all-null arrays and all-valid arrays, now at lengths on both sides of the
cutoff. Skipping the bit inversion fails four tests.
- `nullable_arrays_append_into_one_builder` appends several arrays into one
builder, as the list path does, so a validity buffer is appended at an offset
that is not a whole byte.
- The shuffle crate's tests pass (176), as do `cargo clippy --all-targets
--workspace -- -D warnings` and `cargo +nightly miri test -p
datafusion-comet-shuffle --lib 'spark_unsafe::'`.
The table shows the per-row cost in ns of appending a list column's arrays,
with one null per array. I measured release-build copies of the append method
on an M3 Max with the system allocator, driven the way
`append_list_column_batch` drives it: a `ListBuilder` whose values builder
starts at capacity 100, and 8192 rows cycling through 64 distinct
8-byte-aligned arrays. Each number is the median of 41 interleaved batches for
`i32` and 31 for `i64`. The machine was shared with other jobs.
| Elements per array | main, `i32` | this PR, `i32` | main, `i64` | this PR,
`i64` |
| ------------------ | ----------- | -------------- | ----------- |
-------------- |
| 1 | 6.7 | 6.4 | 6.9 | 6.6
|
| 16 | 36.0 | 36.2 | 36.3 | 36.4
|
| 32 | 71.0 | 70.6 | 74.3 | 74.9
|
| 64 | 148 | 95 | 198 | 170
|
| 256 | 600 | 147 | 799 | 320
|
| 1024 | 2402 | 258 | 3223 | 1149
|
Arrays without nulls cost the same as on main. Taking the bulk path for
every aligned nullable array, as #4672 did, a one-element array cost 85 ns and
a 16-element array 87 ns.
--
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]
