andygrove commented on code in PR #7340:
URL: https://github.com/apache/datafusion-comet/pull/7340#discussion_r4430019664
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -312,6 +318,60 @@ fn parquet_convert_array_impl(
Ok(Arc::new(string_array))
}
+ // Unannotated BINARY read as a string. Spark's cast replaces
malformed UTF-8 with
+ // U+FFFD, where Arrow's safe cast below would return NULL. Top-level
columns don't
+ // reach this arm (#7032). The reader keeps a file's ARROW:schema
types, so an Arrow
+ // writer's `LargeBinary` or `BinaryView` arrives as is; Spark's cast
takes only
+ // `Binary`, and converting `LargeBinary` reuses its value buffer.
+ (Binary | LargeBinary | BinaryView, Utf8) => {
+ let array = match from_type {
+ Binary => array,
+ _ => cast_with_options(&array, &Binary, &PARQUET_OPTIONS)?,
+ };
+ if in_map_key {
+ // Spark tells map keys apart by their bytes. Decoding turns
the keys `FF` and
+ // `EF BF BD` into the same U+FFFD, so a lookup of one returns
the other's value
+ // and `map_from_entries` fails with DUPLICATED_MAP_KEY. A key
keeps its bytes
+ // instead, unchecked, as a top-level value does (#7032).
+ let (offsets, values, nulls) =
array.as_binary::<i32>().clone().into_parts();
+ let keys = unsafe { StringArray::new_unchecked(offsets,
values, nulls) };
Review Comment:
Thanks @sunchao, you're right that the map-key arm breaks the
`new_unchecked` contract when a key is not valid UTF-8. To answer my own
question: this isn't specific to the backport. The same code came from #7106
and is on `main` and `branch-1.2` (and in `1.2.0-rc1`).
It is the same class of problem as the top-level path, though. A top-level
unannotated BINARY column read as a string already reaches a `Utf8` array
unchecked in every release, because arrow-rs validates UTF-8 only for
UTF8-annotated columns (#7032, #4764). Before #7106 these map keys went through
Arrow's safe cast and became NULL. Now they keep Spark's bytes and carry the
same risk as a top-level value. So I'd like to merge this so `branch-1.1`
matches `branch-1.2`, and fix the map-key path with your suggestions in a
follow-up: #7360.
--
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]
