andygrove opened a new pull request, #6604: URL: https://github.com/apache/datafusion-comet/pull/6604
## Which issue does this PR close? Closes #6596. ## Rationale for this change The JVM columnar shuffle converts Spark `UnsafeRow`s to Arrow natively. #6596 compares that converter with the JVM `ArrowWriter` row path on identical rows. The native converter is two to three times faster on fixed-width types, decimals and structs, but three per-value costs took most of its string, map and array time. ## What changes are included in this PR? - `decode_utf8_spark_lossy` checks `is_ascii()` before calling `str::from_utf8`, which costs several times more on short strings. The ASCII check is `#[inline]` and the rest of the decoder stays out of line, because the shuffle calls it once per value from another crate. Every byte sequence decodes exactly as before. `CAST(binary AS string)` and the JVM-to-native import boundary use the same function, so they get the fast path too. - `append_map_elements` reads a map's key and value fields through a new `map_key_value_fields`. It used `get_map_key_value_fields`, which built a `MapFieldNames`, allocating and freeing six strings, for every map value. `make_builders` still gets the names. - A nullable element array with no nulls and at least eight elements is appended with one `append_slice`, as map keys already were. Shorter arrays stay on the per-element loop: with some null elements, arrays of up to five elements were slightly slower with the copy. The timestamp and date appenders were copies of the macro, so they now use it. Per-row cost of `append_columns` plus `builder_to_array`, in ns. Each number is the best of 2000 batches of 8192 rows, with one column and every tenth row null: | Column | Before | After | | ---------------------------------------------------- | ------------------ | --------------- | | string (plain builder) | 11.3 | 5.7 | | string (default `preferDictionary.ratio` of 10) | 20.4 | 17.2 | | map<string,string> | 143 | 49 | | array<string> | 35.8 | 24.8 | | array<int>, 8 / 16 / 50 elements, no nulls | 12.5 / 18.8 / 51.6 | 8.5 / 7.7 / 9.6 | | array<int>, 0-5 elements | 8.4-9.1 | 8.6-8.9 | | int, boolean, decimals, struct<int,long,double,date> | unchanged | unchanged | Spark's `UnsafeProjection` generated the rows from the column shapes in `CometArrowWriterBenchmark`. I measured on an M3 Max with a release build (`-Ctarget-cpu=native`), alternating the two builds, and the machine was shared with other jobs. Counting LZ4 encoding and the write, `map<string,string>` goes from 215 to 124 ns per row and `string` from 35.3 to 31.3. The dictionary trial for top-level strings stays as it is. #6597 tracks it. ## How are these changes tested? - `nullable_primitive_arrays_round_trip_on_every_append_path` appends arrays of every primitive element width, timestamps and dates included, through each path: empty, too short for the copy, holding a null, long enough with no nulls, and with the only null in the second bitset word. Each layout sits both on an 8-byte boundary and off it. The test fails if the null check reads only the first bitset word. - `decode_utf8_spark_lossy_ascii_is_borrowed_zero_copy` checks that ASCII input borrows, and that one byte past ASCII still decodes lossily. - The shuffle and common crates' tests pass (172 and 66), and so does `cargo clippy --all-targets --workspace -- -D warnings`. - On Spark 4.1, `CometShuffleSuite` and `DisableAQECometShuffleSuite` pass (92 tests, including `columnar shuffle tolerates non-UTF-8 bytes in a StringType column`), and so does `CometNativeCastSuite` (189 tests). -- 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]
