andygrove opened a new issue, #23810: URL: https://github.com/apache/datafusion/issues/23810
### Is your feature request related to a problem or challenge? Follow-on from https://github.com/apache/datafusion/pull/23766, where @alamb noted the allocation pattern in two places ([1](https://github.com/apache/datafusion/pull/23766#discussion_r_hex), [2](https://github.com/apache/datafusion/pull/23766#discussion_r_md5)). The digest functions hex-encode their output by allocating one `String` per row and then copying each of those into the output array: - `datafusion/functions/src/crypto/md5.rs` — builds a `String` per row, collects into a `StringViewArray` - `datafusion/spark/src/function/hash/sha1.rs` — same, into a `StringArray` - `datafusion/spark/src/function/hash/sha2.rs` — same, at eight call sites All of them go through `datafusion_common::utils::hex::encode_bytes`, which exists only to return an owned `String`: ```rust pub fn encode_bytes(bytes: &[u8], case: HexCase) -> String { let mut out = Vec::with_capacity(bytes.len() * 2); encode_bytes_into(bytes, case, &mut out); unsafe { String::from_utf8_unchecked(out) } } ``` So each row costs a heap allocation plus a copy into the final array, even though the encoded length is known exactly up front (`2 * digest_len`, and digest length is fixed per algorithm). This predates #23766 — that PR consolidated the encoders without changing the calling pattern. ### Describe the solution you'd like Rewrite the callers to encode into a reused buffer and append to a builder, then drop `encode_bytes` entirely. The shared module already has the non-allocating primitives: ```rust pub fn encode_bytes_into(bytes: &[u8], case: HexCase, out: &mut Vec<u8>); pub fn encode_bytes_to_slice(bytes: &[u8], case: HexCase, out: &mut [u8]) -> Result<()>; ``` For `md5` a `StringViewBuilder` plus a scratch buffer avoids the intermediate `String`. For `sha1`/`sha2`, whose digests are fixed-size, the output length for the whole array is known in advance, so the same direct value/offset-buffer approach already used by Spark's `hex` (`hex_encode_bytes`) and by `encode` (`hex_encode_array`) applies. Removing `encode_bytes` also shrinks the public API added to `datafusion-common` in #23766. ### Describe alternatives you've considered Leaving it — the allocation is proportional to row count but small and short-lived, and the digest computation itself dominates. Worth measuring before and after; `datafusion/functions/benches/crypto.rs` and `datafusion/spark/benches/sha2.rs` both exist. ### Additional context `Encoding::encode_bytes` in `datafusion/functions/src/encoding/inner.rs` also calls it, but only on the scalar path, so that one is not hot. -- 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]
