fix(arrow): reconcile evolved nested structs by field id, not position

[ewoodbury]

Which issue does this PR close?

• Closes Type casting error when reading files persisted with old schema for complex type #2617
• Revives fix(arrow): reconcile evolved nested structs by field id, not position #2647 (original work by @jordepic)

Rationale for this change

When a nested struct (or a struct nested in a list/map) gains a field after some Parquet files were written, RecordBatchTransformer promoted the column with a positional Arrow cast. Children after the gap no longer line up, so the read fails (for example Casting from Utf8 to Struct(...), or expected 3 got 2 when the new field is appended). Iceberg-Java projects nested children by field id and fills missing optional fields with nulls. The files are valid.

equals_datatype also ignores nested names and field ids, so a same-type reorder or rename was passed through unchanged. A drop-and-re-add of the same nested name kept the old field's values under the new id.

This also shows up in Apache DataFusion Comet 1.0's native Iceberg scan (Spark 4 + Iceberg): vanilla Spark reads the evolved table, Comet's iceberg-rust path errors on the old files.

What changes are included in this PR?

This is @jordepic's #2647 rebased onto current main, plus the follow-up from review on #3255.

Replace the flat cast with a PromotePlan built once per file: nested struct children are matched by PARQUET:field_id, recursing through list and map. Per-batch apply is index lookups and array assembly.

A present column always goes through PromotePlan::build. It is a pass-through only when that plan is a no-op, so reorder, rename, and a same-name field-id change are reconciled instead of copied through.

