Optimize Spark hex null handling - #23688
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23688 +/- ##
==========================================
- Coverage 80.75% 80.75% -0.01%
==========================================
Files 1096 1096
Lines 373282 373316 +34
Branches 373282 373316 +34
==========================================
+ Hits 301440 301455 +15
- Misses 53869 53872 +3
- Partials 17973 17989 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
may be related to #23766 |
kosiew
left a comment
There was a problem hiding this comment.
Looks good overall. The byte-array optimization stays nicely scoped, and the targeted Spark hex tests pass. I left one optional suggestion to add Binary or LargeBinary dictionary coverage where the dictionary values array itself contains a null.
|
@floze-the-genius |
Reuse the input null buffer and split nullable and no-null encoding paths to avoid rebuilding validity and per-value Option checks. Add pointer-reuse and no-null regressions plus dedicated Criterion cases.
4e9f34a to
af3fafa
Compare
|
Rebased onto current main and resolved the conflict while keeping the shared |
## Which issue does this PR close? - Closes apache#23674. ## Rationale for this change Spark hex encoding preserves input validity exactly, but the byte-array path rebuilt the null bitmap row by row and always iterated through `Option` values. The input arrays already expose both the original `NullBuffer` and an all-valid fast path. ## What changes are included in this PR? - pass the byte array accessor into `hex_encode_bytes` so the output can clone and reuse the input `NullBuffer` - split nullable and no-null loops, avoiding per-row validity checks when the input has no null buffer - preserve exact offset, overflow, casing, dictionary, and null semantics - add a sliced-array regression that verifies semantic equality and bitmap pointer reuse - add no-null output coverage and dedicated UTF-8/binary Criterion cases ## Are these changes tested? Yes. - `cargo test -p datafusion-spark function::math::hex --lib`: 10 passed - `cargo fmt --all -- --check`: passed - `cargo clippy -p datafusion-spark --all-targets --all-features --no-deps -- -D warnings`: passed - `git diff --check`: passed - benchmark target compiled with `--features core` The workspace dependency clippy invocation without `--no-deps` reaches an unrelated existing `dead_code` warning in `datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:464`; the changed crate is clean. ### Benchmark Command (same warmed build and profile for upstream `main` and this branch): ```bash CARGO_BUILD_JOBS=2 \ CARGO_PROFILE_BENCH_LTO=false \ CARGO_PROFILE_BENCH_CODEGEN_UNITS=16 \ cargo bench -p datafusion-spark --features core --bench hex -- \ 'hex_(utf8|binary)_no_nulls' ``` Mean times from the immediate upstream-then-branch sequence: | case | upstream main | this PR | change | | --- | ---: | ---: | ---: | | UTF-8, 1,024 rows | 79.895 us | 59.181 us | -25.9% | | UTF-8, 4,096 rows | 317.64 us | 244.88 us | -22.9% | | UTF-8, 8,192 rows | 509.64 us | 411.27 us | -19.3% | | Binary, 1,024 rows | 86.135 us | 40.600 us | -52.9% | | Binary, 4,096 rows | 346.16 us | 253.13 us | -26.9% | | Binary, 8,192 rows | 635.98 us | 424.78 us | -33.2% | ## Are there any user-facing changes? No API or behavior changes. This is an internal performance optimization for Spark-compatible hex expressions.
Which issue does this PR close?
Rationale for this change
Spark hex encoding preserves input validity exactly, but the byte-array path rebuilt the null bitmap row by row and always iterated through
Optionvalues. The input arrays already expose both the originalNullBufferand an all-valid fast path.What changes are included in this PR?
hex_encode_bytesso the output can clone and reuse the inputNullBufferAre these changes tested?
Yes.
cargo test -p datafusion-spark function::math::hex --lib: 10 passedcargo fmt --all -- --check: passedcargo clippy -p datafusion-spark --all-targets --all-features --no-deps -- -D warnings: passedgit diff --check: passed--features coreThe workspace dependency clippy invocation without
--no-depsreaches an unrelated existingdead_codewarning indatafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:464; the changed crate is clean.Benchmark
Command (same warmed build and profile for upstream
mainand this branch):CARGO_BUILD_JOBS=2 \ CARGO_PROFILE_BENCH_LTO=false \ CARGO_PROFILE_BENCH_CODEGEN_UNITS=16 \ cargo bench -p datafusion-spark --features core --bench hex -- \ 'hex_(utf8|binary)_no_nulls'Mean times from the immediate upstream-then-branch sequence:
Are there any user-facing changes?
No API or behavior changes. This is an internal performance optimization for Spark-compatible hex expressions.