perf: optimize `cast_binary_to_string` binary-format styles (up to 27x faster) by andygrove · Pull Request #4912 · apache/datafusion-comet

andygrove · 2026-07-13T18:39:28Z

Which issue does this PR close?

N/A

Rationale for this change

cast_binary_to_string handles CAST(binary AS string) and Spark 4.0's ToPrettyString binary formatting styles (uppercase hex, space-delimited uppercase hex, base64, and the array-of-signed-bytes "basic" style). The previous implementation allocated an owned String per row (and, for the hex styles, per byte), which dominated runtime for the format-heavy styles.

What changes are included in this PR?

This branch now includes #4763, which replaced the unsafe zero-copy reinterpret of the default CAST(binary AS string) path with a JVM-compatible lossy UTF-8 decode (ill-formed bytes become U+FFFD, matching Spark's new String(bytes, UTF_8)). The default and UTF8 paths therefore go through decode_utf8_spark_lossy and are not changed by this PR.

The optimization targets the ToPrettyString styles:

Encode directly into a single pre-sized GenericStringBuilder instead of building a throwaway String per row.
Table-driven uppercase-hex and signed-byte formatting written straight into the builder's value buffer.
A single reusable scratch buffer for base64.
The value buffer capacity is computed up front from the input's total byte length so it is allocated once.

How are these changes tested?

Existing unit tests in cast.rs cover each style, plus the JVM-compatible lossy decoding of invalid UTF-8 on the default and UTF8 paths.

Benchmark (criterion), this PR vs apache/main:

case	main	this PR	speedup
hex, long values	10.95 ms	400.9 µs	27.3x
hex discrete, short values	2.265 ms	89.08 µs	25.4x
hex discrete, with nulls	3.424 ms	182.4 µs	18.8x
hex discrete, long values	11.04 ms	706.9 µs	15.6x
basic, long values	8.758 ms	3.450 ms	2.5x
base64, long values	344.6 µs	295.6 µs	1.2x
default style, long values	2.548 ms	2.474 ms	1.03x

Full criterion output (--baseline main):

