Core: Add ManifestFile adapter for v4 tracked files by anoopj · Pull Request #16867 · apache/iceberg

anoopj · 2026-06-18T23:42:49Z

Add TrackedFileAdapters.asManifestFile to wrap a DATA_MANIFEST or DELETE_MANIFEST TrackedFile as a ManifestFile.

The ManifestFile adapter will only be used by tests. Note that ManifestFile doesn't support manifest DVs. So in the actual manifest read path, the reader resolves the MDV first and produces the resulting TrackedFiles, which are then adapted to data/delete files.

Closes #16227

Add TrackedFileAdapters.asManifestFile to wrap a DATA_MANIFEST or DELETE_MANIFEST TrackedFile as a ManifestFile. Closes apache#16227

wombatu-kun

Code looks correct: field delegation, equals/hashCode (consistent with GenericManifestFile), constructor preconditions, and the test coverage are all solid.

Non-blocking discussion point: partitionSpecId() throws UnsupportedOperationException. That is defensible since a v4 manifest is not bound to a single spec. But @rdblue pushed back on #16100 against these adapters exposing an invalid or arbitrary spec ID, so it may be worth confirming with reviewers that a throw is the intended contract for any future caller that reads partitionSpecId().

stevenzwu · 2026-06-19T06:09:13Z

+            NO_PARTITION,
+            8L,
+            2048L);
+    file.set(MANIFEST_INFO_ORDINAL, manifestInfo);


we can use the builder from @gaborkaszab 's PR: #16769

Sounds good. I can make the change if #16769 gets merged first or do this as a followup PR if this gets merged first.

FYI, in the meantime the builder got merged

gaborkaszab

Thank you for the PR, @anoopj !
I took a Quick Look and left some minor comment.

gaborkaszab · 2026-06-19T15:18:15Z

