Match Parquet map key/value by position in nested schema coercion

[adriangb]

Match Parquet map key/value by position in nested schema coercion

Which issue does this PR close?

• Follow-up to fix: make Parquet file schema type coercion work on nested schemas #25193 (which closed Parquet schema file type coercion functionality is not applied on all fields in nested schemas, only on the top-level fields #25192). No separate issue.

Rationale for this change

#25193 made apply_file_schema_type_coercions recurse into nested types so that, for example, a nested Utf8 column in a Parquet file is decoded directly as Utf8View when the table schema asks for it, instead of being cast afterwards.

For Map columns the recursion delegated the key/value children to the by-name Struct path, even though the code comment said container children match by position. Parquet readers always name map children key_value/key/value, while Arrow producers commonly use entries/keys/values (e.g. MapBuilder defaults). With that naming mismatch the view coercion was silently skipped for maps and queries kept the slower cast path.

What changes are included in this PR?

• Map key and value children are now matched by position, regardless of their names. Field names, nullability, metadata, the container kind, FixedSizeList width and Map ordering continue to come from the file schema.
• apply_file_schema_type_coercions is restructured around a recursive coerce_data_type helper that operates on DataTypes directly. This removes the throwaway single-field Schemas built for each nested level, the needs_* pre-scan flags (the result is compared to the input instead), and an unreachable match arm in the container branch (the one uncovered line in the fix: make Parquet file schema type coercion work on nested schemas #25193 codecov report).
• The doc comment now describes the nested matching rules (structs by name, lists and maps by position) and which parts of the result always come from the file.

Behaviour is otherwise unchanged; the existing nested parquet .slt files (schema_evolution_nested, parquet_nested_schema_pruning, parquet_filter_pushdown) pass.

What is the testing strategy for this PR?

Unit tests in schema_coercion.rs:

• nested_coercion_preserves_list_containers now uses a table-side child that differs from the file-side child in name, nullability, metadata, FixedSizeList width and Map ordering, and asserts the file's values win. Mutation testing against fix: make Parquet file schema type coercion work on nested schemas #25193 showed that taking the width, ordering or child field from the table survived the previous version of this test; a wrong FixedSizeList width is accepted silently by parquet-rs and would decode data with the wrong list size.
• nested_coercion_matches_map_entries_by_position: Arrow-named table map vs Parquet-named file map, plus a map with a different number of entry children that must be left alone.
• nested_coercion_map_reader_produces_string_views: writes a file with Parquet's map naming and no embedded Arrow schema, coerces against an Arrow-named table schema, and checks the Parquet reader accepts the schema and produces StringViewArray keys and values.
• nested_coercion_list_inside_struct.

The two map tests fail on main without the fix.

Are there any user-facing changes?

No API changes. Map columns whose table schema uses keys/values naming now get the same nested view/string coercion as structs and lists, avoiding a cast during scan.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LA4Rks1XaN8wxpszj1hXQQ

[adriangb]

@adragomir or @aditanase could you take a look at this followup PR? Thanks!

[Copilot]

🟡 Changes recommended

The map unit test uses identical target types and therefore does not verify key/value positional ordering.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Refactors nested Parquet schema coercion to match map key/value fields positionally while preserving file-side container properties.

Changes:

• Adds recursive DataType coercion helpers.
• Preserves file metadata, nullability, widths, and ordering.
• Adds nested map/list coercion tests.

File summaries

File	Description
datafusion/datasource-parquet/src/schema_coercion.rs	Implements positional map coercion and expands tests/documentation.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 99.69789% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.34%. Comparing base (cc06f1b) to head (6b729aa).
⚠️ Report is 48 commits behind head on main.

Files with missing lines	Patch %	Lines
...tafusion/datasource-parquet/src/schema_coercion.rs	99.69%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25342      +/-   ##
==========================================
+ Coverage   81.92%   82.34%   +0.41%     
==========================================
  Files        1135     1137       +2     
  Lines      427573   432281    +4708     
  Branches   427573   432281    +4708     
==========================================
+ Hits       350281   355946    +5665     
+ Misses      56365    54832    -1533     
- Partials    20927    21503     +576     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[adriangb]

cc @xudong963 @kosiew as committers that could review this change

[xudong963]

The function unconditionally collects every file field into a new Fields, even when no supported coercion can occur. The previous top-level pre-scan avoided this traversal, reference-count churn, and allocation for schemas containing only primitive non-string types.

So how about restoring a cheap candidate check at apply_file_schema_type_coercions, or building the output lazily only after the first actual change?

[adriangb]

Good catch, fixed in adad560.

Measured against main's implementation (ns per apply_file_schema_type_coercions call, release build):

schema	main	this PR
100 primitive cols, nothing coercible	1629	3255
50 struct/list cols, nothing coercible	17285	5588
100 string cols, all coerced	14351	14837

The one regression left is a hash lookup per column on the all-primitive path, which the old code skipped by early-returning right after building its HashMap. At ~1.6 µs per file open for a 100-column schema I'd rather not reintroduce the hand-maintained list for it, but it's a one-liner if you'd prefer it gone.

[adragomir]

@adriangb looks good, I understand the difference and the addition for the Map type.
I can confirm that I have tested this with our entire test (nothing fancy, just a test on a nested predicate on a large delta table with nested schema), and it still passes correctly - the benchmark return numbers in the ballpark.
Personally, I would minimize churn - like minimizing the changes to minimize the patch, and just treat the Map stuff using the new function, but that's not related to functionality.

[adriangb]

@adragomir I looked for ways to minimize the diff / churn but all of the changes here seem worth it. In particular on keeping main's structure and only routing Map through a new function, that would mean keeping the throwaway single-field Schema built per nested level (that's the 17285 → 5588 ns in #25342 (comment), and it would stay on the slow side). The unreachable match arm that was the single uncovered line in #25193's codecov report, and the name-matching that map entries can't use anyway (Map has to come out of that grouped or-pattern either way, since its children match by position).

[sunchao]

Thanks Adrian. I reviewed the positional map matching and the recursive refactor against cc06f1b. In an isolated harness using the source module and the locked Arrow/Parquet 59.3.0 dependencies, all 11 head module tests pass; the same tests on base fail only the two new map regressions. A further 11,664-case schema comparison confines behavior changes to renamed maps. One reader/cast correctness regression remains, detailed inline: invalid UTF-8 in binary map children can now escape as invalid string arrays. This was reproduced against both revisions, with valid multibyte UTF-8 controls.

[sunchao]

[P1] Preserve UTF-8 validation for binary map children

This also enables Binary → string decoding for renamed map children, exposing an existing parquet-rs validation limitation to reads that previously used a validating cast. With Arrow/Parquet 59.3.0, I reproduced this using a file map named key_value/key/value, one key "k" and binary value [0xff], and a table map named entries/keys/values with Utf8View values (write without embedded Arrow metadata, as in the new reader test).

On base, coercion returns None, decoding produces valid binary data, and the strict map cast returns Encountered non UTF-8 data. On this head, the reader returns Ok with an invalid StringViewArray; the subsequent strict cast to the table map also succeeds, and its to_data().validate_full() still fails with Encountered non-UTF-8 data at index 0. The decoder decides whether to validate from the physical Parquet UTF8 annotation, which a binary column lacks, then constructs the overridden string-view array unchecked. Utf8/LargeUtf8 targets instead panic in the debug reader. Valid multibyte UTF-8 passes on both revisions.

Could we retain a validating conversion for binary-to-string map children, or ensure reader validation before enabling that conversion, and add this invalid-byte regression? The Utf8 → Utf8View optimization can still decode directly.

[adriangb]

Fixed in 993334b.

exposing an existing parquet-rs validation limitation to reads that previously used a validating cast

I think this is a key fact: main is quite broken w/ these type overrides.

datafusion-cli -c "COPY (SELECT decode('ff','hex') AS b) TO 'bin.parquet'" -c "set datafusion.execution.parquet.binary_as_string = true" -c "SELECT b FROM 'bin.parquet'"

This gives a segfault, cc @alamb

[adriangb]

Filed both halves:

• parquet-rs: parquet: an ArrowReaderOptions::with_schema override from Binary to a string type skips UTF-8 validation arrow-rs#11140. ByteArrayColumnValueDecoder::new takes the UTF-8 validation decision from the physical annotation only (desc.converted_type() == ConvertedType::UTF8), so a with_schema override from Binary to Utf8, LargeUtf8 or Utf8View returns an unvalidated array. OffsetBuffer::into_array then uses build_unchecked in release builds. The issue has a standalone parquet-only reproducer.
• DataFusion: binary_as_string over a Parquet Binary column with invalid UTF-8 causes a segfault #25509. The segfault above, with the release and debug results for Binary, LargeBinary and BinaryView.

To be explicit about the scope: this is pre-existing on main for top level columns, so it is not introduced here. Utf8View is the worse of the two paths, because the debug check in into_array does not apply to it.

[kosiew]

@adriangb, thanks for working on this. The refactor looks good overall, and I like that the map key/value fields are now matched positionally while preserving the file-side schema attributes. I just have one small suggestion for additional test coverage.

[kosiew]

Could we add a unit test where a map key or value is itself a struct with differently named fields? I think that would be useful for locking in the intended behavior: positional matching applies to the map entries, while nested struct fields continue to be matched by name.

[adriangb]

Added! 6b729aa

[adriangb]

Sending this to merge, thank you @kosiew @sunchao