dwsmith1983 commented on PR #5654: URL: https://github.com/apache/datafusion-comet/pull/5654#issuecomment-5814246944
> #5786 is about to be merged, lets see what is still to be fixed and address it in this PR #5786 now owns the duplicate name case at every level. `match_struct_fields` rejects a requested nested name that two siblings carry, `rewrite` rejects a referenced root name the file repeats, `id_duplicate_roots` rejects an id whose physical root name is duplicated, and `check_decoded_field_names` refuses any duplicate inside a subtree a Comet cast decodes in full. This branch sits on top of all of that. The first-wins root binding is gone, since main rejects that read and pins the rejection in `CometNativeReaderSuite`, and the branch's own duplicate-name message is gone with it. Where the resolver here replaced `match_struct_fields`, it raises main's `duplicate Parquet field name` error for byte-identical siblings in both case modes, so every path reports that shape the same way. What this PR still fixes is field ids, which #5786 does not touch. A requested id that two file fields carry inside a struct. File `s (id 2) <x: bigint (id 1), y: bigint (id 1)>`, read with `spark.sql.parquet.fieldId.read.enabled=true` and schema `s (id 2) <x: bigint (id 1)>`. Spark raises `_LEGACY_ERROR_TEMP_2094` from `matchIdField`. On main `match_struct_fields` keeps the first field per id (`map.entry(id).or_insert(i)`) and returns `x = 42` with no error, and none of the duplicate-name checks fire because the names differ. The resolver here raises Spark's error at any depth, and only for the ids a read references, so an unrequested duplicate id at the root no longer fails the whole scan the way `remap_physical_schema` did. `ParquetReadSuite` has this as `duplicate field id inside a struct is rejected when a requested id matches two fields`. Swapped ids inside a struct. File `s <x (id 1), y (id 2)>`, requested `s <x (id 2), y (id 1)>`. Names and types agree, so main's relabel shortcut in `CometCastColumnExpr` hands back the file's columns under the requested names and the values come out swapped. The mapping resolved once per file is not positional for that shape, so the relabel is skipped and each field reads by id. The id shield in `remap_physical_schema` runs after the name match. Main renames a physical column away whenever its folded name equals an id-bearing requested name, before an id-less requested field can claim it by name. File `κ (id 2)`, requested `Κ (id 1)` plus id-less `κ`, case-insensitive: main null-fills both, Spark and this branch return `(NULL, 7)`. The rest is the once-per-file `FieldMapping` handed to every cast instead of re-resolving per batch, and one `list_element_field` helper shared by the resolver, the converter and the adapter's struct walk, so a `List<Struct>` read as `LargeList<Struct>` resolves its elements by id. In the merge, main's `needs_remap`, the collision check in `rewrite`, `id_duplicate_roots` and the `checked_decoded_expr` wrapping stand as written. Removed from the branch: `has_duplicate_names`, `shadowed_by_earlier_duplicate`, the three root first-wins tests, the matching `ParquetReadSuite` read and `duplicate-root-names.parquet`. `issue_5783_nested_name_duplicate` is ported onto `resolve_field_mapping` because `match_struct_fields` no longer exists. One divergence from Spark remains after the merge, and it comes from #5786 rather than from this branch. `id_duplicate_roots` in `schema_adapter.rs` rejects a requested id whenever its physical root name repeats in the file, even when the id itself matches exactly one field. Physical `[d (id 1), d (id 2)]` read as `d (id 1)` with field-id reads on: Spark's `matchIdField` in `ParquetReadSupport.clipParquetGroupFields` (branch-3.5 lines 438-450) finds one field for id 1 and reads it, and main raises `duplicate Parquet field name 'd'`. `CometNativeReaderSuite`'s `duplicate Parquet field names - root group and unprojected root duplicates` pins that rejection, so the merge keeps it and a Rust test named `known_divergence_repeated_root_name_rejects_unambiguous_field_id` records it. The one way to close it is to narrow `id_duplicate_roots` to fire only when the requested id is ambiguous, in this PR, which reverses that test. -- 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]
