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 / large-list / 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 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)
• 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's Arrow schema is converted to the table's List before its elements are reconciled, as main's cast did

Are these changes tested?

Yes. The #2647 tests, plus process_record_batch coverage for 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.

Out of scope

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).

[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]

This guard is right for structs whose types differ, but files without field ids that previously passed through positionally still need to work once more columns reach PromotePlan::build (see the comment at line 1108). A map from an id-less file is the case that breaks: the Map arm calls build_struct_children directly, and key/value carry no ids because the reader assigns fallback ids to top-level fields only. With the line 1108 change alone, reading an id-less map<string, int> column fails with DataInvalid => cannot reconcile struct fields by id: no source field carries a field id. On this head it passes.

Could we keep positional matching when there are no ids and the child types still line up? Doing it here covers the struct and map arms in one place, and recursing through build handles a struct nested in an id-less struct:

Suggested change

	if !source_fields.is_empty() && source_by_id.is_empty() {
	return Err(Error::new(
	ErrorKind::DataInvalid,
	"cannot reconcile struct fields by id: no source field carries a field id",
	));
	}
	// Reachable for id-less files with nested types because the reader only
	// applies name mapping to top-level fields. Keep positional matching when
	// the child types still line up, and error otherwise rather than silently
	// nulling every child.
	if !source_fields.is_empty() && source_by_id.is_empty() {
	if source_fields.len() == target_fields.len()
	&& source_fields
	.iter()
	.zip(target_fields.iter())
	.all(|(s, t)| s.data_type().equals_datatype(t.data_type()))
	{
	return source_fields
	.iter()
	.zip(target_fields.iter())
	.enumerate()
	.map(|(source_index, (source_field, target_field))| {
	Ok(ChildPlan::FromSource {
	source_index,
	plan: Self::build(
	source_field.data_type(),
	target_field.data_type(),
	snapshot_schema,
	)?,
	})
	})
	.collect();
	}
	return Err(Error::new(
	ErrorKind::DataInvalid,
	"cannot reconcile struct fields by id: no source field carries a field id",
	));
	}

With this and the line 1108 change, tests for an id-less map<string, int> and an id-less struct<inner: struct<x: int>> both pass through process_record_batch, and so does test_read_parquet_without_field_ids_with_struct in projection.rs.

[ewoodbury]

Yes, makes sense - Done in same commit, inside build_struct_children, so struct and map share it. Types line up by position, then we recurse through build. promote_idless_map_keeps_data checks both keys and values. promote_idless_nested_struct_keeps_data is the struct-in-struct case. Same-type id-less reorder still follows file order, which matches the old pass-through, and it's not distinguishable without ids. (Recursive name mapping would be the real fix for that)

