Repository navigation
Conversation
stevenzwu
force-pushed
the
snapshot_file_extract
branch
3 times, most recently
from
August 5, 2026 04:12
51e06e3 to
ee6b97b
Compare
anoopj
reviewed
Aug 6, 2026
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
August 6, 2026 05:44
ee6b97b to
0073109
Compare
stevenzwu
commented
Aug 6, 2026
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
August 6, 2026 06:00
0073109 to
63c4057
Compare
gaborkaszab
reviewed
Aug 6, 2026
gaborkaszab
left a comment
gaborkaszab
left a comment
Contributor
There was a problem hiding this comment.
Thanks for keeping this rolling, @stevenzwu !
I have 2 comments in general:
- I think I'd not include metadata tables to this PR. That seems an orthogonal question and can be a follow-up and a different discussion how to visualize this abstraction
- For me the abstraction layer we introduce here is too specific. I describe this in multiple of my comments, but basically we could simplify encryption APIs if we introduced a more general abstraction here that goes for every file that keeps it's encrypted encryption keys in
TableMetadata.encryptionKeys, includingTableStataisticsandPartitionStatistics(see this for the former)
1 of 2 tasks
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
stevenzwu
force-pushed
the
snapshot_file_extract
branch
4 times, most recently
from
September 10, 2026 23:10
81159ce to
c9b805a
Compare
stevenzwu
changed the title
stevenzwu
force-pushed
the
snapshot_file_extract
branch
3 times, most recently
from
September 11, 2026 04:24
89ffa8e to
9b1909b
Compare
gaborkaszab
reviewed
Sep 11, 2026
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
…ManifestListFile ManifestListFile and its implementation contains nothing that is specific to manifest lists. It is more generally related to files that use TableMetadata.encryptionKeys to store encrypted encryption key metadata that are referred to by a key ID. This PR introduces a more general functionality that can be used accross multiple file types like manifest lists, V4 root manifests, table statistics and partition statistics. The less general functionality specific to manifest lists is deprecated or removed where possible.
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
October 31, 2026 04:47
9b1909b to
92275f9
Compare
stevenzwu
force-pushed
the
snapshot_file_extract
branch
3 times, most recently
from
November 2, 2026 05:35
68e2be0 to
0b626a8
Compare
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
November 2, 2026 22:21
0b626a8 to
803b4f6
Compare
stevenzwu
changed the title
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
November 2, 2026 23:30
803b4f6 to
015f87b
Compare
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
November 2, 2026 23:39
015f87b to
1284eb0
Compare
stevenzwu
changed the title
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
November 2, 2026 23:52
1284eb0 to
d1c11ad
Compare
…oot manifests v4 snapshot metadata has no format-version field and stores the snapshot's root file in root-manifest instead of manifest-list. Add Snapshot.rootManifestLocation() and keep manifestListLocation() for v1-v3 snapshots. Each snapshot sets exactly one of the two. SnapshotUtil.rootLocation() returns whichever is set, so callers that need the snapshot's root file do not branch on the field. Commits still write a manifest list. Readers that open the snapshot file as a manifest list stay on manifestListLocation() until a root manifest is written. The snapshots metadata table adds root_manifest (field ID 9), set only when the snapshot has a root manifest. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>
stevenzwu
force-pushed
the
snapshot_file_extract
branch
from
November 3, 2026 05:56
d1c11ad to
5de0a95
Compare
nastra
reviewed
Nov 3, 2026
| for (Snapshot snapshot : metadata.snapshots()) { | ||
| String newManifestListLocation = | ||
| newPath(snapshot.manifestListLocation(), sourcePrefix, targetPrefix); | ||
| String manifestList = snapshot.manifestListLocation(); |
Contributor
There was a problem hiding this comment.
do we also need to handle ReachableFileCleanup and IncrementalFileCleanup?
nastra
reviewed
Nov 3, 2026
| TableMetadata metadata, String sourcePrefix, String targetPrefix) { | ||
| List<Snapshot> newSnapshots = Lists.newArrayListWithCapacity(metadata.snapshots().size()); | ||
| for (Snapshot snapshot : metadata.snapshots()) { | ||
| String newManifestListLocation = |
Contributor
There was a problem hiding this comment.
there are a few more places in this class that reference snapshot.manifestListLocation() directly, should we update those as well?
This branch has not been deployed
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.
Stacked on #17545 (
FileWithEncryptedKey).v4 snapshots store a
root-manifestinstead of amanifest-list, and v4 has no format-version field, so each snapshot records which file it has. This addsSnapshot.rootManifestLocation()(default null) next tomanifestListLocation(), which is unchanged for v1–v3. A snapshot sets at most one of the two.SnapshotParserreads and writes whichever is present, and thesnapshotstable adds aroot_manifestcolumn (field ID 9).V4 commits still write a manifest list, and readers that open the file as a manifest list stay on
manifestListLocation()until a v4 root-manifest write path is ready.