API, Core, Spark, Flink: add version-agnostic Snapshot.rootLocation() by stevenzwu · Pull Request #17523 · apache/iceberg

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

Stacked on #17545 (FileWithKeyId).

Add Snapshot.rootLocation() as the version-agnostic accessor for the file a snapshot points at, so later v4 work can address a root manifest through the same accessor that v3 uses for a manifest list. The default returns manifestListLocation(), which is now deprecated, and internal callers across core, Spark, and Flink are migrated to the new accessor. ReachableFileUtil.manifestListLocations() is likewise replaced by rootLocations().

The file itself needs no new type: FileWithKeyId already carries the location and encryption key ID for files whose key metadata lives in TableMetadata.encryptionKeys, and a v4 root manifest is such a file.

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(). Core compares that against package-private TableMetadata.UNREPORTED_FORMAT_VERSION so v3-produced snapshots on a v3→v4 upgraded table keep reporting manifest_list correctly.

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 because every caller that just wants "where is this snapshot's top-level file" would have to branch on format version. rootLocation() 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 manifest list (v3) and the root manifest (v4+) play the same structural role: they are the one file the snapshot addresses directly, and everything else is reached transitively.

AI Disclosure

Model: Cursor Grok 4.6
Platform/Tool: Cursor
Human Oversight: fully reviewed
Prompt Summary: Rebase PR 17523 onto Gabor's FileWithKeyId PR 17545, drop the extra SnapshotFile type, rename snapshotFileLocation to rootLocation, and move the unreported format-version sentinel into core.

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.

I'm wondering if we want to take this one step further and making even more general: I was examining encryption/decryption for StatisticsFile and PartitionStatistics recently, and I found that we have many specializations for the relevant EncryptiongFileIO, FileIO and EncryptionUtil functions for ManifestListFile, but in fact that contains nothing that is manifest list specific.
I think all this boils down to having the necessary fields to create either a regular Input/Output file or the encrypting/decryption versions. The code for this is general: you need a location, an encryption key ID that refers to some encryption key in TableMetadata.encryptionKeys and a mechanism to decrypt the key-metadata referred by the key id. Even this mechanism is not specific to manifest lists or V4 snapshot files.

My point is that I think whatever structure we introduce now to serve a general purpose than ManifestListFile, it should be even more general than SnapshotFile and could cover all file types that are keeping their encryption keys within TableMetadata.encryptionKeys. I'm in trouble with the naming of such a common interface, though.

Just for the record, here is the PR where I introduce the same for StatisticsFile.
If you want, I can explore the more general approach in a separate PR, but it has a direct effect on this one too.

FYI, here is the PR to introduce a more general interface here: #17545
Would you mind taking a look, @stevenzwu @anoopj @amogh-jahagirdar ?

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.

Removed in 63c4057df2. Will reintroduce with the v4 code path that actually consumes 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?

Added TestSnapshotsTable in 63c4057df2 covering both branches — a legacy snapshot (default formatVersion()) keeps manifest_list populated; an adaptive snapshot (overridden formatVersion() == 4) nulls it and populates snapshot_file.

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.

Fixed in 63c4057df2 — the check is now per-snapshot via a new default Snapshot#formatVersion() that returns the ManifestFile.LEGACY_FORMAT_VERSION sentinel. v3-produced snapshots on a v3→v4 upgraded table continue reporting the sentinel and keep manifest_list populated; only snapshots that override formatVersion() (v4+ writers, in a follow-up) trigger the adaptive branch.

stevenzwu · 2026-08-06T05:52:52Z

+   * Sentinel returned by {@link Snapshot#formatVersion()} for snapshots that do not report a format
+   * version — e.g. v3-and-earlier snapshots whose top-level file is a manifest list.
+   */
+  int LEGACY_FORMAT_VERSION = 0;


this is also added in the other PR: https://github.com/apache/iceberg/pull/16936/changes#diff-063af8055f56892097a498f7de319845da2e0d328f8241a5fc1b88a0822ac08f

we will rebase one when the other is merged first.

gaborkaszab

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, including TableStataistics and PartitionStatistics (see this for the former)

gaborkaszab · 2026-08-06T13:45:47Z

  }