cast_binary_to_string: hex discrete, short values
                        time:   [88.535 µs 89.083 µs 89.699 µs]
                        change: [−96.151% −96.123% −96.093%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: hex discrete, long values
                        time:   [703.10 µs 706.89 µs 711.23 µs]
                        change: [−93.507% −93.468% −93.427%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: hex discrete, with nulls
                        time:   [181.74 µs 182.42 µs 183.15 µs]
                        change: [−94.714% −94.685% −94.654%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: hex, long values
                        time:   [397.71 µs 400.88 µs 403.89 µs]
                        change: [−96.332% −96.308% −96.286%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: basic, long values
                        time:   [3.4383 ms 3.4502 ms 3.4628 ms]
                        change: [−60.800% −60.606% −60.407%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: base64, long values
                        time:   [294.46 µs 295.55 µs 296.65 µs]
                        change: [−14.750% −14.364% −13.958%] (p = 0.00 < 0.05)
                        Performance has improved.

cast_binary_to_string: default style, long values
                        time:   [2.4437 ms 2.4741 ms 2.5060 ms]
                        change: [−4.3443% −2.9004% −1.4035%] (p = 0.00 < 0.05)
                        Performance has improved.

parthchandra · 2026-07-13T21:43:17Z

Great improvement @andy.
One question though, could either this code or some downstream code if there is non utf-8 data in the binary data converted to string.

andygrove · 2026-07-13T23:36:27Z

Great improvement @andy. One question though, could either this code or some downstream code if there is non utf-8 data in the binary data converted to string.

It looks like a word is missing from that sentence @parthchandra?

parthchandra · 2026-07-14T00:44:27Z

Great improvement @andy. One question though, could either this code or some downstream code if there is non utf-8 data in the binary data converted to string.

It looks like a word is missing from that sentence @parthchandra?

Oops.
Could either this code or some downstream code fail/panic if there is non utf-8 data in the binary data converted to string.

andygrove · 2026-07-14T00:53:13Z

Oops. Could either this code or some downstream code fail/panic if there is non utf-8 data in the binary data converted to string.

This PR doesn't change any behavior, but yes, this is a known issue and there is already a PR open to fix this. We should merge that one first. Could you take a look?

#4763

parthchandra · 2026-07-14T01:03:46Z

Looks like this PR will maybe conflict with #4763. Agree we should merge #4763 first.

…to_string-datafusion-comet-20260713-121057 # Conflicts: # native/spark-expr/Cargo.toml # native/spark-expr/src/conversion_funcs/cast.rs

andygrove · 2026-07-15T17:54:05Z

Looks like this PR will maybe conflict with #4763. Agree we should merge #4763 first.

#4763 is merged, and this PR has been upmerged.

mbutrovich

First pass, thanks @andygrove!

mbutrovich · 2026-07-15T21:57:28Z

+/// Writes `byte` reinterpreted as a signed decimal, as Spark does when printing a byte array.
+#[inline]
+fn write_i8<W: Write>(out: &mut W, byte: u8) -> std::fmt::Result {
+    // Widened to `i16` so that negating `i8::MIN` does not overflow.
+    let value = i8::from_ne_bytes([byte]) as i16;
+    let magnitude = if value < 0 {
+        out.write_char('-')?;
+        -value
+    } else {
+        value
+    };
+    if magnitude >= 100 {
+        out.write_char((b'0' + (magnitude / 100) as u8) as char)?;
+    }
+    if magnitude >= 10 {
+        out.write_char((b'0' + (magnitude / 10 % 10) as u8) as char)?;
+    }
+    out.write_char((b'0' + (magnitude % 10) as u8) as char)
+}


The hand-rolled three-digit expansion is correct but is more code than needed and is easy to get subtly wrong (the i16 widening exists solely to guard -i8::MIN). Rust's formatter writes an i8 straight into any fmt::Write target with no heap allocation, so the whole function collapses to one line and drops the widening reasoning.

Suggested change:

#[inline] fn write_i8<W: Write>(out: &mut W, byte: u8) -> std::fmt::Result { write!(out, "{}", byte as i8) }

This still writes directly into the builder value buffer (no per-row String), so it keeps the win. If a measurement shows write!'s formatting machinery costs enough to matter on the basic case (2.5x today), keep the manual version but add a benchmark note justifying it, otherwise the manual code is unjustified.

mbutrovich · 2026-07-15T21:57:40Z

+fn write_upper_hex<W: Write>(out: &mut W, byte: u8) -> std::fmt::Result {
+    out.write_char(UPPER_HEX_DIGITS[(byte >> 4) as usize] as char)?;
+    out.write_char(UPPER_HEX_DIGITS[(byte & 0x0f) as usize] as char)
+}


Two write_char calls per byte go through char::encode_utf8 twice. Since both digits are ASCII, building a [u8; 2] and writing it once as a &str is one extend_from_slice of two bytes and removes the as char casts.

Suggested change:

#[inline] fn write_upper_hex<W: Write>(out: &mut W, byte: u8) -> std::fmt::Result { let buf = [UPPER_HEX_DIGITS[(byte >> 4) as usize], UPPER_HEX_DIGITS[(byte & 0x0f) as usize]]; // SAFETY: both bytes are ASCII hex digits. out.write_str(unsafe { std::str::from_utf8_unchecked(&buf) }) }

If the unsafe is unwelcome, str::from_utf8(&buf).unwrap() is fine since the table is ASCII. Hex is the hottest style (27x, called twice per byte), so this is the one place a byte-slice write is worth it over write_char.

mbutrovich · 2026-07-15T21:58:17Z

+    // to a multiple of four per row, hence the extra `num_rows` slack. The default and UTF8 paths
+    // decode JVM-compatibly-lossily, which can expand each byte to a 3-byte U+FFFD replacement.
+    let capacity = match spark_cast_options.binary_output_style {
+        None | Some(BinaryOutputStyle::Utf8) => 3 * value_bytes,


None | Some(Utf8) => 3 * value_bytes sizes for the worst case where every byte expands to a 3-byte U+FFFD. For the common all-valid-UTF-8 column this reserves 3x the needed value buffer up front and never uses it. The default path is the least-improved case (1.03x), so the extra reservation is pure cost there.

Consider sizing at value_bytes (exact for valid UTF-8, the builder grows on the rare invalid byte) or leaving the default/UTF8 path on the plain builder capacity. Measure the default case before and after so this is evidence-based rather than a guess.

mbutrovich · 2026-07-15T21:59:12Z

+        let Some(value) = value else {
+            builder.append_null();
+            continue;
+        };


Correctness depends on every non-null row ending with append_value("") so the null branch's early continue never inherits pending bytes from a prior row. This holds today, but a future edit that adds a continue or an early return inside the match would silently merge one row's bytes into the next. A one-line comment at the null branch stating "the previous iteration always finalized its row with append_value(\"\"), so no bytes are pending here" documents the invariant.

mbutrovich · 2026-07-15T21:59:28Z

+    let capacity = match spark_cast_options.binary_output_style {
+        None | Some(BinaryOutputStyle::Utf8) => 3 * value_bytes,
+        Some(BinaryOutputStyle::Basic) => 6 * value_bytes + 2 * num_rows,
+        Some(BinaryOutputStyle::Base64) => 4 * value_bytes.div_ceil(3) + num_rows,


4 * value_bytes.div_ceil(3) + num_rows already rounds each row up to a full 4-char group via div_ceil(3), then adds another num_rows. Since the encoder is no-pad, the true max is value_bytes.div_ceil(3) * 4 summed per row, and the + num_rows is redundant headroom. Harmless (capacity is a hint) but the comment claims the + num_rows covers the round-up that div_ceil already covers. Drop the + num_rows or fix the comment.

perf: optimize cast_binary_to_string in datafusion-comet-spark-expr

3193f1a

andygrove changed the title ~~perf: optimize cast_binary_to_string in datafusion-comet-spark-expr~~ perf: optimize cast_binary_to_string (up to 7000x faster) Jul 13, 2026

chore: drop now-unused hex dependency from spark-expr

5b446e6

andygrove marked this pull request as ready for review July 13, 2026 20:20

andygrove marked this pull request as draft July 14, 2026 00:53

Merge remote-tracking branch 'apache/main' into auto-opt/cast_binary_…

1dcf990

…to_string-datafusion-comet-20260713-121057 # Conflicts: # native/spark-expr/Cargo.toml # native/spark-expr/src/conversion_funcs/cast.rs

andygrove changed the title ~~perf: optimize cast_binary_to_string (up to 7000x faster)~~ perf: optimize cast_binary_to_string binary-format styles (up to 27x faster) Jul 15, 2026

andygrove marked this pull request as ready for review July 15, 2026 17:53

mbutrovich reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: optimize `cast_binary_to_string` binary-format styles (up to 27x faster)#4912

perf: optimize `cast_binary_to_string` binary-format styles (up to 27x faster)#4912
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:auto-opt/cast_binary_to_string-datafusion-comet-20260713-121057

andygrove commented Jul 13, 2026 •

edited

Loading

Uh oh!

parthchandra commented Jul 13, 2026

Uh oh!

andygrove commented Jul 13, 2026

Uh oh!

parthchandra commented Jul 14, 2026

Uh oh!

andygrove commented Jul 14, 2026

Uh oh!

parthchandra commented Jul 14, 2026

Uh oh!

andygrove commented Jul 15, 2026

Uh oh!

mbutrovich left a comment

Uh oh!

mbutrovich Jul 15, 2026

Uh oh!

mbutrovich Jul 15, 2026

Uh oh!

mbutrovich Jul 15, 2026

Uh oh!

mbutrovich Jul 15, 2026

Uh oh!

mbutrovich Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

andygrove commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

parthchandra commented Jul 13, 2026

Uh oh!

andygrove commented Jul 13, 2026

Uh oh!

parthchandra commented Jul 14, 2026

Uh oh!

andygrove commented Jul 14, 2026

Uh oh!

parthchandra commented Jul 14, 2026

Uh oh!

andygrove commented Jul 15, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andygrove commented Jul 13, 2026 •

edited

Loading