+
+    private TrackedManifestFile(TrackedFile file) {
+      Preconditions.checkArgument(
+          file.tracking() != null, "Cannot create manifest file: no tracking");


nit: other adapters don't check if tracking is null. Not sure we have to here. It's a required field anyway

gaborkaszab · 2026-06-19T15:21:18Z

+    private TrackedManifestFile(TrackedFile file) {
+      Preconditions.checkArgument(
+          file.tracking() != null, "Cannot create manifest file: no tracking");
+      Preconditions.checkArgument(


As per the spec proposal data seq num is 'Optional for leaf manifests, required for root.'. I think it's valid to have it as null here. Seems also optional in ManifestFile

This PR is for root-level manifest tracking where null is truly an invalid state. We can either validate or just have it fail with an NPE.

Seems also optional in ManifestFile

It is required for a manifest file: ManifestFile.sequenceNumber() returns a primitive

just to add on top of what Anoop said. this is for leaf manifest file entry (TrackedFile) in the root manifest file. Hence dataSequenceNumber should be required

Thanks for the explanation @anoopj and @stevenzwu !

gaborkaszab · 2026-06-19T15:40:26Z

+          file.tracking().snapshotId() != null, "Cannot create manifest file: no snapshot ID");
+      Preconditions.checkArgument(
+          file.manifestInfo() != null, "Cannot create manifest file: no manifest info");
+      Preconditions.checkArgument(


This checks an internal invariant for a TrackedFile. As per the spec proposal a DELETE_MANIFEST shouldn't have firstRowId, so I'd assume that this is a contract to keep when we construct a TrackedFile and set this value. In fact this is check in the relevant PR

gaborkaszab · 2026-06-19T15:42:42Z

+    }
+
+    @Override
+    public boolean equals(Object other) {


The other adapters don't override equals and hashCode. Is there a particular reason it's needed here?

I'm trying to be consistent with the semantics of GenericManifestFile.

Do we actually use equals and hashCode? We try to avoid implementing those methods where there are different interpretations of what "equality" might mean.

I didn't add any code that uses it. I was trying to be consistent with the behavior of the GenericManifestFile which defines equality with the location. I have removed it.

stevenzwu

LGTM.

I will merge Gabor's TrackedFileBuilder once CI passed. then let's rebase this one and use the builder as applicable.

stevenzwu · 2026-06-22T20:56:01Z

+    private TrackedManifestFile(TrackedFile file) {
+      Preconditions.checkArgument(
+          file.tracking() != null, "Cannot create manifest file: no tracking");
+      Preconditions.checkArgument(


just to add on top of what Anoop said. this is for leaf manifest file entry (TrackedFile) in the root manifest file. Hence dataSequenceNumber should be required

rdblue · 2026-06-22T23:45:47Z

+    private TrackedManifestFile(TrackedFile file) {
+      Preconditions.checkArgument(
+          file.tracking().dataSequenceNumber() != null,
+          "Cannot create manifest file: no data sequence number");


Nit: I think we prefer the slightly more specific "Invalid data sequence number: null" because "no X" doesn't necessarily mean "x is null".

Also, even though the attempt is to create a manifest file, that's not the actual problem. It's debatable whether to include what is being attempted, "Cannot create manifest file", but I would opt to leave it out because that's clear from other context (that the check is in asManifestFile and the TrackedManifestFile constructor).

rdblue · 2026-06-22T23:49:09Z

+    @Override
+    public int partitionSpecId() {
+      throw new UnsupportedOperationException(
+          "Tracked manifests are not bound to a single partition spec");


I don't think that it is correct to say "Tracked manifests": what is a "tracked" manifest and why is one from a manifest list not "tracked"?

I think it is better to say "v4 manifests are not bound to a single partition spec".

Fixed. I was trying to avoid having too many v4 references.

rdblue · 2026-06-22T23:51:42Z

+    public ManifestContent content() {
+      return file.contentType() == FileContent.DATA_MANIFEST
+          ? ManifestContent.DATA
+          : ManifestContent.DELETES;


Minor: I think it is safer to avoid a dependency between this and the constructor's verification. I would recommend using a switch statement:

switch (file.contentType()) { case DATA_MANIFEST: return ManifestContent.DATA; case DELETE_MANIFEST: return ManifestContent.DELETES; default: throw new UnsupportedOperationException("Unsupported content type for manifests: " + file.contentType()); }

rdblue · 2026-06-22T23:57:35Z

+    public boolean equals(Object other) {
+      if (this == other) {
+        return true;
+      } else if (!(other instanceof TrackedManifestFile)) {


I don't think that the implementation of GenericManifestFile is very helpful. It considers two files equal if they have the same path (which I think is reasonable) but it also excludes any implementation other than GenericManifestFile, which means that they aren't actually interchangeable as the interface suggests.

In addition, this creates a situation where a.equals(b) may be true, but b.equals(a) can be false for reasonable implementations.

I'd prefer to remove this and fix places where it is assumed or used. Good catch, @gaborkaszab!

gaborkaszab · 2026-06-23T08:53:09Z

+          "Cannot create manifest file: no data sequence number");
+      Preconditions.checkArgument(
+          file.tracking().snapshotId() != null, "Cannot create manifest file: no snapshot ID");
+      Preconditions.checkArgument(


This also seems an internal invariant for TrackedFile. The new TrackedFileBuilder guarantees that manifestInfo is set for manifest entries.

That makes sense. Removed.

gaborkaszab · 2026-06-23T09:17:13Z

+            NO_PARTITION,
+            8L,
+            2048L);
+    file.set(MANIFEST_INFO_ORDINAL, manifestInfo);


FYI, in the meantime the builder got merged

gaborkaszab · 2026-06-23T09:18:07Z

+    private TrackedManifestFile(TrackedFile file) {
+      Preconditions.checkArgument(
+          file.tracking() != null, "Cannot create manifest file: no tracking");
+      Preconditions.checkArgument(


Thanks for the explanation @anoopj and @stevenzwu !

gaborkaszab · 2026-06-23T09:29:35Z

+  void testManifestFileAdapterDelegation() {
+    Tracking tracking = createTracking();
+    ManifestInfo manifestInfo = createManifestInfo();
+    TrackedFileStruct file =


nit: Can we hide the construction of the input TrackedFile into a separate function? Like dataManifestFile() it could get ManifestInfo, FileContent, and/or Tracking parameters if that matters for the test.

Done. extracted manifestBuilder(FileContent) / manifestFile(FileContent, ManifestInfo) and now tests build through them.

gaborkaszab · 2026-06-23T09:44:28Z

  }

+  @Test
+  void testManifestFileAdapterDelegation() {


nit: maybe parameterize the test for data + delete manifest. The testDeleteManifestContentMapsToDeletes test could be dropped then

good suggesiton. Done.

# Conflicts: # core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java

gaborkaszab

just some minor comments. Other than them, LGTM!

gaborkaszab · 2026-06-26T13:16:21Z

+            .manifestInfo(manifestInfo)
+            .keyMetadata(ByteBuffer.wrap(new byte[] {7, 8, 9}))
+            .build();
+    populateTrackingFields(file);


Just FYI: in the follow-up for the TrackedFileBuilder PR, we decided to drop populateTrackingFields and went to call the constructor of TrackedFile instead using the createTracking (revived in the follow-up PR) method.

Ack. I can rebase when the followup PR is merged.

gaborkaszab · 2026-06-26T13:20:43Z

+        manifestBuilder(FileContent.DATA_MANIFEST).manifestInfo(createManifestInfo()).build();
+    // Inheritance fills the data sequence number, which the adapter requires; first row ID and
+    // key metadata stay unset.
+    ((TrackingStruct) file.tracking()).set(DATA_SEQUENCE_NUMBER_ORDINAL, DATA_SEQUENCE_NUMBER);


instead of using the ordinal, we can use the inheritFrom(some_manifest) method to do that.

Routing through inheritFrom() will require building a parent Tzracking with several nulls. I prefer to keep it since this is simpler.

anoopj moved this to In review in V4: metadata tree Jun 18, 2026

anoopj added this to V4: metadata tree Jun 18, 2026

github-actions Bot added the core label Jun 18, 2026

anoopj force-pushed the manifestfile-adapter branch 2 times, most recently from b50410f to 718ae34 Compare June 19, 2026 00:42

Core: Add ManifestFile adapter for v4 tracked files

93368cb

Add TrackedFileAdapters.asManifestFile to wrap a DATA_MANIFEST or DELETE_MANIFEST TrackedFile as a ManifestFile. Closes apache#16227

anoopj force-pushed the manifestfile-adapter branch from 718ae34 to 93368cb Compare June 19, 2026 00:47

wombatu-kun approved these changes Jun 19, 2026

View reviewed changes

stevenzwu reviewed Jun 19, 2026

View reviewed changes

gaborkaszab reviewed Jun 19, 2026

View reviewed changes

PR feedback

9974e9b

anoopj requested review from gaborkaszab and stevenzwu June 22, 2026 17:38

stevenzwu approved these changes Jun 22, 2026

View reviewed changes

rdblue reviewed Jun 22, 2026

View reviewed changes

gaborkaszab reviewed Jun 23, 2026

View reviewed changes

anoopj added 2 commits June 23, 2026 11:23

Merge remote-tracking branch 'apache/main' into manifestfile-adapter

67f64ae

# Conflicts: # core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java

Address review comments and adopt TrackedFileBuilder in tests

e3be26b

anoopj requested a review from rdblue June 23, 2026 18:36

gaborkaszab approved these changes Jun 26, 2026

View reviewed changes

anoopj added 3 commits June 26, 2026 17:53

PR feedback

ae41f2a

Merge remote-tracking branch 'apache/main' into manifestfile-adapter

73b9bfa

Fix rebase error

ae9a18e

Uh oh!

Conversation

anoopj commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wombatu-kun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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

anoopj commented Jun 18, 2026 •

edited

Loading