mbutrovich commented on code in PR #4912:
URL: https://github.com/apache/datafusion-comet/pull/4912#discussion_r3591015828
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -803,77 +856,59 @@ fn cast_binary_to_string<O: OffsetSizeTrait>(
.downcast_ref::<GenericByteArray<GenericBinaryType<O>>>()
.unwrap();
- // Build with a GenericStringBuilder and append &str straight from the
decoder's Cow so the
- // common valid-UTF-8 cast path copies the bytes once (into the Arrow
value buffer) instead of
- // twice (an owned String, then a copy into the buffer).
- let mut builder = GenericStringBuilder::<O>::new();
- for value in input.iter() {
- match value {
- Some(value) => match spark_cast_options.binary_output_style {
- // ToPrettyString styles (Spark 4.0+) build owned strings,
which is unavoidable.
- Some(s) => builder.append_value(spark_binary_formatter(value,
s)),
- // Default CAST(binary AS string): borrows in the valid path,
appends once.
- None => builder.append_value(cast_binary_formatter(value)),
- },
- None => builder.append_null(),
- }
- }
- Ok(Arc::new(builder.finish()))
-}
+ let num_rows = input.len();
+ let offsets = input.value_offsets();
+ let value_bytes = offsets[num_rows].as_usize() - offsets[0].as_usize();
+ // Upper bound on the encoded length so the value buffer is allocated
once. Base64 rounds up
+ // 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,
+ Some(BinaryOutputStyle::Basic) => 6 * value_bytes + 2 * num_rows,
+ Some(BinaryOutputStyle::Base64) => 4 * value_bytes.div_ceil(3) +
num_rows,
+ Some(BinaryOutputStyle::Hex) => 2 * value_bytes,
+ Some(BinaryOutputStyle::HexDiscrete) => 3 * value_bytes + 2 * num_rows,
+ };
-/// This function mimics the [BinaryFormatter]:
https://github.com/apache/spark/blob/v4.0.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala#L449-L468
-/// used by SparkSQL's ToPrettyString expression.
-/// The BinaryFormatter was [introduced]:
https://issues.apache.org/jira/browse/SPARK-47911 in Spark 4.0.0
-/// Before Spark 4.0.0, the default is SPACE_DELIMITED_UPPERCASE_HEX
-fn spark_binary_formatter(value: &[u8], binary_output_style:
BinaryOutputStyle) -> String {
- match binary_output_style {
- // Spark's UTF8 BinaryFormatter renders via
`UTF8String.fromBytes(bytes).toString`, i.e.
- // `new String(bytes, UTF_8)`. Route through the shared JVM-compatible
decoder so invalid
- // bytes become U+FFFD (matching Spark) instead of panicking on
non-UTF-8 input (#4488).
- BinaryOutputStyle::Utf8 => decode_utf8_spark_lossy(value).into_owned(),
- BinaryOutputStyle::Basic => {
- format!(
- "{:?}",
- value
- .iter()
- .map(|v| i8::from_ne_bytes([*v]))
- .collect::<Vec<i8>>()
- )
- }
- BinaryOutputStyle::Base64 => BASE64_STANDARD_NO_PAD.encode(value),
- BinaryOutputStyle::Hex => value
- .iter()
- .map(|v| hex::encode_upper([*v]))
- .collect::<String>(),
- BinaryOutputStyle::HexDiscrete => {
+ let mut builder = GenericStringBuilder::<O>::with_capacity(num_rows,
capacity);
+ // Base64 is the only style that cannot encode straight into the builder.
+ let mut base64_buffer = String::new();
+ for value in input.iter() {
+ let Some(value) = value else {
+ builder.append_null();
+ continue;
+ };
Review Comment:
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.
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -803,77 +856,59 @@ fn cast_binary_to_string<O: OffsetSizeTrait>(
.downcast_ref::<GenericByteArray<GenericBinaryType<O>>>()
.unwrap();
- // Build with a GenericStringBuilder and append &str straight from the
decoder's Cow so the
- // common valid-UTF-8 cast path copies the bytes once (into the Arrow
value buffer) instead of
- // twice (an owned String, then a copy into the buffer).
- let mut builder = GenericStringBuilder::<O>::new();
- for value in input.iter() {
- match value {
- Some(value) => match spark_cast_options.binary_output_style {
- // ToPrettyString styles (Spark 4.0+) build owned strings,
which is unavoidable.
- Some(s) => builder.append_value(spark_binary_formatter(value,
s)),
- // Default CAST(binary AS string): borrows in the valid path,
appends once.
- None => builder.append_value(cast_binary_formatter(value)),
- },
- None => builder.append_null(),
- }
- }
- Ok(Arc::new(builder.finish()))
-}
+ let num_rows = input.len();
+ let offsets = input.value_offsets();
+ let value_bytes = offsets[num_rows].as_usize() - offsets[0].as_usize();
+ // Upper bound on the encoded length so the value buffer is allocated
once. Base64 rounds up
+ // 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,
Review Comment:
`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.
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -794,6 +793,60 @@ impl PhysicalExpr for Cast {
}
}
+const UPPER_HEX_DIGITS: [u8; 16] = *b"0123456789ABCDEF";
+
+/// Writes `byte` as two uppercase hex digits.
+#[inline]
+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)
+}
Review Comment:
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:
```rust
#[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`.
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -794,6 +793,60 @@ impl PhysicalExpr for Cast {
}
}
+const UPPER_HEX_DIGITS: [u8; 16] = *b"0123456789ABCDEF";
+
+/// Writes `byte` as two uppercase hex digits.
+#[inline]
+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)
+}
+
+/// 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)
+}
Review Comment:
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:
```rust
#[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.
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -803,77 +856,59 @@ fn cast_binary_to_string<O: OffsetSizeTrait>(
.downcast_ref::<GenericByteArray<GenericBinaryType<O>>>()
.unwrap();
- // Build with a GenericStringBuilder and append &str straight from the
decoder's Cow so the
- // common valid-UTF-8 cast path copies the bytes once (into the Arrow
value buffer) instead of
- // twice (an owned String, then a copy into the buffer).
- let mut builder = GenericStringBuilder::<O>::new();
- for value in input.iter() {
- match value {
- Some(value) => match spark_cast_options.binary_output_style {
- // ToPrettyString styles (Spark 4.0+) build owned strings,
which is unavoidable.
- Some(s) => builder.append_value(spark_binary_formatter(value,
s)),
- // Default CAST(binary AS string): borrows in the valid path,
appends once.
- None => builder.append_value(cast_binary_formatter(value)),
- },
- None => builder.append_null(),
- }
- }
- Ok(Arc::new(builder.finish()))
-}
+ let num_rows = input.len();
+ let offsets = input.value_offsets();
+ let value_bytes = offsets[num_rows].as_usize() - offsets[0].as_usize();
+ // Upper bound on the encoded length so the value buffer is allocated
once. Base64 rounds up
+ // 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,
+ Some(BinaryOutputStyle::Basic) => 6 * value_bytes + 2 * num_rows,
+ Some(BinaryOutputStyle::Base64) => 4 * value_bytes.div_ceil(3) +
num_rows,
Review Comment:
`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.
--
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]
