fix(manifest): preserve snapshot ID inheritance semantics - #947
Merged
Merged
Conversation
Represent ManifestFile::added_snapshot_id as optional to distinguish an inheritance-ready null ID from a literal -1. Reuse manifests only when the ID is null, rewrite explicit -1 manifests with the commit snapshot ID, and continue requiring the field when writing manifest lists.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review comments remain, and targeted test coverage is included.
Review effort: Lite
Findings: None
What changed in this PR
Preserves the distinction between unassigned (null) and explicit -1 manifest snapshot IDs.
Changes:
- Uses
std::optional<int64_t>for manifest snapshot IDs. - Rewrites explicit
-1manifests while reusing only null-ID manifests. - Updates consumers, serialization, scanning, cleanup, and test coverage.
| File | Summary |
|---|---|
src/iceberg/update/snapshot_update.cc |
Updates commit-time metadata handling. |
src/iceberg/update/merging_snapshot_update.cc |
Applies inheritance-aware append behavior. |
src/iceberg/update/merge_append.cc |
Handles null and explicit -1 IDs. |
src/iceberg/update/fast_append.cc |
Rewrites explicit -1 manifests. |
src/iceberg/update/expire_snapshots.cc |
Validates optional IDs during cleanup. |
src/iceberg/test/merging_snapshot_update_test.cc |
Updates inheritance fixtures. |
src/iceberg/test/merge_append_test.cc |
Tests inheritance and explicit -1 copying. |
src/iceberg/test/manifest_list_versions_test.cc |
Tests rejection of unassigned IDs. |
src/iceberg/test/fast_append_test.cc |
Tests inheritance and copying behavior. |
src/iceberg/table_scan.cc |
Handles optional IDs during scans. |
src/iceberg/row/manifest_wrapper.cc |
Exposes optional snapshot IDs. |
src/iceberg/manifest/v3_metadata.cc |
Handles optional IDs for V3 metadata. |
src/iceberg/manifest/v2_metadata.cc |
Handles optional IDs for V2 metadata. |
src/iceberg/manifest/manifest_writer.h |
Documents nullable ID semantics. |
src/iceberg/manifest/manifest_writer.cc |
Preserves null IDs. |
src/iceberg/manifest/manifest_merge_manager.cc |
Updates merge and replacement accounting. |
src/iceberg/manifest/manifest_list.h |
Makes snapshot IDs optional. |
src/iceberg/manifest/manifest_adapter.cc |
Rejects null IDs during serialization. |
src/iceberg/inheritable_metadata.cc |
Validates assigned IDs for reading. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zhjwpku
approved these changes
Sep 20, 2026
zhjwpku
left a comment
zhjwpku
left a comment
Collaborator
There was a problem hiding this comment.
I checked the Java FastAppend impl [0]. It reuses the original manifest only when canInheritSnapshotId() && snapshotId() == null. For an explicit -1, it rewrites the manifest. These changes align the C++ behavior with Java, thanks.
+1
manuzhang
approved these changes
Sep 20, 2026
blackmwk
approved these changes
Sep 20, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Purpose
C++ converted an unassigned manifest snapshot ID from
nullto-1.This made inheritance-ready manifests indistinguishable from manifests
containing a literal
-1.As a result,
FastAppendandMergeAppendcould directly reuse a manifestwith explicit
-1entry snapshot IDs instead of rewriting those entrieswith the new commit snapshot ID.
Changes
ManifestFile::added_snapshot_idtostd::optional<int64_t>.nullfromManifestWriter::ToManifestFile.added_snapshot_idis null.-1and assign the commit snapshot ID.added_snapshot_idwhen writing manifest lists.-1.