+  /**
+   * @deprecated since 1.13.0; use {@link #newInputFile(SnapshotFile)}.


2 comments:

Next release is 1.12.0. Shouldn't we target that with the deprecation?

Shouldn't we also articulate when it will be dropped? Since it's in api/, 2.0.0, I guess

This goes for all deprecation in this PR, except that in core/ we can drop earlier, in 1.13.0

good point. will update to 1.12.0 for now. if this lands after 1.12.0 release, I will update it again

will add 2.0.0 removal note too

gaborkaszab · 2026-08-06T13:58:13Z

-    if (manifestList.encryptionKeyID() != null) {
-      ByteBuffer keyMetadata = manifestList.decryptKeyMetadata(em);
-      return newDecryptingInputFile(manifestList.location(), keyMetadata);
+  public InputFile newInputFile(SnapshotFile snapshotFile) {


I was poking this code for StatisticsFile encryption recently and I had the impression that these newInputFile function parameters are maybe too narrow.
The things we need here aren't really specific to manifest list or snapshot file or any other file:

What we need is the encryptionKeyId, the location and raw key-metadata.

calling snapshotFile.decryptKeyMetadata is somewhat misleading because there is nothing specific happens internally to SnapshotFile or BaseManifestListFile.

I'm wondering if we want to scratch this question within this PR, but might be the right time to introduce a more general way of implementing newInputFile(location, encryptionKeyId, key_metadata)

gaborkaszab · 2026-08-06T14:05:10Z

+   * Sentinel returned by {@link Snapshot#formatVersion()} for snapshots that do not report a format
+   * version — e.g. v3-and-earlier snapshots whose top-level file is a manifest list.
+   */
+  int LEGACY_FORMAT_VERSION = 0;


In case this PR gets merged earlier: I prefer the shorter comment as it is in the other PR:
/** Format version for pre-v4 manifest files. */

Also, shouldn't this be static final? (true for the linked PR as well)

gaborkaszab · 2026-08-06T14:29:16Z

+ * 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'm wondering if we want to take this one step further and making even more general: I was examining encryption/decryption for StatisticsFile and PartitionStatistics recently, and I found that we have many specializations for the relevant EncryptiongFileIO, FileIO and EncryptionUtil functions for ManifestListFile, but in fact that contains nothing that is manifest list specific.
I think all this boils down to having the necessary fields to create either a regular Input/Output file or the encrypting/decryption versions. The code for this is general: you need a location, an encryption key ID that refers to some encryption key in TableMetadata.encryptionKeys and a mechanism to decrypt the key-metadata referred by the key id. Even this mechanism is not specific to manifest lists or V4 snapshot files.

My point is that I think whatever structure we introduce now to serve a general purpose than ManifestListFile, it should be even more general than SnapshotFile and could cover all file types that are keeping their encryption keys within TableMetadata.encryptionKeys. I'm in trouble with the naming of such a common interface, though.

Just for the record, here is the PR where I introduce the same for StatisticsFile.
If you want, I can explore the more general approach in a separate PR, but it has a direct effect on this one too.

gaborkaszab · 2026-08-06T14:32:53Z

+    return decryptSnapshotFileKeyMetadata(manifestList.encryptionKeyID(), em);
+  }
+
+  private static ByteBuffer decryptSnapshotFileKeyMetadata(String keyId, EncryptionManager em) {


This doesn't have to be snapshot file specific. The algorithm is pretty general to all files referenced in table metadata. See here.

gaborkaszab · 2026-08-06T14:39:59Z

              "summary",
-              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())));
+              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())),
+          Types.NestedField.optional(9, "snapshot_file", Types.StringType.get()));


Do we want to change the snapshots metadata table with this PR? I think metadata tables are orthogonal and can be taken care separately. This PR is to introduce another level of abstraction, but how to visualize it in the snapshot table is a different topic IMO.

since we are updating the Snapshot interface, I thought it is better to also address the SnapshotsTable presentation together. will see if others also prefer to separate it out.

github-actions · 2026-09-07T00:28:57Z

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.

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 FileWithKeyId, a more general interface that can be used across 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. Co-authored-by: Cursor <cursoragent@cursor.com>

