dwsmith1983 commented on PR #5654: URL: https://github.com/apache/datafusion-comet/pull/5654#issuecomment-5618378879
> Is there a reason not to fold it into #5654? No. #5808 is now the last three commits here and is closed, so this is the one reviewable change; the description covers both. > Could you and @peterxcli agree an order, and could the one that rebases extend all three places together? Extended all three here so the order stops mattering: `resolve_field_mapping`, `type_holds_struct` and `convert_array` share one `list_element_field` helper covering `List`, `LargeList`, `FixedSizeList` and the two view types, and the converter now converts the element values through the mapping first, rebuilds the array in the file's representation around them, and only then lets Arrow's cast change the layout, the same shape as #5681's arm. Three cases in `struct_field_matching` pin it: a `List<Struct>` requested as `LargeList<Struct>` resolves to a non-positional element mapping, converts by id with swapped values coming back swapped, and null-fills a requested element field the file lacks instead of reading its neighbour. Left @peterxcli a note on #5681 with that, so either rebase is taking one arm over the other. > Would a comment on `type_holds_struct` pointing at the other two help, or is there a way to derive one from another? Derived: all three call the helper, and its doc names them. > Would you narrow the root check in `remap_physical_schema` to the requested columns, or file it as a follow-up and reword the description so the asymmetry reads the way it actually behaves? Narrowed here, and you are right about the description; it now says what happens. `remap_physical_schema` records the ambiguity per logical field instead of failing, leaves the colliding physical fields out of the id rename so the shield fake-names them, and `rewrite` raises for referenced columns the way nested ambiguities already are. Your three-column example reads `[7]` through the adapter now (test `unrequested_duplicate_root_field_ids_read_through_adapter`, with a column the file lacks forcing the adapter), and `rewrite` of the ambiguous column still raises `_LEGACY_ERROR_TEMP_2094`. Both paths validate the same scope, the columns a read requests. > Could you thread `case_sensitive` through the helper and cover it? Threaded, with the case the description now explains: `s(id 10)<x, X>` in a metadata-free file fails at footer load with `_LEGACY_ERROR_TEMP_2093` in case-insensitive mode and reads in case-sensitive mode. The root duplicate id and the `list<struct>` element duplicate are pinned as well, and the description says the root check is closed by the same footer check. The three inline items are in: the memo is a `parking_lot::Mutex` with the helper and the poison imports gone, `throw_spark_error_json` is the one throw site with the `External` arm going through `spark_error_json_in_chain` (a new test drives a `SparkError` behind `External` and `Context` across the JNI boundary and checks the class), and the opener-skip explanation lives on `FieldIdCheck` with the other two pointing at it. The description also carries the `COLUMN_ALREADY_EXISTS` reasoning and the note that the chain walk widens which typed errors keep their class. Core crate 367 tests, bridge 32, each commit compiles on its own, and `ParquetReadV1Suite`, `CometNativeReaderSuite` and `SparkErrorConverterSuite` pass on Spark 3.5 against the rebuilt library. -- 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]
