andygrove opened a new pull request, #6895: URL: https://github.com/apache/datafusion-comet/pull/6895
## Which issue does this PR close? Closes #6596 on `branch-1.1`. #6604 already closed it on `main`. ## Rationale for this change This backports #6604 to `branch-1.1`. It is a performance change, not a bug fix, so as the backporting guide asks, here is why it is worth the risk on a branch that has shipped a release. When Comet can't use native shuffle, for example because the child is a Spark operator that produces rows, the default `auto` shuffle mode falls back to JVM columnar shuffle, which converts each `UnsafeRow` to Arrow in native code. #6596 found that three per-value costs took most of that conversion's time for strings, maps and arrays. #6604 measured these per-row costs before and after: | Column | Before | After | | --- | --- | --- | | `map<string,string>` | 150 ns | 47.3 ns | | `array<string>` | 35.5 ns | 24.3 ns | | string, default `preferDictionary.ratio` | 21.5 ns | 16.9 ns | | `array<int>`, 8 / 16 / 50 elements, no nulls | 12.5 / 18.8 / 51.6 ns | 8.5 / 7.7 / 9.6 ns | Fixed-width types, decimals and structs are unchanged. The risk is small: - The change is confined to per-value conversion code and produces the same output as before. It adds no config and changes no plan, protobuf or shuffle format. - The cherry-pick's diff is byte-identical to #6604's, blob hashes included, so the four files on this branch were exactly the ones #6604 changed on `main`. - #6604 has been on `main` since 2026-10-05, and nothing there has had to fix it since. The two later commits that touch the same files, #6824 and #6861, are further optimizations and are not part of this backport. Not proposed for `branch-1.0`, which takes only selected bug fixes. ## What changes are included in this PR? A cherry-pick (`-x`) of #6604's squash commit, with no adaptations. See #6604 for the details. In short: - `decode_utf8_spark_lossy` checks a value of up to 32 bytes with an inline `is_ascii()` before calling `str::from_utf8`, and checks a longer value 32 bytes at a time, validating only from the first block that isn't ASCII. Besides JVM columnar shuffle, the default `CAST(binary AS string)` decodes every value through this function, and a string import falls back to it when a buffer fails UTF-8 validation. - `append_map_elements` reads a map's key and value fields through `map_key_value_fields`, instead of building a `MapFieldNames`, which allocated and freed six strings, for every map value. - A nullable primitive array with no nulls and at least eight elements is appended with one `append_slice`. The timestamp and date appenders, which were copies of the macro, now use it. ## How are these changes tested? Run locally on this branch with JDK 17: - `cargo test -p datafusion-comet-shuffle -p datafusion-comet-common` passes (172 and 67 tests), including #6604's `nullable_primitive_arrays_round_trip_on_every_append_path` and `decode_utf8_spark_lossy_ascii_is_borrowed_zero_copy`. The 105 `conversion_funcs` tests in `datafusion-comet-spark-expr` pass too, including the binary-to-string casts that call the decoder. - On the default Spark 4.1 profile, `CometShuffleSuite` (46 tests) and `DisableAQECometShuffleSuite` (45) pass. They run JVM columnar shuffle over strings, maps, arrays, structs and the other supported types. So do the three binary-to-string tests in `CometNativeCastSuite`. - `cargo fmt --check` and workspace clippy with `--all-targets -D warnings` are clean, both with my local Rust 1.97 and with a 1.99 nightly, since CI uses stable. #6604's tests check that each new append path gives the same output as the old one, so they pass without the change as well. There is no bug to show failing here. I didn't rerun the benchmarks, because the conversion code on this branch was identical to `main`'s before #6604. By CI's path routing, which I checked with `dev/ci/compute-changes.py`, this PR runs the all-profiles Linux build, macOS, the PyArrow UDF tests, the Spark SQL jobs for 3.5, 4.0 and 4.1, and the four Iceberg versions, so no `run-*` label is needed. -- 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]