Add Snapshot.rootLocation() as the version-agnostic accessor for the file a snapshot points at, so later v4 work can address a root manifest through the same accessor that v3 uses for a manifest list. The default returns manifestListLocation(), which is now deprecated, and internal callers across core, Spark, and Flink are migrated to the new accessor. ReachableFileUtil.manifestListLocations() is likewise replaced by rootLocations(). The file itself needs no new type: FileWithKeyId already carries the location and encryption key ID for files whose key metadata lives in TableMetadata.encryptionKeys, and a v4 root manifest is such a file. Evolve the SnapshotsTable metadata schema to expose the location under a version-agnostic name: add an optional root_location 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(). Core compares that against TableMetadata.UNREPORTED_FORMAT_VERSION so v3-produced snapshots on a v3-to-v4 upgraded table keep reporting manifest_list correctly. Generated-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>

gaborkaszab · 2026-09-11T11:51:42Z

                      schema(),
                      specs,
-                      new BaseManifestListFile(snap.manifestListLocation(), snap.keyId()),
+                      new BaseFileWithKeyId(snap.rootLocation(), snap.keyId()),


I think I missed this in the "generic interface" PR. Already covered there, might worth another rebase with that.

gaborkaszab · 2026-09-11T11:52:54Z

+   * @return the location of the root file for this Snapshot
+   */
+  default String rootLocation() {
+    return manifestListLocation();


Not entirely sure we should default to a deprecated function. Can't we throw UOE and let the implementation override this?

gaborkaszab · 2026-09-11T11:57:10Z

-      // add the manifest list to the delete set, if present
-      if (snapshot.manifestListLocation() != null) {
-        manifestListsToDelete.add(snapshot.manifestListLocation());
+      // add the top-level snapshot file (manifest list for v3, root manifest for v4+) if present


"(manifest list for v3, root manifest for v4+)" maybe we should't spell this out everywhere. Here probably no point of this comment, enough to document what rootLocation means where it's implemented but not at the callsite.
This applies for all the other similar comments across this PR.

gaborkaszab · 2026-09-11T12:00:31Z

    }

-    List<String> manifestListLocations = Lists.newArrayList();
+    List<String> rootLocations = Lists.newArrayList();


nit: I know this is a pure rename of stuff, but this could be nice and simple with stream api

gaborkaszab · 2026-09-11T12:16:20Z


-  private static StaticDataTask.Row snapshotToRow(Snapshot snap) {
+  static StaticDataTask.Row snapshotToRow(Snapshot snap) {
+    boolean adaptive = snap.formatVersion() != TableMetadata.UNREPORTED_FORMAT_VERSION;


Snapshot.formatVersion() is always zero ATM if I'm not mistaken. This again makes me think that the snapshots metadata table part of this PR should be separate and taken care of when we actually have something v4 related to how there.

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

stevenzwu commented Aug 6, 2026

View reviewed changes

stevenzwu force-pushed the snapshot_file_extract branch from 0073109 to 63c4057 Compare August 6, 2026 06:00

gaborkaszab reviewed Aug 6, 2026

View reviewed changes

stevenzwu mentioned this pull request Aug 12, 2026

Spec: require monotonically increasing snapshot timestamp-ms in v4 #16294

Open

2 tasks

github-actions Bot added the stale label Sep 7, 2026

stevenzwu force-pushed the snapshot_file_extract branch 4 times, most recently from 81159ce to c9b805a Compare September 10, 2026 23:10

stevenzwu changed the title ~~API, Core, Spark, Flink: extract SnapshotFile abstraction~~ API, Core, Spark, Flink: add version-agnostic Snapshot.rootLocation() Sep 10, 2026

stevenzwu force-pushed the snapshot_file_extract branch 2 times, most recently from b6fda35 to 89ffa8e Compare September 11, 2026 03:41

stevenzwu force-pushed the snapshot_file_extract branch from 89ffa8e to 9b1909b Compare September 11, 2026 04:24

gaborkaszab reviewed Sep 11, 2026

View reviewed changes

github-actions Bot removed the stale label Sep 12, 2026

		* 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!

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!

gaborkaszab left a comment

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!

github-actions Bot commented Sep 7, 2026

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!

stevenzwu commented Aug 4, 2026 •

edited

Loading

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