API, Core: Add Snapshot.rootManifestLocation() by stevenzwu · Pull Request #17523 · apache/iceberg

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

v4 snapshots store a root-manifest instead of a manifest-list, so Snapshot needs to expose that location. This adds Snapshot.rootManifestLocation() defaulting to null, parsed and written as root-manifest. manifestListLocation is unchanged for v1-v3, and at most one of the two is set. Commits still write a manifest list and readers stay on manifestListLocation until a root manifest write path exists. The snapshots metadata table column and the encryption abstraction were removed from this PR.

Test plan: TestSnapshotJson and TestMetadataUpdateParser round-trips for root-manifest.

AI assisted.

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)

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.

github-actions · 2026-10-12T00:32:53Z

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.

github-actions · 2026-10-20T00:25:29Z

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.

nastra · 2026-11-03T16:49:23Z

    for (Snapshot snapshot : metadata.snapshots()) {
-      String newManifestListLocation =
-          newPath(snapshot.manifestListLocation(), sourcePrefix, targetPrefix);
+      String manifestList = snapshot.manifestListLocation();


do we also need to handle ReachableFileCleanup and IncrementalFileCleanup?

nastra · 2026-11-03T16:51:54Z

      TableMetadata metadata, String sourcePrefix, String targetPrefix) {
    List<Snapshot> newSnapshots = Lists.newArrayListWithCapacity(metadata.snapshots().size());
    for (Snapshot snapshot : metadata.snapshots()) {
-      String newManifestListLocation =


there are a few more places in this class that reference snapshot.manifestListLocation() directly, should we update those as well?

gaborkaszab · 2026-11-05T09:04:49Z

I haven't been looking into this for a long time, but do we still need to make #17545 a dependency of this one? As I see we made the RootManifestFile dependent on ManifestFile that goes against the intent of the "generic common parent" of V3 manifest list file and V4 root manifest file.
Also by the title this PR seems to focus on a common root location that seems a narrower problem.

gaborkaszab

Sorry I wasn't active on this PR for a while. I think the scope has change in the meantime, got narrowed to simply introducing the new root-manifest field. Left some comments with this in mind.

Also, just a thought on the direct follow-ups: There are a handful of new fields introduced for V4 snapshots. This PR introduces a single one of them. Shouldn't we do them in bulk by a single PR?

gaborkaszab · 2026-11-05T09:12:43Z

+        keyId);
+  }
+
+  BaseSnapshot(


Aren't these constructors asymmetric? We have now a variation that accepts manifest list only, and another one that accepts both manifest list and root manifest. To restore symmetry, we either need a third one accepting root manifest only, or simply having the one that accepts both without the other 2 variations. I would personally go for the second option.

went with your recommendation and modify the existing constructor directly.

gaborkaszab · 2026-11-05T09:24:06Z

-          Iterables.transform(snapshot.allManifests(DUMMY_FILE_IO), ManifestFile::path),
-          generator);
+      String rootManifest = snapshot.rootManifestLocation();
+      if (rootManifest != null) {


This is an "if - else { if - else}" structure. I think we can use a cleaner "if - else if - else" non-nested structure by adding String rootManifest = snapshot.rootManifestLocation() above the if.

Done. toJson now reads both locations up front and uses a flat if / else if / else.

gaborkaszab · 2026-11-05T09:26:48Z

+      String rootManifest = snapshot.rootManifestLocation();
+      if (rootManifest != null) {
+        generator.writeStringField(ROOT_MANIFEST, rootManifest);
+      } else {


I think the order of the checks is off. Previously we did "if not V2 then V1". Shouldn't we check V3 root manifest first, then V2 manifest list, then V1?

Agreed. The order is now root-manifest first, then manifest-list, then the embedded v1 manifests.

gaborkaszab · 2026-11-05T09:29:12Z

+
+    if (manifestList != null || rootManifest != null) {
+      // manifest-list is used by v1-v3 snapshots, root-manifest by v4 snapshots
      return new BaseSnapshot(


This seems another reason why we should drop the manifest list only constructor.

Yes, with the list-only constructor gone, fromJson now has a single call that passes both manifestList and rootManifest. The v1 embedded-manifests constructor is still separate.

gaborkaszab · 2026-11-05T09:31:52Z

              6,
              "summary",
-              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())));
+              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())),


I recall @rdblue is in favor of not implementing metadata table support right away as we might want to re-consider how they should look like in V4.
I know I've commented before not to add coverage for metadata table in this PR, and I still think this way.

Agreed. I removed the snapshots metadata table changes from this PR. We can decide how v4 should show up in metadata tables in a separate discussion.

gaborkaszab · 2026-11-05T09:37:30Z

