andygrove commented on code in PR #4763:
URL: https://github.com/apache/datafusion-comet/pull/4763#discussion_r3579951647
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -853,20 +859,78 @@ fn spark_binary_formatter(value: &[u8],
binary_output_style: BinaryOutputStyle)
}
}
-fn cast_binary_formatter(value: &[u8]) -> String {
- match String::from_utf8(value.to_vec()) {
- Ok(value) => value,
- Err(_) => unsafe { String::from_utf8_unchecked(value.to_vec()) },
- }
+fn cast_binary_formatter(value: &[u8]) -> Cow<'_, str> {
+ // CAST(binary AS string) reinterprets the bytes as UTF-8, like Spark's
UTF8String.fromBytes.
+ // Spark keeps the raw bytes, but Arrow's Utf8 type requires valid UTF-8,
and building a String
+ // from non-UTF-8 bytes is undefined behaviour (#4488). Decode
JVM-compatibly-lossily instead:
+ // `decode_utf8_spark_lossy` replaces ill-formed sequences with U+FFFD
exactly as Spark's
+ // `new String(bytes, UTF_8)` does (the same decoder Comet's native
shuffle uses, #4521). The
+ // result is memory-safe valid UTF-8, never feeds invalid bytes into
downstream native string
+ // kernels, and matches Spark's rendered output byte-for-byte. It diverges
from Spark only under
+ // byte-level round-trips such as CAST(CAST(x AS string) AS binary), where
Spark still has the
+ // original bytes and Comet has the U+FFFD replacements. Returning `Cow`
lets the valid path
+ // borrow so the caller appends without an intermediate allocation.
+ decode_utf8_spark_lossy(value)
Review Comment:
Added in fb126fdf8. There are Rust unit tests for this in `cast.rs`, but
nothing pinned the behavior at the Spark level, so `CometCastSuite` now has
`cast BinaryType to StringType - invalid UTF-8 bytes are replaced`: it runs
deterministic invalid inputs (two invalid bytes, the surrogate-range `ED A0 80`
that collapses to a single U+FFFD where Rust's `from_utf8_lossy` would emit
three, a truncated two-byte lead, plus a valid and an empty value) through
`castTest` for Spark parity, then asserts the collected values equal both `new
String(bytes, UTF_8)` (the JVM oracle) and the literal expected strings. A
change in `decode_utf8_spark_lossy` now fails an end-to-end test, not just the
unit tests.
--
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]
