andygrove commented on code in PR #4912:
URL: https://github.com/apache/datafusion-comet/pull/4912#discussion_r3603996100
##########
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:
Collapsed to `write!(out, "{}", byte as i8)`.
##########
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:
Switched to a two-byte ASCII slice + `write_str` via `from_utf8_unchecked`
(SAFETY comment cites the ASCII UPPER_HEX_DIGITS table).
##########
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:
Dropped the 3x reservation for the default/UTF8 path. Valid UTF-8 fits at
`value_bytes` and the builder grows on the rare U+FFFD expansion. Also fixed
the misleading comment on the Base64 estimate.
##########
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:
Added a one-line note at the null branch stating the previous iteration
always finalized its row with `append_value(\"\")`, so no bytes are pending.
##########
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:
Dropped the redundant `+ num_rows` on the Base64 capacity estimate since
`div_ceil(3)` already rounds each row up to a full 4-char group.
--
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]
