Core: Add inheritance to V4ManifestReader by rdblue · Pull Request #18171 · apache/iceberg

rdblue · 2026-09-19T00:24:17Z

This implements inheritance in V4ManifestReader. This does not handle first_row_id assignment, which will be in a separate PR.

v3 and earlier user InheritableMetadataFactory to set up inheritance that sets spec ID, snapshot ID, sequence number, and manifest location. This PR ensures the same fields are set:

Spec ID is written with v4 data files, so it is out of scope
Snapshot ID is inherited using TrackingStruct#inherit(long, long)
Sequence number is also inherited by calling TrackingStruct#inherit(long, long)
Manifest location is set by projecting _file, similar to how _pos is projected through readers

This removes inheritFrom(Tracking) and setManifestLocation. The first inheritance method didn't accept ManifestFile and is now simpler. It doesn't need to check manifest tracking fields consistency. setManifestLocation is not needed by using readers to set the _file field.

Test plan:

Inheritance behavior is tested in TestTrackingStruct. This is expanded to more cases using parameterized tests, not just specific statuses
TestV4ManifestReader verifies simple inheritance cases for each field, but full behavior tests are in TestTrackingStruct.

amogh-jahagirdar · 2026-09-21T13:56:46Z

+  // inheritance test plan:
+  // - snapshot ID inheritance, not dv snapshot ID inheritance
+  // - file seq and data seq inheritance from null WITH ADDED
+  // - file seq and data seq not inherited with EXISTING, MODIFIED, DELETE (SHOULD FAIL?)


For the column update case (and only specifically that case I think), we would want to inherhit data seq. number for the MODIFIED case. We don't have that write path anyways so I'm assuming this comment was just made in the context of this initial inheritance implementation, but just mentioning in case.

I didn't mean to leave this in and I'll remove it. I just wanted to run the full test suite over the weekend.

amogh-jahagirdar

Looks great to me overall, just had some questions/minor comments

amogh-jahagirdar · 2026-09-22T16:29:40Z

-        this.snapshotId = manifestTracking.snapshotId();
-      }
-
-      // manifests do not distinguish between data and file sequence numbers


Curious, why did we drop this check? I think the comment could be dropped but I do think we have the requirement that writers set the same data sequence and file sequence number in V4 for leaf manifests.

The original version relies on a v4 Tracking for the manifest that is being read, but the reader actually has a ManifestFile instance because we want to keep the union type (TrackedFile) limited in the codebase. Things shouldn't accept TrackedFile and validate it when they can take the more specific interface instead.

Using ManifestFile means that we only have one sequence number, so we can't check here.

In addition, the inheritance implementation isn't really the right place to check metadata consistency, if we want to check this at all.

Things shouldn't accept TrackedFile and validate it when they can take the more specific interface instead.

Agree with this, if we use the more specific type, the issue is moot. I missed that we're doing this now.

amogh-jahagirdar · 2026-09-22T16:33:41Z

-        if (dataSequenceNumber == null) {
-          this.dataSequenceNumber = manifestTracking.fileSequenceNumber();
-        }
+    boolean isAdded = status == EntryStatus.ADDED;


I know this is more forward looking than we need right now, but I'm curious how do we envision handling the case when the status is MODIFIED and there's a column update so we need to inherit there as well?

To identify if there was a column change we would pass in the latest_column_update_snapshot_id, and if it's the same as the current snapshot ID (which I think we'd also need to pass in because that can be different than manifestSnapshotId), then we know that this was a column update and the data seq/file seq inheritance should be applied.

I'd like to keep inheritance as simple as possible. I think we would only need to update this so that a null data sequence number is inherited when the state is MODIFIED (and not file sequence number).

It sounds like you're thinking about how to detect when we should override the sequence number by checking the last_column_update_snapshot_id? I don't think I follow the logic of using the current snapshot ID anywhere. Inheritance must happen no matter what the current table state is. We could check against the manifest's snapshot_id because the manifest with an updated entry must be written at the same time. But to me, this is over-complicated compared to expecting the writer to set the data_sequence_number to null in the MODIFIED entry when it needs to be inherited.

Yeah you're right that file sequence number should not be inherited for that case. "Current" may have been a poor choice of words but I just meant the snapshot that's being read at that time, not neccessarily the latest main state. But I think ultimately you're right that we neccessarily need to be writing out a new manifest if we're doing a column update anyways so the manifest snapshot ID may be used. Personally, I'd prefer to take that complexity over having the expectation that writers set it to be null but at the same time it's in the writers own benefit to set it null anyways (that's arguably an easier thing for them to do). Anyways don't want to get ahead of ourselves, that's a later problem, for now this makes sense to me!

since we have the MODIFIED enum value and latest_column_file_snapshot_id field available in Tracking, should we just add the check now for the dataSequenceNumber at line 126 below?

@stevenzwu, I think we should do that as part of adding column files. Right now, we don't have any cases where this should be inherited for MODIFIED status.

I'd prefer to take that complexity over having the expectation that writers set it to be null

Another result of the snapshot ID check to override is that the writer wouldn't have control over inheritance. For example, what if a writer doesn't modify data and instead combines column overlays? In that case, a writer that isn't changing modifying rows should be able to preserve the sequence number if it chooses to but the snapshot ID rule would override. On the other hand, does it matter if it isn't being inherited to the row level?

Overall, I think it's best to rely on a consistent and simple rule that a writer uses null to set the data sequence number to the current commit's sequence number when it is assigned.

stevenzwu

LGTM.

Regarding the modified entry for column files, I don't know if we want to implement it here or follow up later.

rdblue · 2026-09-22T21:25:10Z

Thanks for the reviews, @amogh-jahagirdar and @stevenzwu!

Core: Add inheritance to V4ManifestReader.

ca0fafb

github-actions Bot added parquet core labels Sep 19, 2026

amogh-jahagirdar reviewed Sep 21, 2026

View reviewed changes

rdblue marked this pull request as draft September 21, 2026 16:28

amogh-jahagirdar reviewed Sep 22, 2026

View reviewed changes

Fix review feedback and checkstyle.

a70bcd9

rdblue marked this pull request as ready for review September 22, 2026 19:27

amogh-jahagirdar approved these changes Sep 22, 2026

View reviewed changes

stevenzwu approved these changes Sep 22, 2026

View reviewed changes

rdblue merged commit 17cf4b4 into apache:main Sep 22, 2026
42 of 43 checks passed

rdblue mentioned this pull request Sep 23, 2026

Core: Implement v4 first row ID assignment #18225

Merged

Conversation

rdblue commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzwu Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdblue commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rdblue commented Sep 19, 2026 •

edited

Loading

amogh-jahagirdar Sep 22, 2026 •

edited

Loading

stevenzwu Sep 22, 2026 •

edited

Loading