API, Core, Spark, Flink: extract SnapshotFile abstraction by stevenzwu · Pull Request #17523 · apache/iceberg

stevenzwu · 2026-08-04T22:00:47Z

Introduce SnapshotFile as the version-agnostic name for the top-level file a snapshot points at, so later v4 work can address a root manifest through the same type that v3 uses for a manifest list. Snapshot.snapshotFileLocation() defaults to manifestListLocation(), which is now deprecated; FileIO, EncryptingFileIO, EncryptionUtil, and ReachableFileUtil grow SnapshotFile overloads with the manifest-list variants deprecated as delegating wrappers.

Evolves the SnapshotsTable metadata schema to expose the location under a version-agnostic name: adds an optional snapshot_file column (field ID 9) populated for every snapshot. For tables at v3 and earlier, manifest_list is still populated with the same value so existing consumers are unaffected; for tables at v4+, manifest_list is null, dispatched via the new package-private TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE constant.

Alternative considered: a separate `Snapshot#rootManifestLocation()`

A more literal encoding would keep manifestListLocation() for v3 and add a sibling rootManifestLocation() for v4+. That was rejected for a few reasons:

Every caller that just wants "where is this snapshot's top-level file" would have to branch on format version and call one accessor or the other. snapshotFileLocation() collapses that decision into the interface — v3 snapshots return their manifest list, v4+ snapshots return their root manifest, and callers stop caring about the difference.
The abstraction has to extend past Snapshot — FileIO.newInputFile(...), EncryptingFileIO, and EncryptionUtil.decryptSnapshotFileKeyMetadata(...) all need to accept "the file this snapshot points at" without knowing the format version. A single SnapshotFile type lets those signatures stay uniform; twin ManifestListFile/RootManifestFile types would fork every such API.
The manifest list (v3) and the root manifest (v4+) play the same structural role in a snapshot — they are the one file the snapshot addresses directly, and everything else is reached transitively. A single name reflects that shared role and keeps Snapshot from carrying two mutually-exclusive accessors long-term.

amogh-jahagirdar · 2026-08-05T20:07:28Z

  }