[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!

[mbutrovich]

Schema Evolution allows reordering and renaming inside any struct, not just the top-level schema, and Column Projection requires projecting by id because "the table schema's column names and order may change after a data file is written".

DataType::equals_datatype compares struct children by position, type, and nullability. It ignores child names and metadata, including PARQUET:field_id. So when a nested struct's children are reordered or renamed but the types still line up, this check returns true and the column takes ColumnSource::PassThrough without reaching PromotePlan. What happens when the table has s: struct<b: int (id 6), a: int (id 5)> and the file has s: struct<a (id 5), b (id 6)> with a = [1, 2] and b = [100, 200]? On this head, process_record_batch returns a batch whose schema says position 0 is b, but the array there holds [1, 2]. The struct array also keeps the file's child order, so its data type no longer matches the batch schema. A rename with no type change fails the same way: the array keeps the old child name. promote_evolved_nested_struct_via_process_record_batch only gets the rename right because the int-to-long promotion forces the Promote path.

Could we let the plan decide when to pass through?

Suggested change

	if source_field.data_type().equals_datatype(target_type) {
	ColumnSource::PassThrough {
	source_index: *source_index,
	}
	} else {
	ColumnSource::Promote {
	plan: PromotePlan::build(
	source_field.data_type(),
	target_type,
	snapshot_schema,
	)?,
	source_index: *source_index,
	}
	}
	match PromotePlan::build(
	source_field.data_type(),
	target_type,
	snapshot_schema,
	)? {
	PromotePlan::PassThrough => ColumnSource::PassThrough {
	source_index: *source_index,
	},
	plan => ColumnSource::Promote {
	plan,
	source_index: *source_index,
	},
	}

This needs the change at lines 320-325, or id-less maps start failing. With both, reorder and rename tests pass, and all 1773 tests in cargo test -p iceberg --lib pass. For a struct whose ids match but whose Fields aren't strictly equal (a doc metadata entry, for example), the per-batch cost is one StructArray rebuild from Arc clones.

[ewoodbury]

Yeah - thanks for the explanation, and done in 850c785. A present column always goes through PromotePlan::build, and passes through only when the plan returns PassThrough. Reorder and same-type rename are covered by promote_struct_reorders_children_by_id_via_process_record_batch and promote_struct_renames_child_via_process_record_batch. Drop-and-re-add of the same name is promote_struct_dropped_and_readded_same_name_nulls_by_id: old id 5 values come back null under id 6.

[mbutrovich]

#2668 replaced this helper with the module-level field_with_id(name, data_type, nullable, field_id: i32), which this test module already imports at line 1418. Could the new tests use field_with_id and drop simple_field, so there's one helper for field-id metadata again?

[ewoodbury]

Yep, makes sense, replace with field_with_id in the tests now

[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.

[mbutrovich]

The spec doesn't define positional matching for nested fields. Column Projection resolves a field id missing from a file through name mapping, and a name mapping carries fields for struct children, map keys and values, and list elements. The spec-correct fix for id-less nested structs is recursive name mapping, which #1845 tracks. Until that lands, the positional path here is compatibility behavior, and it should keep what main does today rather than fail reads that work now.

The equals_datatype check came from my suggestion last round, and it fails a read that works on main. Take a file with no field ids where s was written as struct<x: int>, read through a table schema where x is now long. On main that column takes the positional cast and comes back as struct<x: long>. On this head, process_record_batch fails with DataInvalid => cannot reconcile struct fields by id: source fields do not all have field ids.

Could we keep the child-count check and let build decide each pair? Primitive pairs then go through Cast as they do on main, and a struct or list paired with an incompatible type still errors in build:

Suggested change

	// Name mapping only assigns top-level ids. Fully id-less children match by
	// position when types line up. A same-type reorder cannot be detected without
	// ids and stays positional. Any missing id errors instead of nulling that child.
	if !source_fields.is_empty() && source_by_id.len() != source_fields.len() {
	if source_by_id.is_empty()
	&& source_fields.len() == target_fields.len()
	&& source_fields
	.iter()
	.zip(target_fields.iter())
	.all(|(s, t)| s.data_type().equals_datatype(t.data_type()))
	{
	// Name mapping only assigns top-level ids (#1845). Until it recurses, fully
	// id-less children match by position when the child counts line up, as they
	// did before, and `build` checks each pair. A same-type reorder cannot be
	// detected without ids. Any missing id errors instead of nulling that child.
	if !source_fields.is_empty() && source_by_id.len() != source_fields.len() {
	if source_by_id.is_empty() && source_fields.len() == target_fields.len() {

With this change, the int to long case returns struct<x: long> with the original values, and the full cargo test -p iceberg --lib run still passes. Please add a test next to promote_idless_nested_struct_keeps_data that drives an id-less struct<x: int> through transform_top_level against a long target. In the PR description, please update the bullet about id-less structs whose types don't line up and point the recursive name mapping item under Out of scope at #1845.

[ewoodbury]

Done in 749e98b- The id-less path requires the same child count, and build checks each pair, so an id-less struct<x: int> promotes to long. promote_idless_struct_promotes_child_int_to_long sets that through transform_top_level. A different count still errors (same for ids on only some children).

The description now says that, and the out-of-scope name-mapping line points at #1845

[mbutrovich]

Could this build the expected StructArray and compare it whole? The bug this test covers was an array whose child order and names didn't match the schema, and a whole-array comparison also checks child names, nullability, and the PARQUET:field_id metadata. The same applies to the other new tests that check names and values one at a time. I ran this version on the head commit and it passes:

Suggested change

	let s = out.as_struct();
	assert_eq!(s.fields()[0].name(), "b");
	assert_eq!(s.fields()[1].name(), "a");
	assert_eq!(s.column(0).as_primitive::<Int32Type>().values(), &[
	100, 200
	]);
	assert_eq!(s.column(1).as_primitive::<Int32Type>().values(), &[1, 2]);
	let expected = StructArray::new(
	Fields::from(vec![
	field_with_id("b", DataType::Int32, true, 6),
	field_with_id("a", DataType::Int32, true, 5),
	]),
	vec![
	Arc::new(Int32Array::from(vec![100, 200])) as ArrayRef,
	Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
	],
	None,
	);
	assert_eq!(out.as_struct(), &expected);

[ewoodbury]

Makes sense- updated the reorder test to use this StructArray. Updated tests for rename, id-less nested struct, and evolved-struct to compare the whole struct too. (After fixing the merge conflict, I also updated the new drop-and-re-add and the id-less map to be consistent in a56ed6d)

[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.