fix: match Spark's duplicate field and field id semantics in parquet field lookup

[dwsmith1983]

Which issue does this PR close?

Closes #6192: the native scan read a nested struct by position when the requested fields matched the file's by name and type but not by id, so a nested column dropped and added back under the same name came back with its old values and swapped ids came back unswapped, where Spark returns null for the re-added column and exchanges the swapped ones.

Rationale for this change

Spark's ParquetReadSupport.clipParquetGroupFields resolves an id-bearing requested field by id at every nesting level and raises _LEGACY_ERROR_TEMP_2094 when one requested id matches more than one file field. The native scan has a metadata-only relabel shortcut in CometCastColumnExpr that fires when the physical and requested types differ only in nested field names, and it never looked at field ids, so a nested struct whose names and types line up was read by position. Below the root the scan also read the first field carrying a duplicated id where Spark raises.

What changes are included in this PR?

• The relabel shortcut checks ids. In types_differ_only_in_field_names, with field-id reading on, a requested struct field that carries an id must find that id on the file field at its position, otherwise the cast goes through spark_parquet_convert, which resolves by id. The result is computed once per expression into a bool rather than by walking the type tree on every batch.
• match_struct_fields raises Spark's _LEGACY_ERROR_TEMP_2094 when a requested id matches more than one file field at any depth, instead of reading the first match. The root check in remap_physical_schema is unchanged.
• The matched fields in that message are bracketed, [x, y], the way Spark's matchIdField renders them, since the shims pass the list verbatim.
• schema_adapter.rs uses field_id from parquet_support.rs in place of its own copy, and the doc on id_duplicate_roots records why a repeated root name is rejected even for an unambiguous id (Investigate mixed-type duplicate Parquet roots: Spark field-ID reads return anomalous values #5964).

A file without key-value metadata whose schema equals the requested schema is still read positionally, because DataFusion's opener skips the expression adapter for it. #6004 covers the case where the requested schema itself repeats an id.

How are these changes tested?

Rust: test_swapped_field_ids_bypass_relabel_shortcut in cast_column.rs, which fails without the id check in the shortcut. requested_duplicate_field_id_errors with unrequested_duplicate_field_id_reads_fine in parquet_support.rs. parquet_duplicate_file_field_id_rejected_when_requested in schema_adapter.rs, a DataSourceExec scan over a written file whose struct carries one id twice.

Scala, in ParquetReadSuite: one table-driven test reads a struct, a list element and a map value under a dropped-and-re-added id and under swapped ids, pinned on every profile and compared with Spark where Spark's vectorized reader accepts the read, which below a list or map is 4.1 on. multiple id matches compares the message with Spark's in full, and a duplicate id inside a struct on a Spark-written file asserts the native scan and the message. CometNativeReaderSuite asserts the exception class of the nested duplicate name refusal.

ParquetReadV1Suite and CometNativeReaderSuite on the 3.5, 4.0 and 4.1 profiles, the parquet tests of the core crate, clippy, fmt and spotless.

[sunchao]

This fixes stray-name matching and placeholder collisions, and adds nested duplicate-ID rejection and last-wins exact-name lookup. One gap remains: metadata-only struct relabeling bypasses the duplicate-ID check, as detailed inline.

I compared the code with maintained Spark 3.5 and 4.0 sources. Eight component-check groups passed using extracted Comet helpers with Arrow/Parquet 58.4.0 and DataFusion 54.1.0. A separate probe reproduced the cast bypass and verified a renamed-child control. These probes use limited scaffolding and are not full Comet scan, JNI or Spark query tests. The reported 246 native and 58 Spark 3.5 tests are the author's results.

At 04:51 UTC, current-head CI had 29 successful, 32 running and 7 skipped checks. Full CI validation was still pending.

[sunchao]

[P2] Run duplicate-ID validation before metadata-only struct relabeling

Could you route metadata-only struct adaptations through this validation too? For file struct s<x: int id=1, y: int id=1, z: int id=2> and requested s<x: int id=1, y: int id=3, z: int id=2>, Spark rejects requested ID 1 as ambiguous. DataFusion emits a struct cast, but CometCastColumnExpr::evaluate takes types_differ_only_in_field_names and calls relabel_array, because that predicate ignores field-ID metadata. The new lookup never runs and leaves all three physical values in place. A focused probe using the current cast expression and a real Arrow/Parquet round trip returned [42, 43, 44], while renaming requested x made the same input reach the duplicate-ID error. Could you guard the relabel shortcut for ID-based reads and add a cast-expression or scan regression with unchanged child names?

[dwsmith1983]

Good catch, the shortcut sailed right past the new validation. Fixed in 3d68f22: the relabel arm is now guarded so that when use_field_id is set and the requested type carries field id metadata, evaluation falls through to the struct conversion where the duplicate id lookup runs. Chose the guard at the call site rather than inside types_differ_only_in_field_names since that predicate is a pure structural comparison with no access to the parquet options. Your exact probe is now a regression test (unchanged child names, duplicate id 1, asserts the 2094 error) plus a companion pinning that the fast path survives for name only differences without ids and for the flag alone.

[sunchao]

Reviewed head 58e67fee against base 55ae4f20. The PR is focused, but I found one new regression, an incomplete validation fix, and avoidable batch-processing overhead.

1. [P2] Placeholder collisions can suppress column defaults.
   schema_adapter.rs:177 reserves exact names, while missing-column detection uses case-folded names. A generated __comet_unmatched_field_id_1 therefore collides with requested __COMET_UNMATCHED_FIELD_ID_1. In my reproduction, the base returns the configured default 7; this PR returns NULL.

   Reserve names using the existing folded schema names. That change passed the reproduction. Constructing the reservation set only when shielding needs it would also avoid extra hashing on ordinary reads without field IDs.

2. [P2] Duplicate-ID validation still depends on whether a cast occurs.
   The new guard in cast_column.rs:292 misses identical physical/requested schemas. Such reads can omit the cast entirely or return before the guard.

   A real native Parquet scan of identical s<x: long id=1, y: long id=1> schemas returned [42, 43]; Spark 4.1.3’s schema-clipping check rejected the duplicate ID. This also occurs on the base, so it is an incomplete fix, not a new regression. Validation needs to cover reads that require no conversion.

3. Avoid allocating a vector for every unique ID on every struct conversion.
   parquet_support.rs:270 changes the index to HashMap<i32, Vec<usize>>. An allocation probe using the old and new construction loops measured 8 → 264 allocations for 256 unique IDs.

   A compact unique/duplicate entry would preserve the behavior. Collect matching field names only when reporting an ambiguity. The new contains_field_id_metadata predicate also depends on immutable expression state and can be computed once.

The strongest design improvement is to resolve and validate requested fields once per file schema, then reuse the mapping across batches. That addresses the validation bypasses and repeated lookup work together. Metadata-only relabeling remains safe when the resolved mapping is positional. A small mapping object is a useful abstraction here.

Validation: 100 native Parquet tests passed, with default HDFS features disabled. Additional head/base probes confirmed both correctness cases. Performance evidence measures component allocations, not overall scan speed. CI snapshot: 57 passed, 7 running, 7 skipped. Nothing was posted to GitHub.

[dwsmith1983]

Thanks, all three are addressed in a69b5f5, following the once-per-file design you suggested.

The physical expression adapter factory already runs once per file schema, so it now resolves a small FieldMapping tree (struct sources, list and large list elements, map key and value, leaf) for every logical field that holds a struct, mirroring Spark's clipParquetSchema at each nesting level. Duplicate requested ids and ambiguous case-insensitive names are detected there, stored per logical field, and raised when the column is referenced, whether or not a cast is later emitted. The per-batch conversion receives the resolved mapping and applies it positionally, so there is no hashing or per-id allocation on the batch path; the index type is a compact entry that records an index and an ambiguity flag, and matching names are collected only when the error is built. The contains_field_id_metadata predicate is gone; the relabel shortcut is gated on the mapping being positional instead.

Your repros: the identical s<x: long id=1, y: long id=1> schema now raises the duplicate id error with no cast in the plan, pinned in Rust through the exec path and in ParquetReadV1Suite (the Scala case fails against the previous native library and passes now). The folded placeholder collision returns the configured default again, pinned in the adapter tests with Spark-style key-value metadata on the file.

One residual worth naming: DataFusion's opener skips the adapter entirely when the logical and physical schemas compare equal and no predicate exists. Spark-written files always carry key-value metadata that arrow-rs folds into the physical schema, so they always go through the adapter, but a file with no metadata at all and duplicated ids inside a struct would still read positionally. Happy to cover that in a follow-up if you think it matters.

[dwsmith1983]

@sunchao the once-per-file mapping round covering your three findings is pushed. Ready for another look.

[andygrove]

convert_struct in native/core/src/parquet/parquet_support.rs around line 563 now calls array.column(from_index) with an index resolved at planning time against adapted_physical_schema, where the old code derived it from the runtime array's own DataType. The only guard is sources.len() != to_fields.len(), which checks the target side. What guarantees the struct array the reader hands back always carries the same children in the same order as the physical field the mapping was resolved against? If that can drift at all, this is an index panic in the executor rather than a DataFusionError. Checking from_index against array.num_columns() would bound the worst case.

The description lists last-wins exact-name resolution as one of the three fixes and resolve_struct_mapping does it for struct children. At the top level in case-sensitive mode with no field ids, needs_remap is false in schema_adapter.rs around line 520, so resolution falls to DefaultPhysicalExprAdapter, which goes through Schema::index_of and returns the first match. Spark builds caseSensitiveParquetFieldMap at the root message level with the same .toMap it uses for nested groups. Was the top level deliberately left out of scope?

The field-id ambiguity path is covered from several angles now. The case-insensitive name ambiguity that resolve_struct_mapping raises around line 388 does not appear to have a companion test in the new struct_field_matching module. It might be worth pinning that half too, since it is the branch that decides between an error and a silently wrong column.

On the residual you named where DataFusion skips the adapter when the two schemas compare equal and there is no predicate, I confirmed that short circuit in the 55.0.0 opener. Could you open a tracking issue and link it here so it does not get lost? The branch also conflicts with main right now and needs a rebase before anything meaningful runs against it.

[dwsmith1983]

Rebased onto main and the three points are in the head (b438dce).

Bounds: convert_struct now looks the source index up with columns().get and returns an error naming the requested field, the index, and the child count instead of indexing; a test feeds a struct with fewer children than the mapping expects and asserts the error. A struct column arriving with a non-struct mapping errors the same way rather than passing through. I did not add a type-equality check there, since convert_array dispatches on the runtime child type and strict equality would reject coercions it handles.

Root last-wins: not deliberate, the top level had simply fallen to the default adapter. With duplicate exact names at the root in case-sensitive mode the remap path now runs, the shadowed earlier fields get a placeholder name so the default adapter's index_of lands on the last one, and the nested resolver does the same; Spark 3.5's clipParquetGroupFields uses one .toMap for root and nested groups. Tests at the adapter level and through DataSourceExec, plus one that a field id match still wins over the duplicate. One thing worth knowing: parquet-mr writes two root columns named d into a single column chunk keyed by path, so a Spark-written file with that shape reads six interleaved values in Spark and arrow-rs alike, and no Spark-comparable end-to-end assertion exists for it; the Rust scan test uses arrow-rs to write the file.

The case-insensitive ambiguity now has its companion test in struct_field_matching: A and a against a requested a errors naming both in case-insensitive mode and reads a in case-sensitive mode.

The opener short circuit is tracked in #5801.

[andygrove]

Spark brackets this list in matchIdField with mkString("[", ", ", "]"), so its message reads Found duplicate field(s) "1": [x, y] in id mapping mode and ours reads "1": x, y. Now that this helper formats the list for root and nested fields alike, could it add the brackets? The Scala test could then compare the whole message with Spark's instead of just the prefix.

[dwsmith1983]

Now that this helper formats the list for root and nested fields alike, could it add the brackets?

Done. field_names_with_id now returns the list bracketed and comma joined the way matchIdField renders it, so the message Comet hands to foundDuplicateFieldInFieldIdLookupModeError reads Found duplicate field(s) "1": [x, y] in id mapping mode on both sides. The Rust display string dropped its own brackets so the list is not wrapped twice, and the Rust assertions pin id=1 matches [x, y] rather than only the id.

On the Scala side, multiple id matches and duplicate field id inside a struct is rejected when a requested id matches two fields run the same read with Comet off, take the duplicate id message Spark raises, check that it lists the expected fields ("1": [a, rand2] for the root case, "1": [x, y] for the struct), and assert that Comet's message equals it in full.

[andygrove]

Thanks for the quick turnaround on today's comments. The brackets, the drop-and-re-add tests and sunchao's shape all look right, and I checked the 3.5 and 4.0 gating against ParquetColumnVector in 3.5.9 and 4.0.4, which do have that check. Since your runs were on 3.5, I ran everything on the default Spark 4.1 profile, and it's all green. I also disabled each of the new guards in turn, the relabel gate, the nested and root duplicate-id errors, and the nested raise in rewrite, and every one of them has a test that fails without it.

On narrowing id_duplicate_roots, I'd keep it as it is. I measured the shape on a file with roots a (id 1) = 1, a (id 2) = 2 and b (id 3) = 3, in both case modes. With the guard removed, Comet returns [1, 2] for a read of id 1 or id 2, which is two rows from a one-row file. Spark doesn't give a usable answer either. Its vectorized reader returns [1] for both ids, so id 2 comes back with id 1's value, and the parquet-mr reader fails outright. That's the same name-based leaf lookup #5964 describes. So the rejection protects the decoder rather than diverging from a Spark behavior we could match. Could the doc comment on known_divergence_repeated_root_name_rejects_unambiguous_field_id say that, and link #5964? As written it reads as a gap waiting to be closed, and closing it would bring #5783 back at the root.

[andygrove]

This turns main's error into an INTERNAL_ERROR. SparkError::Internal goes through SparkErrorConverter as SparkException.internalError, so a requested nested duplicate now reaches the user as [INTERNAL_ERROR] Found duplicate Parquet field name 'dup' SQLSTATE: XX000. That tells them they've hit a bug in the engine, for a limitation scans.md documents. On main the same read raises CometNativeException with the plain message. On this branch the root check and the non-pruning path still do, so the same limitation surfaces as two exception classes depending on nesting. In the test log, all ten duplicate Parquet field names fail clearly cases, the schema-merge test and the fixture test take the INTERNAL_ERROR path. The tests don't notice because they only match the message text. Could the resolver raise the same error main does here? And would you add an assertion on the exception class in one of those tests, so the paths can't drift apart again?

[dwsmith1983]

The resolver is back to main's match_struct_fields with its DataFusionResult, so a requested nested duplicate name raises the same execution error main raises, and nothing is cached per field. The only addition there is the duplicate-id arm, which raises SparkError::DuplicateFieldByFieldId the way the root check does. CometNativeReaderSuite duplicate Parquet field names fail clearly now asserts that the cause chain holds a CometNativeException and no INTERNAL_ERROR, so the nested and root paths cannot drift apart again.

[andygrove]

Is there something about duplicate-nested-names.parquet that a Spark-written file can't reproduce? It was written by pyarrow 25.0.1, and nothing in the repo says how to regenerate it. The two reads it covers, s.other succeeding and s.dup refusing, are the pair CometNativeReaderSuite already covers on main with a file written from named_struct('dup', id, 'dup', id + 100, 'other', id + 900). If the pyarrow writer is the point, could the test say why? Otherwise I'd drop the test and the binary, which also takes a little off the diff comphead was concerned about.

[dwsmith1983]

Nothing about the pyarrow writer mattered, the file only gave the two reads a nested duplicate name. The test and duplicate-nested-names.parquet are gone. CometNativeReaderSuite covers both reads on main with a file written from named_struct.

[comphead]

Thanks @dwsmith1983 checking this now

[comphead]

Thanks @dwsmith1983. I reproduced #6192 end to end on main at 64e98918ab (Spark 4.1.3, native scan). A struct, an array<struct> element and a map value all come back by position, both when x is dropped and re-added and when the ids are swapped. A root-level swap and a nested swap with one extra requested field both match Spark, so the relabel shortcut is the whole cause, and the positional gate here fixes it.

The fix itself is a small part of the diff, and much of the rest has no user-visible effect on main, so I would like to split it. Only the first row below comes from a run. The others are from reading the code.

Change	On main	Suggestion
Relabel shortcut ignores field ids (#6192)	Wrong results	Keep, as a small PR (sketch inline)
A requested nested id matches two file fields	Reads the first match where Spark raises _LEGACY_ERROR_TEMP_2094	Keep, as a small PR (sketch inline)
[x, y] brackets in the 2094 message	The shims pass matchedFields verbatim, so Spark's message loses them	Keep, with the previous row
Root duplicate id raised only for referenced columns	Fails the whole scan, even when that column is not selected	Optional follow-up that reuses id_duplicate_roots
Shield after the name match	Not reachable from Spark (inline)	Drop
FieldMapping once per file, list_element_field, convert_list, renames	No behavior change, #5681 already resolves list elements by id	Separate PR with a benchmark

A local sketch of the first three rows is +60/-14 across cast_column.rs, parquet_support.rs, schema_adapter.rs and error.rs. It compiles and passes clippy. I have not run tests on it. It is enough because:

• check_conversion already walks the requested tree with match_struct_fields once per file, before every non-Variant CometCastColumnExpr, so the duplicate-id error fires at plan time.
• #6004 declines requested schemas that repeat an id. An ambiguous read therefore always has differing physical and logical types, so it always gets a cast.

Tests: keep, already covered on main, or tied to the new machinery

• Keep (these fail on main): test_swapped_field_ids_bypass_relabel_shortcut, the three nested column dropped and re-added… tests (merged, see inline), requested_duplicate_field_id_errors together with unrequested_duplicate_field_id_reads_fine, parquet_duplicate_file_field_id_rejected_when_requested, duplicate field id inside a struct…, the message check in multiple id matches, and root_duplicate_id_raised_only_for_referenced_columns if the root change stays.
• Already covered on main:
  • duplicate_exact_names_are_rejected_when_requested by issue_5783_nested_name_duplicate.
  • unrequested_duplicate_siblings_do_not_block_the_read and the Scala duplicate exact nested names… test by CometNativeReaderSuite (inline).
  • case_insensitive_ambiguous_names_error_but_exact_match_reads and case_insensitive_byte_identical_children_raise_spark_duplicate_error by #5751's native reader duplicate nested struct fields: * (case-insensitive).
  • convert_list_to_large_list_null_fills_missing_element_field by list_representations_preserve_missing_struct_fields.
  • known_divergence_repeated_root_name_rejects_unambiguous_field_id by the CometNativeReaderSuite pin its own doc names. Its rationale could live in a comment on id_duplicate_roots.
  • parquet_field_id_miss_null_fills_but_exact_name_sibling_still_reads and parquet_field_id_match_beats_stray_column_with_requested_name, which read the same on main as far as I can tell.
• Tied to the new machinery, so they go with it: field_match_records_ambiguity_without_allocating, the four resolve_mapping_* tests, convert_list_to_large_list_reads_elements_by_id, leaf_mapping_converts_list_and_map_elements, mapping_index_beyond_struct_children_errors, test_relabel_shortcut_kept_for_name_only_differences_without_ids, parquet_field_id_miss_case_insensitive_sibling_still_reads and parquet_shield_placeholder_never_folds_onto_requested_column.

None of these can move to SQL file tests, because Spark SQL DDL cannot attach parquet.field.id metadata.

For context, Spark 3.5.9, 4.0.4 and 4.1.3 share the same field-id logic in clipParquetGroupFields. DuckDB, StarRocks and iceberg-rust's RecordBatchTransformer also match nested fields by id at every level and resolve outside the batch loop, so resolving once per file is a sound direction for its own PR. DuckDB and StarRocks silently keep the first duplicate id, so the 2094 error is only about matching Spark.

[comphead]

This gate is the whole #6192 fix, and it does not need a FieldMapping. Checking ids in the positional walk that types_differ_only_in_field_names already does is enough, with use_field_id read from parquet_options:

// Struct arm, per (pf, lf) pair
&& (!use_field_id || field_id(lf).is_none() || field_id(lf) == field_id(pf))

Separately, positional and types_differ_only_in_field_names depend only on fields fixed at construction, yet both walk the type tree on every batch. One bool computed in with_parquet_options would cover both. And since field_mapping is always set together with parquet_options, the two could be one Option.

[dwsmith1983]

Done that way. types_differ_only_in_field_names takes use_field_id and its struct arm requires !use_field_id || field_id(lf).is_none() || field_id(lf) == field_id(pf) per pair, with use_field_id read from the parquet options. The result is computed once into a relabel_only bool, in try_new without ids and again in with_parquet_options with the options' use_field_id, so evaluate no longer walks the type tree per batch. FieldMapping is gone, so parquet_options is the only Option left on the expression.

[comphead]

Returning SparkError from the resolver, so errors can be cached per field, changes two error classes. Here, the case-sensitive nested duplicate becomes SparkError::Internal, which ShimSparkErrorConverter turns into SparkException.internalError, Spark's INTERNAL_ERROR (SQLSTATE XX000). Spark reserves that for engine bugs, and main raises an execution error with the same text. At L426, a JNI failure while folding names is flattened into a string, so the Java exception that #5845 made propagate is lost. The tests only match message substrings, so neither shows up. Keeping DataFusionResult and not caching errors avoids both.

[dwsmith1983]

match_struct_fields keeps DataFusionResult and returns each error where it finds it, so the case-sensitive nested duplicate stays main's execution error and a fold_names failure propagates as the Java exception #5845 made it. Nothing is cached per field. CometNativeReaderSuite asserts the exception class on the nested duplicate read.

[comphead]

The later index that also records is never read, because both ambiguous branches return an error. An Option<usize> entry does the same in one pass without this struct, record_field_match or field_match_records_ambiguity_without_allocating. It is also all the nested duplicate-id fix needs in match_struct_fields on main:

// None marks an id that more than one file field carries
map.entry(id).and_modify(|m| *m = None).or_insert(Some(i));
// ...
(true, Some(id)) => match from_id_to_index.get(&id) {
    Some(None) => Err(/* DuplicateFieldByFieldId, matched fields as "[x, y]" */),
    index => Ok(index.copied().flatten()),
},

[dwsmith1983]

Done as sketched. from_id_to_index is a HashMap<i32, Option<usize>> filled with map.entry(id).and_modify(|m| *m = None).or_insert(Some(i)), and the id arm raises DuplicateFieldByFieldId on Some(None) with the matched names from field_names_with_id. FieldMatch, record_field_match and their test are gone.

[comphead]

impl From<SparkError> for DataFusionError in native/common/src/error.rs already does this, so the call sites can use .into(). In the same vein, field_id becomes pub(crate) here while schema_adapter.rs keeps its identical parse_field_id. One of the two could go.

[dwsmith1983]

Both call sites use .into() now. parse_field_id is gone from schema_adapter.rs, which uses field_id from parquet_support.rs instead.

[comphead]

The renames (parquet_convert_array_impl to convert_array, parquet_convert_struct_to_struct to convert_struct, while parquet_convert_map_to_map keeps its name), the convert_list extraction, the has_timestamp_unit rewrite and the reflowed arms do not change behavior, but they add a lot of diff. Resolving once per file does save the per-batch match_struct_fields work on main (folded names and a Vec per name, though ASCII names never reach the JVM). That saving is not measured yet. Could it be its own PR, with a case added to native/core/benches/parquet_timestamp_conversion.rs?

[dwsmith1983]

Dropped from this PR. parquet_support.rs is main plus the duplicate-id arm in match_struct_fields and field_names_with_id. The once-per-file mapping, the renames, convert_list, list_element_field and the has_timestamp_unit change sit on a side branch for a follow-up PR, which will carry a case in native/core/benches/parquet_timestamp_conversion.rs so the per-batch saving is measured.

[comphead]

I don't think this ordering is reachable from Spark. It only matters when an id-bearing and an id-less requested field fold to the same name, and DataSource.resolveRelation rejects that read schema with COLUMN_ALREADY_EXISTS (checkSchemaColumnNameDuplication, in 3.5.9, 4.0.4 and 4.1.3). In case-sensitive mode the shield only fires on identical names, which Spark rejects in any mode. By my reading, the case-sensitive Kappa read already returns (NULL, 7) on main. I would keep main's order and drop the case-insensitive Kappa and placeholder tests.

[dwsmith1983]

Main's order stays. The reorder, the case-insensitive Kappa test and the placeholder test are gone.

[comphead]

This parallels id_duplicate_roots: both map a logical name to a root error that rewrite raises when the column is referenced. One map would do, and rewrite could then check each referenced column in one pass instead of three loops over col_names.

[dwsmith1983]

The root change is dropped, so remap_physical_schema raises the duplicate root id for the whole schema as it does on main and root_id_ambiguities is gone. If a follow-up narrows that to referenced columns it will reuse id_duplicate_roots as the one map and check each referenced column in one pass.

[comphead]

This repeats scan_parquet apart from the defaults and the writer properties. Could scan_parquet take those as optional arguments instead? Likewise, field_with_id here and in parquet_support.rs, and int_field_with_id in cast_column.rs, overlap the existing id_meta and struct_type_with_field_id helpers.

[dwsmith1983]

scan_with_adapter and scan_with_defaults are gone. The kept test calls scan_parquet as it is. The key-value metadata was there so that a file whose schema equals the requested one still reaches the adapter, and in this test the schemas differ by the id on y, so the adapter runs without it. field_with_id and int_field_with_id are gone too. The tests in all three files use id_meta and struct_type_with_field_id, with struct_type_with_field_id now pub(crate) in the schema_adapter test module.

[comphead]

CometNativeReaderSuite already covers both halves on main. duplicate Parquet field names fail clearly - * covers the refused read (struct, array element and map value, at two batch sizes), and duplicate Parquet field names - exact-name projection works in both resolver modes covers the unique sibling. Those tests write the file with named_struct, so I think this test and duplicate-nested-names.parquet can both go.

[dwsmith1983]

Both are gone.

[comphead]

These pin #6192 well. Could the three tests become one table-driven test over s, l and m, with a swapped-id row (x (id 2), y (id 1)) added? The swapped case is the other shape in the issue, and right now only Rust covers it.

[dwsmith1983]

Done. One test reads s, l and m under two read schemas, x (id 3), y (id 2) for the dropped-and-re-added shape and x (id 2), y (id 1) for the swapped one, so the Scala side now covers both shapes in the issue. The expected rows are derived from the written rows through the per-case remap, null for the re-added x and the two values exchanged for the swap, and pinned with checkAnswer after the comparison with Spark. The comparison with Spark below a list or map starts at 4.1 for both shapes: on 3.5 Spark's own vectorized reader rejects the clipped struct in ParquetColumnVector for the swapped ids too, since the clip carries the file's field order there.

[dwsmith1983]

The nested duplicate name raises main's execution error again, since match_struct_fields is back to its DataFusionResult shape with only the duplicate-id arm added, and CometNativeReaderSuite now asserts a CometNativeException and no INTERNAL_ERROR in the cause chain. The id_duplicate_roots doc carries your measurement, the two rows from a one-row file, Spark's vectorized reader returning id 1's value for both ids and parquet-mr failing, links #5964 and names the CometNativeReaderSuite test that pins the rejection, and the known_divergence test is gone with it. The pyarrow fixture and its test are gone as well. The PR is cut to comphead's table, with the rest on a side branch for a follow-up.

[dwsmith1983]

The PR now follows the table. It keeps the relabel gate in cast_column.rs, the nested duplicate-id error in match_struct_fields and the brackets in the 2094 message, each as sketched inline. The root change is dropped and main's behaviour stays, the shield keeps main's order, and the FieldMapping machinery, the list changes and the renames are on a side branch for a follow-up PR with a benchmark case.

The tests follow the keep list: the swapped-id cast test, the three drop-and-re-add tests merged into one table-driven test with the swapped row, the requested and unrequested duplicate-id pair, the DataSourceExec scan over a file with a duplicated id, the struct duplicate-id Scala test and the message check in multiple id matches. The rationale for known_divergence_repeated_root_name_rejects_unambiguous_field_id is now a comment on id_duplicate_roots, and that test is gone with the others you listed. Against main the diff is +459/-76 across error.rs, cast_column.rs, parquet_support.rs, schema_adapter.rs, ParquetReadSuite and CometNativeReaderSuite. ParquetReadV1Suite and CometNativeReaderSuite pass on the 3.5, 4.0 and 4.1 profiles.

[andygrove]

I re-ran this at d377eacc9 on the default Spark 4.1 profile, and the core crate's parquet tests, clippy, fmt, ParquetReadV1Suite and CometNativeReaderSuite all pass. I also disabled each of the three changes in turn, the id check in the relabel shortcut, the duplicate-id arm in match_struct_fields and the brackets in field_names_with_id, and each one fails a Rust test without it.

The shape I wanted to rule out is a file with s<x (id 1), y (id 1)> read as s<x (id 1), y>, with no id on y. #6004 lets that schema through because it repeats no id, and the new check in the shortcut accepts it position by position. It still raises Spark's 2094 for a struct and for a list element, because check_conversion runs match_struct_fields when the file is opened, before the shortcut can fire.

CI hasn't run on this head yet, so it needs a green run before it merges, but the change itself looks good to me.

[dwsmith1983]

@andygrove could you approve CI on 958107a34? A merge with main replaced the head the earlier run was on. Main only brought in an Iceberg residual fix and a test change, so nothing in this PR changed.

[comphead]

Thanks @dwsmith1983 now PR is 450 LOC, down from 2.1k, thanks for bearing with me

[andygrove]

@dwsmith1983 could you hold off on merging main for now? Each push cancels the running CI, so the Spark SQL and Iceberg jobs never get to finish. The merge queue tests against the latest main anyway, so the branch doesn't need to be up to date to land.

[dwsmith1983]

could you hold off on merging main for now?

Will do, across all my open PRs. The one exception I expect is between this PR and #6116, which conflict in a single helper at the top of schema_adapter.rs. Whichever of the two lands second will need that hunk resolved, and I will push that only once the first one has merged.