• Absent nested child that is an identity partition source → that partition value (Column Projection), before initial-default or null. main errors on this read; null-filling it would return the wrong value
• Absent optional children → typed nulls
• Absent required children without a default → error
• Absent children with initial-default → FeatureUnsupported (RecordBatchTransformer does not apply initial-default for nested fields #3261), unless an identity constant applies
• Source struct whose children have no field ids, and the same child count → positional match, and build checks each pair (an id-less int child still promotes to long). Name mapping only stamps top-level ids (Use schema visitor in ArrowReader name mapping #1845)
• Source struct with a different child count, or field ids on only some children → error, rather than null-filling the id-less ones
• Primitive promotions still use cast
• A list read as LargeList from the file is converted to the table's List before its elements are reconciled. Table schemas are always List, so a LargeList target is rejected

Are these changes tested?

Yes. Promote tests compare the whole array, including child names, nullability, and field ids. process_record_batch covers nested reorder, nested rename, an appended field, a drop-and-re-add of the same nested name, an id-less map, an id-less nested struct, and an id-less int child promoted to long. Error tests check ErrorKind, including field ids on only some source children.

• identity_partition_fills_missing_nested_field covers Column Projection for a nested identity source: s.region is absent from the file and comes back as the partition value ("us"). On main that read errors.

Out of scope

Release note: reading a pre-evolution file against a nested field that has an initial-default now returns FeatureUnsupported until #3261 lands.

Nested initial-default (#3261). An id-less reorder of same-typed children cannot be told apart from the file's original order without field ids, so that case stays positional. Recursive name mapping is the proper fix for id-less files (#1845). A struct whose requested children are all missing from the file reads as NULL when another column is projected, because the projection mask selects no leaf under it (#3378, related to #3374). That predates this PR.

[ewoodbury]

Hi @mbutrovich -- I hit the same struct issue, I rebased and revived Jordan's PR including his revisions from your earlier review. It looks like the approach was agreed upon and it just needed another review/approval. Would you be able to take another pass?

[mbutrovich]

Thanks @ewoodbury! I have reopened the original PR. Where possible (engaged authors) I'd like to keep contributions attributed to the original author(s) and benefit from earlier review cycles.

I reopened #2647.

[ewoodbury]

thanks!

[jordepic]

@ewoodbury I'm on PTO for the week if you want to take this one down!

[mbutrovich]

@ewoodbury I'm on PTO for the week if you want to take this one down!

In that case, I will take a review pass on this tonight.

[mbutrovich]

Thanks for picking this up @ewoodbury, and for keeping @jordepic's commit authorship intact. Since @jordepic is out this week, let's finish it here. The new promote_struct_fills_appended_field_by_id test pins the expected 3 got 2 failure from #2617 directly, and it's a good addition.

This branch was cut before my second round on #2647, so that round still applies. The nested reorder/rename pass-through bug reproduces on this head, and the fix I suggested there has a gap for id-less maps. The inline comments have a version that covers both, verified against the full cargo test -p iceberg --lib. Please add the reorder, rename, id-less map, and id-less nested struct cases as tests along with the fix.

[mbutrovich]

The FeatureUnsupported error is better than a silent null. The spec does define this case though. Default values says sub-field defaults are tracked in the sub-field's metadata, and its table gives {"x": 3} with a point.y default of 0 as {"x": 3, "y": 0}. Reads of older files on a v3 table with a defaulted nested field will fail until this is implemented. I couldn't find an issue for it. Could you open one and reference it in this comment and in the error message, so the gap is tracked after merge?

[ewoodbury]

For sure, created #3261. The comment cites it, and the error links to the full url https://github.com/apache/iceberg-rust/issues/3261

[dannycjones]

Thanks for improving the error message here, I just stumbled on this too!

[ewoodbury]

Hey @mbutrovich - thanks for the review! Should have all of those addressed now with a focus on correctness around edge cases, please do take another look once you get a chance !

[mbutrovich]

Thanks for the revision, @ewoodbury! One regression is left, and it comes from the id-less guard I suggested last round, so that's on me. Details are inline, along with a request about how the new tests assert their results. Once both are addressed I expect to approve.

[ewoodbury]

Thanks and great catch @mbutrovich - updated! Fixed the merge conflict and updated tests to be consistent as well

One more regression I found: a Parquet Arrow schema hint can read a list as LargeList while the table schema is List. Main cast that; PromotePlan only matched the same offset width, so the read failed with cannot promote even when the element type was unchanged.

6082824 casts the offset width, then runs the element plan, and test promote_large_list_file_column_to_list covers it.

[ewoodbury]

Hey @mbutrovich -- could you take another pass when you get the chance? thanks!

[mbutrovich]

Thanks for the updates, @ewoodbury, and for catching the LargeList regression. This pass turned up a case I missed in earlier rounds, and I'm sorry to raise it this late.

[ewoodbury]

Thanks @mbutrovich for those catches - updated again and ready for you when you have a chance!

[mbutrovich]

Thanks for sticking with this, @ewoodbury, and thanks to @jordepic for the original change. This LGTM!

[ewoodbury]

Thanks Matt, much appreciated! @laskoviymishka, would you be able to take a look for a final review on this one?

[comphead]

Verdict: approve with nits. I found no correctness problems in the change.

Checked on 4f781f1c9:

• CI is green and the 47 tests in arrow::record_batch_transformer pass locally.
• A scratch reader test (not in the PR) reads a real Parquet file with field ids at every level. It passes on this head and fails on main with Incorrect number of arrays for StructArray fields, expected 4 got 2. The table evolved a struct, a list<struct>, and a map<string, struct> with a reorder, an int to long promotion, and added children.
• Scratch checks on PromotePlan also pass for sliced list, map, and struct inputs, a sliced LargeList, zero-row input, a nullability change on a list element, and a metadata-only difference.

The inline comments are simplifications and test dedup. They were applied and checked locally, and cargo clippy -p iceberg --all-targets --all-features -- -D warnings and cargo fmt --check are clean.

Optional: every new test builds its Arrow arrays by hand, so nothing pins that parquet-rs puts PARQUET:field_id on list element and map key and value. It does today. One reader-level test in reader/projection.rs would guard that assumption. It could stand in for a few of the unit tests listed in the inline comment on the test module.

Question, not caused by this PR: the description says absent optional children become typed nulls. That does not hold when every requested leaf of a struct is missing from the file and another column is projected. get_arrow_projection_mask_with_field_ids then selects no leaf under the struct, so the struct is treated as absent and reads as NULL for every row. Repro: a file {id, s: struct<x (id 5)>} with s null in row 1, read through a table {id, s: struct<x (id 6)>}. Projecting [1, 2] gives struct validity [false, false, false]. Projecting [2] gives [true, false, true], because the mask falls back to ProjectionMask::all() and PromotePlan runs. Java's ParquetValueReaders.structReader reads a presence column for this case (read from the source, not run). This looks like a follow-up issue plus a line under "Out of scope".

Please check and address all review comments in this PR.

[comphead]

apply_list is generic over the offset type, but it has one caller and that caller always passes a ListArray, because a LargeList is cast to List just above. Inlining it removes the helper and the GenericListArray and OffsetSizeTrait imports (import ListArray instead). Delete apply_list below when you apply this.

Suggested change

	Self::apply_list(source, field, element)
	Ok(Arc::new(ListArray::try_new(
	field.clone(),
	source.offsets().clone(),
	element.apply(source.values())?,
	source.nulls().cloned(),
	)?))

[ewoodbury]

Done in ee881aa. Inlined it and dropped GenericListArray and OffsetSizeTrait in favor of ListArray

[comphead]

!source_fields.is_empty() is redundant. With no source fields source_by_id is empty as well, so source_by_id.len() != source_fields.len() is already false.

Suggested change

	if !source_fields.is_empty() && source_by_id.len() != source_fields.len() {
	if source_by_id.len() != source_fields.len() {

[ewoodbury]

Good point, agreed, removed in ee881aa

[comphead]

Constant and Null both end in create_column, which already returns new_null_array for a None value (create_primitive_array_repeated in value.rs). One variant shaped like ColumnSource::Add covers both. It also removes the new_null_array import and an arm in apply_struct:

    Add {
        target_type: DataType,
        value: Option<PrimitiveLiteral>,
    },

Build it with value: Some(datum.literal().clone()) for the identity constant and value: None for the null fill. Apply it with RecordBatchTransformer::create_column(target_type, value.as_ref(), source.len()). Checked locally together with the two comments above: the 47 arrow::record_batch_transformer tests pass, and clippy and cargo fmt --check are clean.

[ewoodbury]

Done in same commit. Both are now ChildPlan::Add { target_type, value: Option<PrimitiveLiteral> }, applied through create_column, and the new_null_array import and Null arm are gone.

[comphead]

Several new tests hit the same PromotePlan branch as another new test. The ones below were removed or merged locally. That takes arrow::record_batch_transformer from 47 to 40 tests and removes about 230 lines. Then 17 single-line mutations were applied to PromotePlan (wrong child index, dropped parent nulls, skipped list or map element apply, no LargeList cast, Cast arm as pass-through, equals_datatype pass-through, and others). Every mutation that the original tests catch is still caught by the reduced set. This is a spot check and not a coverage measurement.

• promote_struct_fills_appended_field_by_id differs from this test only in where the gap sits. One target with a middle and an appended field covers both.
• promote_struct_missing_field_before_nested_list_struct is this test with a pass-through list child.
• promote_large_list_element_struct_fills_added_field_by_id is promote_list_element_struct_fills_added_field_by_id plus the LargeList to List cast. promote_large_list_file_column_to_list covers that cast.
• promote_struct_renames_field_by_id repeats promote_struct_renames_child_via_process_record_batch, which also guards the pass-through decision.
• promote_struct_promotes_child_primitive and promote_struct_preserves_null_parent_rows are covered by promote_evolved_nested_struct_via_process_record_batch (an int to long child and a null parent row).
• promote_idless_struct_promotes_child_int_to_long folds into promote_idless_nested_struct_keeps_data if the table makes the inner x a long.
• promote_evolved_nested_struct_via_process_record_batch rebuilds the setup that transform_top_level already provides. It can call the helper with a StructArray that carries the null buffer.

[ewoodbury]

Thanks - agree with this cleanup, applied in ee881aa, so the module is down from 47 to 40 tests.

The appended field is folded into promote_struct_fills_added_fields_by_id (middle + appended gap). Inner x in promote_idless_nested_struct_keeps_data is now long, so it still guards the id-less int to long case from the last round.

promote_evolved_nested_struct_via_process_record_batch now goes through transform_top_level as well. I spot-checked the reduced set with a few mutations (old equals_datatype guard, dropped parent nulls, wrong child index, no LargeList cast, ignored partition constant), and each is still caught.

[ewoodbury]

Thanks for the thorough review @comphead; I applied all three simplifications and the test dedup in ee881aa (47 > 40 tests, +43/-288).

Good catch on the all-leaves-missing struct. I went ahead and opened up #3378 and added this under Out of scope.

On the optional note, I think it made sense to include reader-level test in projection.rs for a separate PR at this point if that works for you, given the testing and cases it now covers here. Agree with this test cleanup though, I think it could live with the #3378 fix as it relates to the end-to-end read behavior. Thanks!

[laskoviymishka]

@mbutrovich and @comphead have already done careful passes, so I'll keep to what's additive.

The design reads well: reconciling nested children by PARQUET:field_id, resolving a PromotePlan once per file so per-batch work stays index lookups and assembly, and failing loudly where support stops rather than null-filling. The reorder/rename/id-less cases from the earlier rounds are fixed and well covered.

The one thing that gives me slight pause is the id-less positional fallback in build_struct_children — a same-type reorder there still reads the old child's data under the new id with no error. It's documented and no worse than the previous positional cast, so I don't think it blocks, but since the whole change is about retiring positional matching I'd rather we either tighten it (require child names to match) or pin the behavior with a test and cross-link #1845 so it doesn't read as a supported path.

Before merge, the two I'd actually want:

• @comphead's reader-level field-id test. Every fixture hand-stamps ids today, so nothing pins that parquet-rs stamps element / map key+value in practice — if it doesn't, we silently fall back to the path this PR replaces and the suite stays green. It can replace a few of the hand-built unit tests.
• a call on the id-less fallback above — tighten or pin, either is fine as long as it's deliberate.

On @comphead's projection-mask question (an all-leaves-missing struct reading NULL rather than a non-null struct of nulls): agreed it's pre-existing and out of scope here, I'd just file the follow-up and drop a one-line pointer in the code so it's tracked. Same for nested initial-default (#3261) — erroring is the right stopgap, though it looks cheap enough to just support given ChildPlan::Add already carries the literal. Worth a release note that a pre-evolution file with a nested default now errors instead of reading a silent null — a conscious trade, but a behavior change.

Everything else from me is inline and non-blocking. Given two sign-offs already, I'm happy to merge as is, but will leave this PR to soak for some time.

[laskoviymishka]

This positional branch is the one spot that can still silently misassign: a same-type reorder (or a drop-plus-add that keeps the count and types) reads the old child's data under the new child's id, with no error. It's documented and no worse than the old positional cast, so I'm not blocking on it — but since the whole point of this change is to kill positional matching, I'd at least require the source and target child names to match here (as a multiset) and error otherwise, or pin the current behavior with a test and cross-link #1845 so nobody later mistakes this for a supported path.

[ewoodbury]

Makes sense - decided to pin it so promote_idless_same_type_reorder_stays_positional locks in the current behavior, and the comment now calls it a compatibility fallback until #1845. I held off on the name check: it would make renamed children in files without nested ids fail, and a reorder keeps the same names, so it wouldn't catch this case anyway, and #1845 is the real fix.

[laskoviymishka]

HashMap::insert silently overwrites on a duplicate id, so a struct with two children sharing an id collapses the count and then trips the do not all have field ids error below — which points at the wrong cause. I'd catch the collision where it happens:

if source_by_id.insert(id, idx).is_some() {
    return Err(invalid_data!("duplicate field id {id} in struct"));
}

[ewoodbury]

Good catch, done in f136c36. It'll now return duplicate field id {id} in struct when a repeated id is found, and promote_rejects_invalid_source_at_build covers it

[laskoviymishka]

Erroring loudly here beats silently null-filling, so no objection to shipping it as-is. That said, this looks cheap to just support: ChildPlan::Add already carries Option<PrimitiveLiteral>, and the value is iceberg_field.initial_default fed through create_column — the exact path the constant branch just above already uses. If it folds in cleanly I'd rather close #3261 than leave a nested column with a default unreadable for every pre-evolution file; otherwise keep the issue open with a TODO right here.

[ewoodbury]

Agreed it's close. After going through it in detail, I'd rather close #3261 completely than half-close it here. The issue also covers the non-null {} struct default case (which is referenced by #2635), and that will need a non-null struct built from each child's default at both the top level and nested levels, which is a more substantial change.

I'd say better option is to open a focused follow-up that covers both, once this is in. So for now I left the FeatureUnsupported error and its #3261 TODO as is.

[laskoviymishka]

This guard only rejects an illegal source when the target is nested, so a nested source against a primitive target (struct→Int32) falls through to Cast and only errors when the first batch arrives — and never at all for a zero-row file. Since the win of resolving the plan once is surfacing structural mismatches at plan time, I'd add the symmetric arm: error when the source is Struct/List/LargeList/Map and the target isn't.

[ewoodbury]

Done in f136c36. A struct/list/map source against a target of a different kind now errors when the plan is built, so zero-row files fail too. Reads that work today still go through the same-kind and LargeList -> List arms; the only cast this now rejects is list -> string, which isn't a valid promotion. Covered in `promote_rejects_invalid_source_at_build

[laskoviymishka]

Non-struct map entries are a bad input, not an internal invariant, so I'd use invalid_data! here to match the sibling cannot promote / required nested field arms rather than Unexpected. Minor, but it keeps the kind meaningful for callers that match on it (promote_err below has the same smell).

[ewoodbury]

Makes sense, switched the map-entries arm to invalid_data! and added a case for it. I kept promote_err as Unexpected: it only fires if a batch's column doesn't match the schema the plan was built from (which is a caller bug rather than bad file data).

[laskoviymishka]

This absent-field policy (initial-default → required → null) restates the top-level Add logic in generate_transform_operations, down to map_or(!is_nullable) here versus required there — close enough to drift apart later. Not blocking, but a shared resolve_absent_field(field, target_type, constants) would keep the two in lockstep.

[laskoviymishka]

If I'm reading arrow's List/Map equality right, assert_eq!(out.as_list::<i32>(), &expected) compares through ArrayData and doesn't check the element/entries field's own name or PARQUET:field_id — which is exactly what PromotePlan installs. So a regression that kept the source element's stale id/name would still pass this test. I'd also assert on the type, where Field equality does include metadata: assert_eq!(out.data_type(), expected.data_type()), and the same at the equivalent spots in the map tests.

[ewoodbury]

Checked this one against arrow-data. ListArray equality compares the full element Field, including name and PARQUET:field_id: changing either in expected fails the existing assert.

Checked and agree on maps though, arrow skips the entries field name there; updated so both map tests now also assert out.data_type() == expected.data_type()

[laskoviymishka]

+1 to @comphead's reader-level test suggestion. Every fixture here hand-stamps field_with_id, so nothing actually pins that parquet-rs writes PARQUET:field_id onto struct children, list element, and map key/value. If any of those stamps is absent in practice, build_struct_children quietly falls back to the positional / DataInvalid path this change was written to eliminate, and all of these tests still pass green. One end-to-end test in reader/projection.rs that writes an old nested schema and reads it back through an evolved one would guard the real contract — and as @comphead noted, it can stand in for several of these unit tests. While you're in there, a sliced list/map input and a null list/map slot would pin the offset reuse too.

[ewoodbury]

Makes sense! With two suggestions, went ahead and just added that reader-level test now: test_read_evolved_nested_fields_by_field_id in reader/projection.rs: direct link

It writes a real Parquet file with an old struct, list and map<string, struct>, then reads it back through an evolved schema (reorder, added children, int to long, null list, and map slots) and compares. The test currently fails on main with Arrow Schema Error. If nested field ids go missing, it errors rather than falling back to position, since every nested struct gains children. Sliced input can't come out of a Parquet read, so promote_sliced_list_with_null_slot covers that as a unit test in fff2420. I kept the existing unit tests, since several are the cases from earlier reviews.

[ewoodbury]

Thanks @laskoviymishka! Pre-merge items are in:

1. Reader-level field-id test: test_read_evolved_nested_fields_by_field_id writes a real Parquet file with an old nested schema and reads it back through an evolved one. It fails on main with Arrow Schema Error, and here it errors rather than falling back to position if nested field ids go missing (test was suggested by @comphead and +1'd here).
2. Id-less fallback: kept on purpose, pinned with a test, and the comment points at Use schema visitor in ArrowReader name mapping #1845.

I also applied the duplicate-id check, the plan-time guard for nested sources read as a non-nested type, DataInvalid for map entries, the map data_type asserts, and a test covering sliced-lists. #3378 is filed, with a pointer in get_arrow_projection_mask_with_field_ids.

Mentioned in replies: for #3261 I'd rather close it completely in a focused follow-up than half-close it here. That PR should cover nested defaults, the non-null {} struct default from #2635, and the shared absent-field helper. So this keeps the nested initial-default as FeatureUnsupported, and I've added a release note for that to the description. Let me know if any final notes!

[laskoviymishka]

thanks for contribution!
welcome to contributors squad!