+  @Override
+  public String snapshotFileLocation() {


Maybe we should call this rootLocation? It feels weird to have to do snapshot.snapshotFileLocation() because the method name sounds duplicative in the context of already being within a snapshot. and I think rootLocation also generalizes to v4+ and older format version as the manifest list can still be considered a root of the metadata tree.

amogh-jahagirdar · 2026-08-05T20:59:10Z

+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a mix of data-file entries


I guess I'm not following, why do we need this interface? Regardless of V4 or earlier we have a root file but we just need the location and all the other things like encryption key etc should all just work as they do today.

Ah okay, for decryption we have a bunch of code that takes in ManifestListFile

I think same principle as my other naming comment, maybe we want to just call this RootFile? It doesn't attach this interface too hard to the "Snapshot" though of course for the forsseable future Snapshot will always have a pointer to a single root. Not as opinionated on this one though

I prefer something like ContentRoot - this is self explanatory and also applies to previous manifest list files also.

amogh-jahagirdar · 2026-08-05T21:02:08Z

+   *
+   * @return the location of the snapshot file for this Snapshot
+   */
+  default String snapshotFileLocation() {


same as my other comment, I feel like we should just call it rootLocation? It generalizes to both v3 and v4 imo and feels less awkawrd than snapshot.snapshotFileLocation()?

anoopj · 2026-08-06T00:42:57Z

+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a mix of data-file entries


I prefer something like ContentRoot - this is self explanatory and also applies to previous manifest list files also.

anoopj · 2026-08-06T00:45:30Z

+import org.apache.iceberg.encryption.EncryptionManager;
+import org.apache.iceberg.encryption.EncryptionUtil;
+
+class BaseSnapshotFile implements SnapshotFile, Serializable {


This is not used anywhere yet?

yeah. this will be used in the v4 code path, which is not integrated yet. I can remove this for now and introduce it when we actually use it.

anoopj · 2026-08-06T00:49:26Z

        snap.operation(),
-        snap.manifestListLocation(),
-        snap.summary());
+        adaptive ? null : location,


We might be missing tests for the adaptive being true branch?

anoopj · 2026-08-06T00:51:27Z


-  private static StaticDataTask.Row snapshotToRow(Snapshot snap) {
+  private static StaticDataTask.Row snapshotToRow(Snapshot snap, int formatVersion) {
+    boolean adaptive = formatVersion >= TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE;


A tree upgraded from v3 to v4 would have adaptive true at the table level, but there could be snapshots that are v3 produced snapshots. How do we handle it correctly? It seems a bit tricky.

good catch. this is a bug. I have some change locally that is not extracted properly in this PR. I intended to add this new API to the Snapshot interface.

default int formatVersion() { return ManifestFile.LEGACY_FORMAT_VERSION; }

I should have checked snap.formatVersion() != ManifestFile.LEGACY_FORMAT_VERSION. I should also add a test coverage for this scenario.

Introduce SnapshotFile as the version-agnostic name for the top-level file a snapshot points at, so later v4 work can address a root manifest through the same type that v3 uses for a manifest list. Snapshot.snapshotFileLocation() defaults to manifestListLocation(), which is now deprecated; FileIO, EncryptingFileIO, EncryptionUtil, and ReachableFileUtil grow SnapshotFile overloads with the manifest-list variants deprecated as delegating wrappers. Evolve the SnapshotsTable metadata schema to expose the location under a version-agnostic name: add an optional snapshot_file column (field ID 9) populated for every snapshot. Whether the row also populates manifest_list is dispatched per-snapshot via a new default Snapshot.formatVersion() (returning the new ManifestFile.LEGACY_FORMAT_VERSION sentinel); v3-and-earlier snapshots retain their manifest_list value, and adaptive v4+ snapshots — which report a real format version — set it to null. Per-snapshot dispatch is required so v3-produced snapshots on a v3->v4 upgraded table keep reporting manifest_list correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions Bot added API spark core flink labels Aug 4, 2026

stevenzwu force-pushed the snapshot_file_extract branch 3 times, most recently from 51e06e3 to ee6b97b Compare August 5, 2026 04:12

amogh-jahagirdar reviewed Aug 5, 2026

View reviewed changes

anoopj reviewed Aug 6, 2026

View reviewed changes

stevenzwu force-pushed the snapshot_file_extract branch from ee6b97b to 0073109 Compare August 6, 2026 05:44

Navigation Menu

Uh oh!

API, Core, Spark, Flink: extract SnapshotFile abstraction - #17523

API, Core, Spark, Flink: extract SnapshotFile abstraction#17523
stevenzwu wants to merge 1 commit into
apache:mainfrom
stevenzwu:snapshot_file_extract

stevenzwu commented Aug 4, 2026 •

edited

Loading

Uh oh!

amogh-jahagirdar Aug 5, 2026

Uh oh!

amogh-jahagirdar Aug 5, 2026

Uh oh!

amogh-jahagirdar Aug 5, 2026

Uh oh!

amogh-jahagirdar Aug 5, 2026

Uh oh!

anoopj Aug 6, 2026

Uh oh!

amogh-jahagirdar Aug 5, 2026

Uh oh!

anoopj Aug 6, 2026

Uh oh!

anoopj Aug 6, 2026

Uh oh!

stevenzwu Aug 6, 2026

Uh oh!

anoopj Aug 6, 2026

Uh oh!

anoopj Aug 6, 2026

Uh oh!

stevenzwu Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		* The top-level file that a {@link Snapshot} points at. For v3 and earlier this is a manifest list
		* (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a mix of data-file entries

Uh oh!

Conversation

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

Uh oh!

Alternative considered: a separate Snapshot#rootManifestLocation()

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevenzwu commented Aug 4, 2026 •

edited

Loading

Alternative considered: a separate `Snapshot#rootManifestLocation()`