Skip to content

Core: Add inheritance to V4ManifestReader - #18171

Draft
rdblue wants to merge 1 commit into
apache:mainfrom
rdblue:v4-manifest-reader-inheritance
Draft

rdblue wants to merge 1 commit into
apache:mainfrom
rdblue:v4-manifest-reader-inheritance

Conversation

@rdblue

@rdblue rdblue commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

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.

// 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?)

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@rdblue
rdblue marked this pull request as draft September 21, 2026 16:28
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