andygrove commented on code in PR #4763:
URL: https://github.com/apache/datafusion-comet/pull/4763#discussion_r3538630122
##########
native/spark-expr/src/utils.rs:
##########
@@ -373,6 +374,123 @@ pub fn unlikely(b: bool) -> bool {
b
}
+/// Decode `bytes` as UTF-8 the way Spark renders `StringType` -- `new
String(bytes, UTF_8)` on the
+/// JVM -- replacing each ill-formed sequence with a single `U+FFFD` and
skipping the same number of
+/// bytes the JDK's UTF-8 `CharsetDecoder` (action REPLACE) would. Valid UTF-8
is returned as a
+/// zero-cost borrow.
+///
+/// This intentionally differs from `str::from_utf8_lossy` for surrogate-range
three-byte sequences
+/// (`ED A0..BF ..`, e.g. CESU-8 / Java modified-UTF-8 supplementary chars)
and for some other
+/// ill-formed multi-byte units: `from_utf8_lossy` follows the Unicode
"maximal subpart" rule and
+/// can emit one `U+FFFD` per byte, whereas the JDK collapses certain
ill-formed units into a single
+/// `U+FFFD`. Matching the JDK byte-for-byte means Comet renders arbitrary
bytes identically to
+/// Spark -- whether they arrive via a columnar shuffle or a `CAST(binary AS
string)`. The
+/// per-class malformed lengths below (E0/ED overlong & surrogate handling,
F0/F4 range checks)
+/// match the observable replacement behavior of the JDK UTF-8 decoder; they
were determined from
+/// observed `new String(bytes, UTF_8)` output, not by reviewing the OpenJDK
source.
+pub fn decode_utf8_spark_lossy(bytes: &[u8]) -> Cow<'_, str> {
Review Comment:
Done in bdf9491. `decode_utf8_spark_lossy` and its JVM-oracle tests now live
in `datafusion-comet-common` next to `bytes_to_i128`, and the shuffle crate
imports it from there instead of reaching into `datafusion-comet-spark-expr`.
##########
native/spark-expr/src/utils.rs:
##########
@@ -373,6 +374,123 @@ pub fn unlikely(b: bool) -> bool {
b
}
+/// Decode `bytes` as UTF-8 the way Spark renders `StringType` -- `new
String(bytes, UTF_8)` on the
+/// JVM -- replacing each ill-formed sequence with a single `U+FFFD` and
skipping the same number of
+/// bytes the JDK's UTF-8 `CharsetDecoder` (action REPLACE) would. Valid UTF-8
is returned as a
+/// zero-cost borrow.
+///
+/// This intentionally differs from `str::from_utf8_lossy` for surrogate-range
three-byte sequences
+/// (`ED A0..BF ..`, e.g. CESU-8 / Java modified-UTF-8 supplementary chars)
and for some other
+/// ill-formed multi-byte units: `from_utf8_lossy` follows the Unicode
"maximal subpart" rule and
+/// can emit one `U+FFFD` per byte, whereas the JDK collapses certain
ill-formed units into a single
+/// `U+FFFD`. Matching the JDK byte-for-byte means Comet renders arbitrary
bytes identically to
+/// Spark -- whether they arrive via a columnar shuffle or a `CAST(binary AS
string)`. The
+/// per-class malformed lengths below (E0/ED overlong & surrogate handling,
F0/F4 range checks)
+/// match the observable replacement behavior of the JDK UTF-8 decoder; they
were determined from
+/// observed `new String(bytes, UTF_8)` output, not by reviewing the OpenJDK
source.
+pub fn decode_utf8_spark_lossy(bytes: &[u8]) -> Cow<'_, str> {
+ // Fast path: well-formed UTF-8 borrows with zero copy (the overwhelmingly
common case).
+ if let Ok(s) = std::str::from_utf8(bytes) {
+ return Cow::Borrowed(s);
+ }
+
+ const RC: char = '\u{FFFD}';
+ let n = bytes.len();
+ let mut out = String::with_capacity(n);
+ let mut i = 0;
+ while i < n {
Review Comment:
Added the `// guards above keep cp a valid scalar` note on each of the three
`char::from_u32(cp).unwrap()` calls in bdf9491.
On the `bytes.get(i + 1)` suggestion, I left the explicit `i + 1 >= n` style
as is. Several of those bounds checks do double duty: on a truncated lead they
also drive the `i = n` skip-to-EOF behavior, so folding them into `bytes.get()`
would only apply cleanly to a subset of branches. Converting just those would
make the decoder mix two bounds-checking idioms, and for a delicate
byte-for-byte parity function I would rather keep it uniform. Happy to revisit
if you feel strongly.
--
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]
