refactor: share hex encoding across datafusion-common, functions, and spark#23766
Open
andygrove wants to merge 8 commits into
Open
refactor: share hex encoding across datafusion-common, functions, and spark#23766andygrove wants to merge 8 commits into
andygrove wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23766 +/- ##
=======================================
Coverage 80.71% 80.71%
=======================================
Files 1089 1090 +1
Lines 368748 368814 +66
Branches 368748 368814 +66
=======================================
+ Hits 297633 297691 +58
- Misses 53372 53378 +6
- Partials 17743 17745 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
andygrove
changed the title
Migrate the Spark-compatible hex function onto the shared datafusion_common::utils::hex module (encode_bytes_into, encode_u64) instead of its own lookup tables, keeping the post-apache#23473 buffer structure (manual value/offset buffers, try_reserve guard, StringArray::new_unchecked) unchanged.
Replace the per-file hex-encoding lookup tables in Spark's sha1 and sha2 functions with the shared datafusion_common::utils::hex module, continuing the consolidation started for to_hex, hex, and md5.
Add encode_bytes_to_slice to the shared hex module for callers writing into a pre-sized buffer, and migrate the encode() scalar function's last two hex call sites (inner.rs) off the hex crate onto it. This closes the API gap that had left hex_encode_array as the final divergent hex encoder in the workspace.
Reword the write_digits doc comment to describe the actual borrow conflict (the v == 0 early return, not a return inside the loop), drop the unreachable & 0xF mask in build_lookup, add doctests to the public encode_bytes/encode_u64/encode_bytes_to_slice functions plus a less internal module doc comment, replace the tautological encode_bytes/encode_bytes_into agreement test with one that pins buffer-reuse behavior in encode_u64, and cover the untested lowercase path of hex_encode_bytes in the Spark hex function.
andygrove
force-pushed
the
auto-opt/to_hex-datafusion-20260721-094849
branch
from
July 21, 2026 20:37
d791b9a to
60a69fd
Compare
andygrove
changed the title
The encode entry points are called once per row from datafusion-functions and datafusion-spark. Plain #[inline] left them out-of-line across the crate boundary, which cost 2-3% on Spark's byte paths and up to 18% on to_hex's i32 path relative to the pre-refactor local implementations.
alamb
approved these changes
Jul 22, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me -- thank you @andygrove
I wonder if we have something similar with base64 🤔
| debug_assert_eq!(out.len(), bytes.len() * 2); | ||
| let lookup = case.lookup(); | ||
| for (&b, chunk) in bytes.iter().zip(out.chunks_exact_mut(2)) { | ||
| chunk.copy_from_slice(&lookup[b as usize]); |
Contributor
There was a problem hiding this comment.
this is probably going to be quite fast
| /// pre-allocated output array) and want to write directly into it rather | ||
| /// than appending to a `Vec`. | ||
| /// | ||
| /// The length requirement is only checked in debug builds, via |
Contributor
There was a problem hiding this comment.
Why not just check it and return an internal error? It seems better than a silent failure in release builds
Comment on lines
+165
to
+168
| /// returned slice reborrows it. The `v == 0` case returns early; if that early | ||
| /// return instead reborrowed `buf` as `&buf[..]` inline in [`encode_u64`], the | ||
| /// borrow checker would extend that reborrow over the rest of the function | ||
| /// body, conflicting with the mutable writes on the non-zero path below. |
Contributor
There was a problem hiding this comment.
I think it would be enough here to describe the borrow checker and not try to summarize the implementation detail too
Suggested change
| /// returned slice reborrows it. The `v == 0` case returns early; if that early | |
| /// return instead reborrowed `buf` as `&buf[..]` inline in [`encode_u64`], the | |
| /// borrow checker would extend that reborrow over the rest of the function | |
| /// body, conflicting with the mutable writes on the non-zero path below. | |
| /// returned slice reborrows it. |
| /// assert_eq!(encode_bytes(&[0xde, 0xad, 0xbe, 0xef], HexCase::Upper), "DEADBEEF"); | ||
| /// ``` | ||
| #[inline] | ||
| pub fn encode_bytes(bytes: &[u8], case: HexCase) -> String { |
Contributor
There was a problem hiding this comment.
I think this function will perform poorly as it allocates -- it might be a good idea as a follow on PR to remove. this function and rewrite any uses of it so they don't allocate a bunch of strings
| binary_array.iter().map(|opt| opt.map(hex_encode)).collect(); | ||
| let string_array: StringViewArray = binary_array | ||
| .iter() | ||
| .map(|opt| opt.map(|b| encode_bytes(b, HexCase::Lower))) |
Contributor
There was a problem hiding this comment.
This is unfortunate (as it makes a bunch of allocated strings and then copies them again into theStringViewArray -- using a temporary buffer ald a stringViewBuilder could avoid the intermediate allocation
This can be done as a follow on PR (or never) -- the old version dod this too
|
|
||
| /// Converts the number to its equivalent hexadecimal representation. | ||
| /// to_hex(2147483647) = '7fffffff' | ||
| fn to_hex_array<T: ArrowPrimitiveType>(array: &ArrayRef) -> Result<ArrayRef> |
Contributor
There was a problem hiding this comment.
here is an good example of creating StringArrays directly with hex
| unsafe { std::str::from_utf8_unchecked(hex).to_string() } | ||
| } | ||
|
|
||
| /// Trait for converting integer types to hexadecimal in a buffer |
Contributor
There was a problem hiding this comment.
I wonder if we should move this code into the common library crate as well
Contributor
|
run benchmarks to_hex hex sha2 crypto |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing auto-opt/to_hex-datafusion-20260721-094849 (5e7e211) to eef1017 (merge-base) diff using: to_hex File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing auto-opt/to_hex-datafusion-20260721-094849 (5e7e211) to eef1017 (merge-base) diff using: crypto File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing auto-opt/to_hex-datafusion-20260721-094849 (5e7e211) to eef1017 (merge-base) diff using: sha2 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing auto-opt/to_hex-datafusion-20260721-094849 (5e7e211) to eef1017 (merge-base) diff using: hex File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagecrypto — base (merge-base)
crypto — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageto_hex — base (merge-base)
to_hex — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesha2 — base (merge-base)
sha2 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagehex — base (merge-base)
hex — branch
File an issue against this benchmark runner |
Contributor
|
Bench results look good |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
N/A
Rationale for this change
Hex encoding was implemented six times across the workspace, at two different levels of optimization.
Three copies used a fast byte-pair lookup table:
datafusion/spark/src/function/math/hex.rsdatafusion/functions/src/string/to_hex.rsdatafusion/functions/src/encoding/inner.rs(via thehexcrate)The other three used a slower nibble-at-a-time loop pushing one character at a time:
datafusion/functions/src/crypto/md5.rsdatafusion/spark/src/function/hash/sha1.rsdatafusion/spark/src/function/hash/sha2.rsBeyond the duplication, this split meant the digest functions were paying for a slower encoder
than the one already sitting elsewhere in the tree. Consolidating on a single implementation
removes the duplication and moves
md5,sha1, andsha2onto the fast path.What changes are included in this PR?
A new
datafusion_common::utils::hexmodule holds the only hex encoder in the workspace:Four entry points rather than one because the call sites have genuinely different output needs,
and forcing them through a single shape would cost an allocation somewhere:
to_hexwritesstraight into a
StringArrayvalues buffer, Spark'shexappends to a reused scratchVec,encodewrites into a pre-sized slice, and the digest functions want an ownedString.Migrated call sites, all producing bit-identical output:
functions/src/string/to_hex.rsspark/src/function/math/hex.rsbuild_hex_lookupandhex_int64deletedfunctions/src/crypto/md5.rshex_encodedeletedspark/src/function/hash/sha1.rsspark/src/function/hash/sha2.rshex_encodedeleted; eight call sites migratedfunctions/src/encoding/inner.rshexcrateDeliberately left alone:
hex::decode/hex::decode_to_sliceinencoding/inner.rs, and Spark'sunhex. The decodedirection has different semantics — Spark's
unhexleft-pads odd-length input, thehexcratedoes not — so unifying it is a separate question. The
hexdependency stays for those.ScalarValue's binaryDisplayimpl incommon/src/scalar/mod.rs, which writes to afmt::Formatterrather than a byte buffer, and is a cold path.Three details worth a reviewer's attention:
encode_u64disagreed on zero:to_hexwrote'0'into the caller'sbuffer, Spark's
hex_int64returned a'staticb"0"that never touched it. The shared versionalways writes into the buffer and returns a subslice of it, so the lifetime is uniform. Both
callers still produce
"0".hex_encode_bytesguards large binary input withchecked_mul(2)+try_reserve,returning a
DataFusionErrorrather than aborting on allocation failure. That guard stays at thecall site;
encode_bytes_intoperforms no reservation of its own, so behaviour is unchanged.#[inline(always)], not#[inline]. They are called once per row from twoother crates, and plain
#[inline]left them out-of-line across the crate boundary. Measuredcost of that: +3% on Spark's byte paths and up to +18% on
to_hex's i32 path, i.e. therefactor was a net regression on those benchmarks until the attribute changed.
Are these changes tested?
Every migrated function keeps its existing unit and sqllogictest coverage, which is what pins
bit-identical output — in particular
spark/hash/sha1.sltandsha2.sltassert concrete digeststrings for all four SHA-2 bit lengths across both the scalar and array paths, and
expr.sltcovers
to_hexandmd5.New unit tests in
common/src/utils/hex.rscover zero,u64::MAX, single-nibble values, theodd/even digit-count boundary, two's complement of negative input, empty input, all 256 byte
values in both cases, appending into a non-empty buffer, and that a reused scratch buffer never
leaks stale digits between calls. Tests cross-check against
format!("{:x}")rather thanrestating the implementation.
Two Spark tests changed.
test_hex_int64now driveshex_encode_int64instead of the deletedprivate
hex_int64, keeping all ten cases includingi64::MIN,-1, and the uppercaseexpectations.
test_hex_lookup_table_covers_all_byteswas deleted — it only cross-checked the rawlookup tables, and
encode_bytes_covers_every_byte_valuenow does that exhaustively through thepublic API. A test was added for Spark's lowercase byte path, which previously had no coverage.
Benchmarks
Criterion,
apache/main@eef101769as baseline. Median of the reported change interval.datafusion/functions/benches/to_hex.rs:i32_randomi64_randomi64_large_valuesscalar_i32−2.3%,scalar_i64−1.8%.datafusion/spark/benches/sha2.rs:array_binary_256array_scalar_binary_256scalar/size=1−3.3%.datafusion/spark/benches/hex.rs:hex_int64hex_int64_dicthex_utf8hex_binaryThe
hex_utf8andhex_binarypaths already used the byte-pair table before this PR, so they areexpected to be flat; they are.
datafusion/functions/benches/crypto.rs:md5_array−4.0%,md5_scalar−3.7%. Thesha224/sha256/sha384/sha512cases in that file range from −0.1% to +2.2%, but they exercisecrypto/basic.rs, which this PR does not modify and which contains no hex encoding — those numbersare run-to-run variance, not an effect of this change.
No number is quoted for Spark
sha1: it has no benchmark, and its change is the same substitutionapplied to
md5andsha2.Are there any user-facing changes?
No behaviour change — all migrated functions produce byte-identical output.
datafusion_common::utils::hexis new public API ondatafusion-common.