Skip to content

AddFiles: regenerate name mapping, read footers once, error helper - #39836

Open
claudevdm wants to merge 5 commits into
apache:masterfrom
claudevdm:iceberg-prepass-schema-v2
Open

AddFiles: regenerate name mapping, read footers once, error helper#39836
claudevdm wants to merge 5 commits into
apache:masterfrom
claudevdm:iceberg-prepass-schema-v2

Conversation

@claudevdm

@claudevdm claudevdm commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

AddFiles: heal stale or malformed name mappings at commit time

Files registered by AddFiles carry no Iceberg field ids, so readers resolve every column through the schema.name-mapping.default table property. A column missing from that mapping reads as null in every registered file. Before this change the commit stage only created the mapping when the property was absent. A mapping that was present but broken was trusted as is, and a malformed one threw inside the commit stage and blocked every manifest commit for the table.

Changes

Before each manifest commit, CommitManifestFilesDoFn now parses the stored mapping, checks that it covers the current schema, and regenerates it when it does not. Regeneration preserves custom names from the old mapping where it safely can. Logic lives in the new NameMappingUtils class.

Cases handled

Absent mapping. Generated from the schema, same as before.

Malformed mapping. Unparseable JSON, a name used by two fields at the same level, or a duplicate field id. Treated as absent and regenerated with a warning. Previously this threw on every commit, permanently.

Stale mapping. The schema gained a column but the mapping was never updated, typically a hand written mapping from the original file import.

schema:  id (1), name (2), age (3)
mapping: [{"field-id": 1, "names": ["id", "ident"]}]
result:  [{"field-id": 1, "names": ["id", "ident"]},
          {"field-id": 2, "names": ["name"]},
          {"field-id": 3, "names": ["age"]}]

The custom alias ident survives.

Stale nested mapping. The coverage check walks into structs, lists and maps using the same element, key and value path segments Iceberg uses. A mapping that resolves events and events.element.a but is missing events.element.b is detected and regenerated. A top level only check would have passed it.

Null id entry. {"names": ["user_id"]} with no field id means the column maps to nothing and reads as null. Counted as not covered.

Wrong id entry. user_id mapped to id 3 while the schema says 7. Readers would bind the column to the wrong field. Counted as not covered.

Healthy mapping with custom names. Left byte for byte untouched. No property commit happens.

Pre-rename file names. A column renamed from old_name to new_name keeps the old name as an alias, so files written before the rename still resolve:

[{"field-id": 1, "names": ["new_name", "old_name"]}]

Name collisions during regeneration. Schema names always win. If the old mapping carried amt as an alias of amount but the schema now has a real amt column, the alias is dropped with a warning naming the field path. If two carried aliases collide with each other at one level, the first in schema column order wins.

Dropped columns. Old entries whose id is no longer in the schema, including null id tombstones, are not carried over. Their file columns stay unmapped, which readers treat the same way.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@claudevdm
claudevdm requested a review from ahmedabu98 August 21, 2026 15:40
@claudevdm
claudevdm marked this pull request as ready for review August 21, 2026 15:40

@ahmedabu98 ahmedabu98 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits but LGTM! Thanks for writing this. Remember to trigger Iceberg ITs

Feel free to merge after addressing comments and tests pass

NameMapping mapping = MappingUtil.create(table.schema());
String mappingJson = NameMappingParser.toJson(mapping);
table.updateProperties().set(TableProperties.DEFAULT_NAME_MAPPING, mappingJson).commit();
// Forces name-based resolution: zero-copy files carry no field ids, so any schema column

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Forces name-based resolution: zero-copy files carry no field ids, so any schema column
// Forces name-based resolution: zero-copy files typically don't carry field ids, so any schema column

nit: Parquet files written with Iceberg will have field ids, so this doesn't apply for use cases registering files written by another Iceberg catalog

return NameMappingParser.fromJson(mappingJson);
} catch (RuntimeException e) {
LOG.warn(
"Malformed {} property; it will be regenerated from the schema: {}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also log the malformed mappingJson ?

assertNotNull(mapping.find("name"));
assertNotNull(mapping.find("age"));
// The user's alias survived the regeneration.
assertNotNull(mapping.find("ident"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include assertNotNull(mapping.find("id")); ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also for all these assertions, would it be possible to generate an "expected" cherNameMapping object (i think the general case would just be creating it from the schema) and do a simple equality check?

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @chamikaramj for label java.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants