andygrove commented on code in PR #4763:
URL: https://github.com/apache/datafusion-comet/pull/4763#discussion_r3579953777
##########
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:
You are right, and the docs in this PR understated the divergence: they only
called out byte-level round-trips. Distinct ill-formed sequences all decode to
the same U+FFFD, so `CAST(b AS STRING) = CAST(X'EFBFBD' AS STRING)` is `false`
in Spark (UTF8String compares the raw bytes) and `true` in Comet, and the same
collapse can affect joins, grouping, ordering, and byte-based functions such as
`contains`. fb126fdf8 documents that class explicitly in the compatibility
guide, the cast expression-audit note, and the comment on
`cast_binary_formatter`.
I have kept the cast `Compatible` rather than downgrading it, for two
reasons:
1. Comet already behaves this way independently of the cast. The native
columnar shuffle decodes with the same `decode_utf8_spark_lossy` (#4521), so an
invalid-UTF-8 string that goes through a Comet shuffle already collapses to
U+FFFD regardless of how it was produced. Making just this one cast fall back
would not restore byte-exact string identity in a Comet plan.
2. Falling back would trade this divergence for the UB the PR removes. If
the cast runs on the JVM, Spark produces a raw-byte `UTF8String`, and that
value then enters the native pipeline through the unchecked Arrow FFI import
(`from_ffi` does not validate UTF-8), where any downstream native string kernel
reading it via `StringArray::value` materializes an invalid `&str`. That is Gap
B in #4764.
The real fix is a single ingress policy (decode, do not reinterpret, at
every boundary), tracked by the epic #4764, which the compatibility guide now
links from this section. Happy to be argued out of this if you think the
value-identity divergence is severe enough to warrant gating the cast in the
meantime.
##########
docs/source/contributor-guide/adding_a_new_expression.md:
##########
@@ -478,6 +478,21 @@ pub fn spark_ceil(
}
```
+#### Producing strings from arbitrary bytes
+
+Spark's `StringType` may contain bytes that are not valid UTF-8 (Spark stores
them verbatim), but
+Arrow's string arrays must be valid UTF-8. Any native code that turns
arbitrary bytes into a string
+must therefore **decode** them, not reinterpret them. Building a Rust
`String`/`&str` from non-UTF-8
+bytes is undefined behaviour, and an Arrow string array that holds invalid
UTF-8 is unsound for the
+downstream kernels that read it via `StringArray::value` (which assumes the
UTF-8 invariant).
+
+Use `crate::utils::decode_utf8_spark_lossy`. It replaces each ill-formed
sequence with `U+FFFD`
Review Comment:
Fixed in fb126fdf8: the reference now points at
`datafusion_comet_common::decode_utf8_spark_lossy`
(`native/common/src/utils.rs`), which is where it moved in the previous round.
--
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]