-              FileFormat.AVRO, fileIO.newInputFile(snapshot.manifestListLocation()))
+              FileFormat.AVRO,
+              ManifestLists.newInputFile(
+                  fileIO, new BaseFileWithEncryptedKey(location, null, snapshot.keyId(), null)))


I'm not entirely sure about the scope of this PR now. Based on the title I have the impression that we introduce support for the new root-manifest field to Snapshot and its parsers. Here we also do some other stuff:

Route InputFile creation through the caching ManifestLists path.

Introduce the more generic abstraction of ManifestListFile in order to be able to call ManifestLists.newInputFile.

Currently, I don't think that any of the above is the scope of this PR. Then we could remove the dependency on #17545 too.

Agreed on the scope. I removed the FileCleanupStrategy change and the ManifestLists routing, so this PR no longer depends on #17545. It now only adds the root-manifest field to Snapshot, BaseSnapshot, and SnapshotParser, plus the path rewrite in RewriteTablePathUtil.

When the selected snapshot references a Parquet root manifest, plan the scan through the native FilePlanner instead of the manifest-list ManifestGroup path. DataTableScan and BaseDistributedDataScan gate on root-manifest presence via a shared SnapshotScan.planV4RootManifest helper, so manifest-list snapshots (v1-v3 and those retained across an upgrade) are unaffected. RootManifestFile.from resolves the root against the table location; column stats and manifest-scan metrics are forwarded; unsupported entries (encrypted roots, column files, pre-v4 delete manifests) are rejected when encountered. Stacked on the preceding cherry-picked commit "API, Core: Add v4 root manifest location to Snapshot" (duplicated from apache#17523 to unblock the read path); drop it once apache#17523 merges on its own.

V4 snapshots reference a single root manifest instead of a manifest list (spec change in apache#16025). Add Snapshot.rootManifestLocation() and serialize it as the "root-manifest" field, keeping manifestListLocation() and "manifest-list" for v1 through v3. BaseSnapshot stores the location and does not support reading manifests from a root manifest until the reader lands. Root-manifest snapshots are built through BaseSnapshot.withRootManifest(), which requires an operation, a first-row-id, and a sequence number greater than 0; keeping manifest-list and root-manifest construction on separate paths makes setting both impossible. SnapshotParser rejects a root-manifest snapshot that also sets a manifest list or is missing sequence-number, summary, summary.operation, first-row-id, or added-rows. The v1 through v3 fallback that defaults a missing operation to overwrite is preserved for manifest-list snapshots and skipped for root-manifest snapshots, which require an operation. Producing a root manifest, reading one, and updating the manifest-list-centric callers (expire and cleanup, metadata tables, RewriteTablePath) are deferred to the root manifest reader and writer, which are not yet in the tree. This duplicates the standalone rootManifestLocation change from apache#17523 (currently stacked on encryption PR apache#17545). It is included here only to unblock the v4 read path and should be rebased/dropped once apache#17523 lands on its own. Spec PR: apache#16025 (cherry picked from commit 02eb1329e21d06f8ed8acb5702702a180a0a9051)

Route a data scan to the v4 `FilePlanner` when the selected snapshot references a root manifest. Stacked on a cherry-picked commit from apache#17523

v4 snapshots store their root file in "root-manifest" instead of "manifest-list". Add Snapshot.rootManifestLocation() and serialize it as the "root-manifest" JSON field, keeping manifestListLocation() and "manifest-list" for v1 through v3. Each snapshot sets exactly one of the two. Read-path subset of apache#17523 (commit 5de0a95), excluding its encryption dependency and the manifest-list-centric callers (metadata tables, cleanup, RewriteTablePath) plus the engine and doc changes the read path does not need. (cherry picked from commit 5de0a95)

Route a data scan to the v4 `FilePlanner` when the selected snapshot references a root manifest. Stacked on a cherry-picked commit from apache#17523

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 at most one of the two. 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. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

stevenzwu · 2026-11-12T00:34:49Z

-          Iterables.transform(snapshot.allManifests(DUMMY_FILE_IO), ManifestFile::path),
-          generator);
+      String rootManifest = snapshot.rootManifestLocation();
+      if (rootManifest != null) {


Done. toJson now reads both locations up front and uses a flat if / else if / else.

stevenzwu · 2026-11-12T00:34:50Z

+      String rootManifest = snapshot.rootManifestLocation();
+      if (rootManifest != null) {
+        generator.writeStringField(ROOT_MANIFEST, rootManifest);
+      } else {


Agreed. The order is now root-manifest first, then manifest-list, then the embedded v1 manifests.

stevenzwu · 2026-11-12T00:34:52Z

+
+    if (manifestList != null || rootManifest != null) {
+      // manifest-list is used by v1-v3 snapshots, root-manifest by v4 snapshots
      return new BaseSnapshot(


Yes, with the list-only constructor gone, fromJson now has a single call that passes both manifestList and rootManifest. The v1 embedded-manifests constructor is still separate.

stevenzwu · 2026-11-12T00:34:54Z

              6,
              "summary",
-              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())));
+              Types.MapType.ofRequired(7, 8, Types.StringType.get(), Types.StringType.get())),


Agreed. I removed the snapshots metadata table changes from this PR. We can decide how v4 should show up in metadata tables in a separate discussion.

stevenzwu · 2026-11-12T00:34:55Z

-              FileFormat.AVRO, fileIO.newInputFile(snapshot.manifestListLocation()))
+              FileFormat.AVRO,
+              ManifestLists.newInputFile(
+                  fileIO, new BaseFileWithEncryptedKey(location, null, snapshot.keyId(), null)))


