feat: implement manifest entry metadata inheritance #178
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wgtmac
requested changes
Aug 17, 2025
HeartLinked
force-pushed
the
feat/inheritable-metadata
branch
from
254ae32 to
e93f48a
Compare
wgtmac
requested changes
Aug 19, 2025
wgtmac
reviewed
Aug 20, 2025
wgtmac
reviewed
Aug 20, 2025
wgtmac
approved these changes
Aug 20, 2025
Fokko
reviewed
Aug 25, 2025
Fokko
reviewed
Aug 25, 2025
Fokko
approved these changes
Aug 25, 2025
Contributor
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
One minor comment, but looks good 👍
- Add InheritableMetadata interface and implementations for applying metadata inheritance rules to manifest entries - Fix ManifestReader::Make methods to properly handle Result types from InheritableMetadataFactory methods - Move constructor/destructor definitions from header to implementation to avoid redefinition errors - Add missing ManifestListReaderImpl::Files() method implementation - Update method names from MakeReader to Make for consistency This ensures proper inheritance of snapshot_id, sequence_number, and file_sequence_number from manifest metadata to individual entries according to Iceberg specification.
- Add explicit keyword to ManifestReaderImpl and ManifestListReaderImpl constructors to prevent unintended implicit conversions - Update table_scan.cc to use the new Make method names instead of deprecated MakeReader methods
HeartLinked
force-pushed
the
feat/inheritable-metadata
branch
from
0fce446 to
2de3512
Compare
Contributor
Author
|
@Fokko Hello, I've addressed the latest review comments and resolved the PR conflict. The changes are now ready for merge. Thank you! |
wgtmac
approved these changes
Aug 25, 2025
Contributor
|
Thanks @HeartLinked for working on this, and thanks @wgtmac for the review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements the metadata inheritance system for manifest entries, enabling proper inheritance of
snapshot_id, sequence_number, and file_sequence_number from manifest list to manifest entries.
Changes
InheritableMetadatainterface withBaseInheritableMetadata,EmptyInheritableMetadata, andCopyInheritableMetadataimplementationsInheritableMetadataFactoryfor creating appropriate metadata inheritance instancesManifestReaderImpl::Entries()methodMakeReadertoMakefor consistencyTest Plan
close #179 .