Optimize Spark hex null handling#23688
Conversation
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23688 +/- ##
==========================================
- Coverage 80.67% 80.67% -0.01%
==========================================
Files 1087 1087
Lines 367541 367560 +19
Branches 367541 367560 +19
==========================================
- Hits 296529 296524 -5
- Misses 53355 53367 +12
- Partials 17657 17669 +12 ☔ 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.
| hex_encode_bytes(arr.iter(), lowercase, arr.len())? | ||
| hex_encode_bytes(&arr, lowercase)? | ||
| } | ||
| DataType::Binary => { |
There was a problem hiding this comment.
Could we add a small regression test for a dictionary-backed Binary or LargeBinary array where dict.values() itself contains a null, rather than only testing null dictionary keys? The new byte-array path preserves the values array NullBuffer directly. We already cover this shape for Int64 in test_dict_values_null, so matching binary coverage would help pin down this updated path. This is optional and does not need to block the PR.
|
@floze-the-genius |
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.