Agreed on the scope. I removed the FileCleanupStrategy change and the ManifestLists routing, so this PR no longer depends on #17545. It now only adds the root-manifest field to Snapshot, BaseSnapshot, and SnapshotParser, plus the path rewrite in RewriteTablePathUtil.

stevenzwu · 2026-11-12T00:36:39Z

      Map<String, String> summary,
      Integer schemaId,
      String manifestList,
+      String rootManifest,


I followed @gaborkaszab 's suggestion and dropped the list-only overload, keeping a single constructor that takes both locations. Existing callers pass null for rootManifest.

The upside is a single signature. The downside is every previous list-only call site now passes an extra null, which leads to churning in test code.

This is how BaseSnapshot has grown before: schemaId (#2275), firstRowId/addedRows (#11948), and keyId (#12927) were added to the same constructor.

We might want to introduce a dedicated contract once all the other V4 Snapshot fields are introduced, but let it be the concert of the follow-ups, not this one.
Thanks!

stevenzwu · 2026-11-12T00:42:47Z

+        keyId);
+  }
+
+  BaseSnapshot(


went with your recommendation and modify the existing constructor directly.

stevenzwu · 2026-11-12T02:40:08Z

Also, just a thought on the direct follow-ups: There are a handful of new fields introduced for V4 snapshots. This PR introduces a single one of them. Shouldn't we do them in bulk by a single PR?

@gaborkaszab I would separate them out. this can get in first to unblock @anoopj

nastra · 2026-11-12T08:16:26Z

+   * Return the location of this snapshot's root manifest, or null if this snapshot has no root
+   * manifest.
+   *
+   * <p>v4 snapshots store this in {@code root-manifest} instead of a manifest list. At most one of


"At most one of this..." reads a bit confusing. Maybe just say something like "Snapshots in v1-v3 use {@link #manifestListLocation()} instead"?

nastra · 2026-11-12T08:20:44Z

+            null /* summary */,
+            1 /* schemaId */,
+            "ml.avro" /* manifestListLocation */,
+            null /* rootManifest */,


we should maybe have a test in this class where root manifest is set

nastra · 2026-11-12T08:24:55Z

              snapshot.schemaId(),
-              newManifestListLocation,
+              manifestList == null ? null : newPath(manifestList, sourcePrefix, targetPrefix),
+              rootManifest == null ? null : newPath(rootManifest, sourcePrefix, targetPrefix),


we should probably have a test in TestRewriteTablePathUtil that makes sure the root manifest is rewritten to the new location

gaborkaszab

Thanks @stevenzwu !

gaborkaszab · 2026-11-12T07:35:57Z

@@ -166,10 +166,26 @@ default Iterable<DeleteFile> removedDeleteFiles(FileIO io) {
  /**
   * Return the location of this snapshot's manifest list, or null if it is not separate.


The "or null if.." part is also relevant now here, right?

gaborkaszab · 2026-11-12T07:39:04Z

   * Return the location of this snapshot's manifest list, or null if it is not separate.
   *
-   * @return the location of the manifest list for this Snapshot
+   * <p>Only set for tables with format version 1 through 3. Snapshots in v4 tables use {@link


I'm hesitant adding such a comment here. We also don't mention that for first-row-id, added-rows etc. are only relevant from V3 onwards.
I'd like it to the spec to describe the relation between this and rootManifestLocation.

gaborkaszab · 2026-11-12T07:39:20Z

+   * Return the location of this snapshot's root manifest, or null if this snapshot has no root
+   * manifest.
+   *
+   * <p>v4 snapshots store this in {@code root-manifest} instead of a manifest list. At most one of


Same comment as above

gaborkaszab · 2026-11-12T07:40:31Z

      Map<String, String> summary,
      Integer schemaId,
      String manifestList,
+      String rootManifest,


We might want to introduce a dedicated contract once all the other V4 Snapshot fields are introduced, but let it be the concert of the follow-ups, not this one.
Thanks!

gaborkaszab · 2026-11-12T07:43:49Z

      Long firstRowId,
      Long addedRows,
      String keyId) {
+    Preconditions.checkArgument(


This verifies they can't be set together. Should we also verify that one of them has to be set?

gaborkaszab · 2026-11-12T08:28:08Z

    if (allManifests == null) {
-      // if manifests isn't set, then the snapshotFile is set and should be read to get the list
+      // if manifests isn't set, then the manifest list must be read
+      Preconditions.checkArgument(


Don't we want to reverse this and at L181 verify that rootManifestLocation is null, otherwise throw that we don't support this for V4. We could leave the rest of the function intact.

gaborkaszab · 2026-11-12T09:01:17Z

+    String rootManifest = JsonUtil.getStringOrNull(ROOT_MANIFEST, node);
+
+    if (manifestList != null || rootManifest != null) {
+      // manifest-list is used by v1-v3 snapshots, root-manifest by v4 snapshots


nit: comment is not needed

gaborkaszab · 2026-11-12T09:11:17Z

    for (Snapshot snapshot : metadata.snapshots()) {
-      String newManifestListLocation =
-          newPath(snapshot.manifestListLocation(), sourcePrefix, targetPrefix);
+      String manifestList = snapshot.manifestListLocation();


We're introducing the new field to Snapshot, I don't think that we should implement table rewrite functionality right away. Could that be separate?

gaborkaszab · 2026-11-12T09:14:59Z

+  public void testAddSnapshotWithRootManifest() {
+    String action = MetadataUpdateParser.ADD_SNAPSHOT;
+    long snapshotId = 2;
+    int schemaId = 3;


nit: this could be inline at L443

gaborkaszab · 2026-11-12T09:18:46Z


    assertThat(snapshot.snapshotId()).isEqualTo(expected.snapshotId());
+    assertThat(snapshot.manifestListLocation()).isEqualTo(expected.manifestListLocation());
+    assertThat(snapshot.rootManifestLocation()).isEqualTo(expected.rootManifestLocation());


Not sure what coverage we add with these asserts on rootManifestLocation, because they are always null, except in one dedicated test. Should we assert isNull() as we do with other fields below?

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

Comment thread core/src/main/java/org/apache/iceberg/BaseSnapshot.java Resolved

Comment thread api/src/main/java/org/apache/iceberg/SnapshotFile.java Resolved

Comment thread api/src/main/java/org/apache/iceberg/Snapshot.java Resolved

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

Comment thread api/src/main/java/org/apache/iceberg/ManifestFile.java Resolved

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

Merged

1 of 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 3 times, most recently 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

github-actions Bot added the stale label Oct 12, 2026

github-actions Bot closed this Oct 20, 2026

stevenzwu force-pushed the snapshot_file_extract branch 3 times, most recently from 68e2be0 to 0b626a8 Compare November 2, 2026 05:35

nastra added this to V4: metadata tree Nov 2, 2026

stevenzwu force-pushed the snapshot_file_extract branch from 0b626a8 to 803b4f6 Compare November 2, 2026 22:21

stevenzwu changed the title ~~API, Core, Spark, Flink: add version-agnostic Snapshot.rootLocation()~~ API, Core, Spark, Flink: add Snapshot.rootManifestLocation() Nov 2, 2026

stevenzwu force-pushed the snapshot_file_extract branch from 803b4f6 to 015f87b Compare November 2, 2026 23:30

github-actions Bot removed the NESSIE label Nov 2, 2026

stevenzwu force-pushed the snapshot_file_extract branch from 015f87b to 1284eb0 Compare November 2, 2026 23:39

stevenzwu changed the title ~~API, Core, Spark, Flink: add Snapshot.rootManifestLocation()~~ API, Core, Spark: add Snapshot.rootManifestLocation() Nov 2, 2026

stevenzwu force-pushed the snapshot_file_extract branch 2 times, most recently from d1c11ad to 5de0a95 Compare November 3, 2026 05:56

nastra requested a review from amogh-jahagirdar November 3, 2026 14:50

nastra reviewed Nov 3, 2026

View reviewed changes

gaborkaszab reviewed Nov 5, 2026

View reviewed changes

anoopj mentioned this pull request Nov 11, 2026

Core: Plan v4 root-manifest snapshots with the file planner #18884

Open

stevenzwu commented Nov 12, 2026

View reviewed changes

nastra reviewed Nov 12, 2026

View reviewed changes

gaborkaszab reviewed Nov 12, 2026

View reviewed changes

		@@ -166,10 +166,26 @@ default Iterable<DeleteFile> removedDeleteFiles(FileIO io) {
		/**
		* Return the location of this snapshot's manifest list, or null if it is not separate.

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 12, 2026

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

stevenzwu commented Aug 4, 2026 •

edited

Loading

gaborkaszab commented Nov 5, 2026 •

edited

Loading