API, Core: Add ManifestFile adapter for v4 tracked files#17932

gaborkaszab · 2026-09-03T11:34:16Z

Not sure about the general opinion on this, but I was asked on code reviews multiple occasions to remove all the tiny nitpicking that are unrelated to the PR itself. I know it seems an overkill to open a separate PR to these, but following that logic, this and the same below should be removed from this PR.

Seemed a bit of al overkill. :) I will remove it if there is objection from another reviewer

gaborkaszab · 2026-09-03T11:51:45Z

Same question as for Tracking: Are we sure ManifestInfo is not null (part of the projection)?

It should be part of the projection and a reader/caller responsibility. I don't think we should be doing null guards here.

I agree that we don't want to add a check to account for the case where manifest info wasn't projected.
However, we do need to account for the case where the manifest has not been written into a root manifest and does not yet have Tracking metadata. I think tracking should be: file.tracking() != null ? file.tracking().dataSequenceNumber() : null

file.tracking() != null ? file.tracking().dataSequenceNumber() : null
This works for boxed accessors, but won't compile because we need to return a primitive long. ManifestFile.sequenceNumber() and minSequenceNumber() both return long. So should we just return a sentinel value? Perhaps -1?
public abstract class ManifestWriter<F extends ContentFile> implements FileAppender {
// stand-in for the current sequence number that will be assigned when the commit is successful
// this is replaced when writing a manifest list by the ManifestFile wrapper
static final long UNASSIGNED_SEQ = -1L;
Also I assume we need to handle this in sequenceNumber() also.

That constant is always replaced and we should not return it through an API method.
We should just return the value and accept the NPE if it is null. If we can't express that by checking tracking, then it's an NPE either way and we should just return the expression without a null check.

Sounds good. So not making any changes here.

rdblue · 2026-09-04T22:05:32Z

@stevenzwu added these methods to ManifestFile in https://github.com/apache/iceberg/pull/16936/changes. Maybe we should add those here first, since this is a simpler interface?
If we add them, then I think we should test non-zero values. I think that these should default to 0 since that is the case for pre-v4 manifest files. (See my comment: https://github.com/apache/iceberg/pull/16936/changes#r3929689004)
If we don't end up adding them here, we'll need to change these values and test them after that PR is merged.

To reduce churn in @stevenzwu 's PR, I will leave them out for now. I can do a followup as soon as his PR is merged.

gaborkaszab · 2026-09-03T12:24:21Z

Is spec not allowed or optional for manifests? I know it they aren't bound to a single partition as the comment says, but in case they happen to, then is it still not allowed to set this?

My rationale was that the concept doesn't apply anymore, so callers should not rely on it.

I agree with this. We don't want to alter the method unless we have to. Throwing an exception is the right call for now.

-Original file line number
+Diff line change
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+import java.nio.ByteBuffer;
+/** A bitmap that is serialized inline in a metadata file. */
+public interface ManifestBitmap {
+ /** Number of bits set in this bitmap. */
+ int cardinality();
+ /** Return whether the bit at {@code position} is set. */
+ boolean isSet(int position);
+ /** Return the serialized bitmap as a {@link ByteBuffer}. */
+ ByteBuffer buffer();
+}

-Original file line number
+Diff line change
@@ Expand Up / @@ -126,7 +126,7 @@ static Schema schema() { @@
  /** Returns length of the manifest file. */
  long length();
- /** Returns iD of the {@link PartitionSpec} used to write the manifest file. */
+ /** Returns ID of the {@link PartitionSpec} used to write the manifest file. */
  int partitionSpecId();
  /** Returns the content stored in the manifest; either DATA or DELETES. */
@@ Expand All / @@ -138,7 +138,7 @@ static Schema schema() { @@
  /** Returns the lowest data sequence number of any live file in the manifest. */
  long minSequenceNumber();
- /** Returns iD of the snapshot that added the manifest file to table metadata. */
+ /** Returns ID of the snapshot that added the manifest file to table metadata. */
  Long snapshotId();
  /**
@@ Expand DownExpand Up / @@ -210,6 +210,11 @@ default Long firstRowId() { @@
  return null;
  }
+ /** Returns the manifest deletion vector, or null if absent. */
+ default ManifestBitmap manifestDeletionVector() {
+ return null;
+ }
  /**
  * Copies this {@link ManifestFile manifest file}. Readers can reuse manifest file instances; use
  * this method to make defensive copies.
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -24,7 +24,7 @@ @@
 import java.util.Set;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
-/** Adapts {@link TrackedFile} entries to the {@link DataFile} and {@link DeleteFile} APIs. */
+/** Adapts {@link TrackedFile} entries to their read APIs, for example {@link DataFile}. */
 class TrackedFileAdapters {
  private TrackedFileAdapters() {}
@@ Expand DownExpand Up @@
  return new TrackedEqualityDeleteFile(file, resolveSpec(file, specsById));
  }
- /** Shared base for all tracked file adapters. */
+ static ManifestFile asManifestFile(TrackedFile file) {
+ Preconditions.checkArgument(
+ file.contentType() == FileContent.DATA_MANIFEST
+ || file.contentType() == FileContent.DELETE_MANIFEST,
+ "Invalid content type for ManifestFile: %s",
+ file.contentType());
+ return new TrackedManifestFile(file);
+ }
+ /** Shared base for data and delete file adapters. */
  private abstract static class TrackedFileAdapter<F extends ContentFile<F>>
  implements ContentFile<F> {
  private final TrackedFile file;
@@ Expand DownExpand Up @@
  }
  }
+ /** Adapts a TrackedFile to {@link ManifestFile}. */
+ private static class TrackedManifestFile implements ManifestFile {
+ private final TrackedFile file;
+ private TrackedManifestFile(TrackedFile file) {
+ this.file = file;
+ }
+ @Override
+ public String path() {
+ return file.location();
+ }
+ @Override
+ public long length() {
+ return file.fileSizeInBytes();
+ }
+ @Override
+ public int partitionSpecId() {
+ throw new UnsupportedOperationException(
+ "v4 manifests are not bound to a single partition spec");
+ }
+ @Override
+ public ManifestContent content() {
+ 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());
+ }
+ }
+ @Override
+ public long sequenceNumber() {
+ return file.tracking().dataSequenceNumber();
+ }
+ @Override
+ public long minSequenceNumber() {
+ return file.manifestInfo().minSequenceNumber();
+ }
+ @Override
+ public Long snapshotId() {
+ return file.tracking().snapshotId();
+ }
+ @Override
+ public Integer addedFilesCount() {
+ return file.manifestInfo().addedFilesCount();
+ }
+ @Override
+ public Long addedRowsCount() {
+ return file.manifestInfo().addedRowsCount();
+ }
+ @Override
+ public Integer existingFilesCount() {
+ return file.manifestInfo().existingFilesCount();
+ }
+ @Override
+ public Long existingRowsCount() {
+ return file.manifestInfo().existingRowsCount();
+ }
+ @Override
+ public Integer deletedFilesCount() {
+ return file.manifestInfo().deletedFilesCount();
+ }
+ @Override
+ public Long deletedRowsCount() {
+ return file.manifestInfo().deletedRowsCount();
+ }
+ @Override
+ public List<PartitionFieldSummary> partitions() {
+ return null;
+ }
+ @Override
+ public ByteBuffer keyMetadata() {
+ return file.keyMetadata();
+ }
+ @Override
+ public Long firstRowId() {
+ return file.tracking().firstRowId();
+ }
+ @Override
+ public ManifestBitmap manifestDeletionVector() {
+ ByteBuffer dv = file.manifestInfo().dv();
+ if (dv == null) {
+ return null;
+ }
+ return new ManifestBitmap() {
+ @Override
+ public int cardinality() {
+ throw new UnsupportedOperationException("Bitmap decoding has not been implemented");
+ }
+ @Override
+ public boolean isSet(int position) {
+ throw new UnsupportedOperationException("Bitmap decoding has not been implemented");
+ }
+ @Override
+ public ByteBuffer buffer() {
+ return dv;
+ }
+ };
+ }
+ @Override
+ public ManifestFile copy() {
+ return new TrackedManifestFile(file.copy());
+ }
+ }
  private static PartitionSpec resolveSpec(
  TrackedFile file, Map<Integer, PartitionSpec> specsById) {
  Integer specId = file.specId();
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -33,16 +33,19 @@ @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.EnumSource;
+import org.mockito.Mockito;
 class TestTrackedFileAdapters {
  private static final int FORMAT_VERSION_V4 = 4;
  private static final String MANIFEST_LOCATION = "s3://bucket/table/manifest.parquet";
  private static final String DATA_FILE_LOCATION = "s3://bucket/data/file.parquet";
  private static final String DV_LOCATION = "s3://bucket/puffin/dv-file.bin";
+ private static final long MANIFEST_FILE_SIZE = 2048L;
  // Tracking values that the delegation tests validate.
  private static final long MANIFEST_POS = 3L;
+ private static final long SNAPSHOT_ID = 42L;
  private static final long DATA_SEQUENCE_NUMBER = 10L;
  private static final long FILE_SEQUENCE_NUMBER = 11L;
  private static final long FIRST_ROW_ID = 1000L;
@@ Expand DownExpand Up / @@ -91,6 +94,36 @@ class TestTrackedFileAdapters { @@
  CONTENT_STATS.setStats(3, GEOM_STATS);
  }
+ private static final Tracking MANIFEST_TRACKING =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ SNAPSHOT_ID,
+ DATA_SEQUENCE_NUMBER,
+ FILE_SEQUENCE_NUMBER,
+ null, // dvSnapshotId
+ FIRST_ROW_ID,
+ null, // deletedPositions
+ null); // replacedPositions
+ private static final byte[] MANIFEST_DV = new byte[] {1, 2, 3};
+ private static final ByteBuffer MANIFEST_KEY_METADATA = ByteBuffer.wrap(new byte[] {7, 8, 9});
+ private static final ManifestInfo MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(3)
+ .existingFilesCount(5)
+ .deletedFilesCount(2)
+ .replacedFilesCount(0)
+ .addedRowsCount(300L)
+ .existingRowsCount(500L)
+ .deletedRowsCount(200L)
+ .replacedRowsCount(0L)
+ .minSequenceNumber(7L)
+ .dv(ByteBuffer.wrap(MANIFEST_DV))
+ .dvCardinality(4L)
+ .build();
  @Test
  void dataFileAdapterDelegation() {
  TrackingStruct tracking =
@@ Expand DownExpand Up / @@ -350,6 +383,83 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() { @@
  .hasMessage("Cannot create DV delete file: no deletion vector");
  }
+ @ParameterizedTest
+ @EnumSource(
+ value = FileContent.class,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterDelegation(FileContent contentType) {
+ TrackedFile file =
+ new TrackedFileStruct(
+ MANIFEST_TRACKING,
+ contentType,
+ FORMAT_VERSION_V4,
+ MANIFEST_LOCATION,
+ FileFormat.PARQUET,
+L, // recordCount
+ MANIFEST_FILE_SIZE,
+ null, // specId
+ null, // partition
+ null, // contentStats
+ null, // sortOrderId
+ null, // deletionVector
+ MANIFEST_INFO,
+ MANIFEST_KEY_METADATA,
+ null, // splitOffsets
+ null); // equalityIds
+ ManifestFile manifest = TrackedFileAdapters.asManifestFile(file);
+ ManifestContent expectedContent =
+ contentType == FileContent.DATA_MANIFEST ? ManifestContent.DATA : ManifestContent.DELETES;
+ assertThat(manifest.path()).isEqualTo(MANIFEST_LOCATION);
+ assertThat(manifest.length()).isEqualTo(MANIFEST_FILE_SIZE);
+ assertThat(manifest.content()).isEqualTo(expectedContent);
+ assertThat(manifest.sequenceNumber()).isEqualTo(DATA_SEQUENCE_NUMBER);
+ assertThat(manifest.minSequenceNumber()).isEqualTo(MANIFEST_INFO.minSequenceNumber());
+ assertThat(manifest.snapshotId()).isEqualTo(SNAPSHOT_ID);
+ assertThat(manifest.addedFilesCount()).isEqualTo(MANIFEST_INFO.addedFilesCount());
+ assertThat(manifest.addedRowsCount()).isEqualTo(MANIFEST_INFO.addedRowsCount());
+ assertThat(manifest.existingFilesCount()).isEqualTo(MANIFEST_INFO.existingFilesCount());
+ assertThat(manifest.existingRowsCount()).isEqualTo(MANIFEST_INFO.existingRowsCount());
+ assertThat(manifest.deletedFilesCount()).isEqualTo(MANIFEST_INFO.deletedFilesCount());
+ assertThat(manifest.deletedRowsCount()).isEqualTo(MANIFEST_INFO.deletedRowsCount());
+ assertThat(manifest.firstRowId()).isEqualTo(FIRST_ROW_ID);
+ assertThat(manifest.keyMetadata()).isEqualTo(MANIFEST_KEY_METADATA);
+ assertThat(manifest.manifestDeletionVector().buffer()).isEqualTo(ByteBuffer.wrap(MANIFEST_DV));
+ assertThat(manifest.partitions()).isNull();
+ assertThatThrownBy(manifest::partitionSpecId)
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("v4 manifests are not bound to a single partition spec");
+ }
+ @Test
+ void manifestFileAdapterCopy() {
+ TrackedFile file = Mockito.mock(TrackedFile.class);
+ TrackedFile fileCopy = Mockito.mock(TrackedFile.class);
+ Mockito.when(file.contentType()).thenReturn(FileContent.DATA_MANIFEST);
+ Mockito.when(file.copy()).thenReturn(fileCopy);
+ Mockito.when(fileCopy.location()).thenReturn(MANIFEST_LOCATION);
+ ManifestFile copy = TrackedFileAdapters.asManifestFile(file).copy();
+ // copy() delegates to the tracked file's copy(), which deep-copies the nested structs.
+ Mockito.verify(file).copy();
+ assertThat(copy.path()).isEqualTo(MANIFEST_LOCATION);
+ }
+ @ParameterizedTest
+ @EnumSource(
+ value = FileContent.class,
+ mode = EnumSource.Mode.EXCLUDE,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterRejectsNonManifestContent(FileContent contentType) {
+ TrackedFileStruct file = dummyTrackedFile(contentType);
+ assertThatThrownBy(() -> TrackedFileAdapters.asManifestFile(file))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid content type for ManifestFile: %s", contentType);
+ }
  @Test
  void dataFileWithoutDeletionVectorReturnsNull() {
  TrackedFile fileWithoutDv = mock(TrackedFile.class);
@@ Expand Down @@

Navigation Menu

Uh oh!

API, Core: Add ManifestFile adapter for v4 tracked files#17932

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gaborkaszabSep 3, 2026

Uh oh!

anoopjSep 3, 2026

Uh oh!

gaborkaszabSep 3, 2026

Uh oh!

anoopjSep 3, 2026

Uh oh!

rdblueSep 3, 2026

Uh oh!

anoopjSep 4, 2026

Uh oh!

rdblueSep 4, 2026

Uh oh!

anoopjSep 4, 2026

Uh oh!

Uh oh!

rdblueSep 4, 2026•
edited
Loading

Uh oh!

anoopjSep 4, 2026

Uh oh!

gaborkaszabSep 3, 2026

Uh oh!

anoopjSep 3, 2026

Uh oh!

rdblueSep 3, 2026

Uh oh!

Uh oh!

Uh oh!

API, Core: Add ManifestFile adapter for v4 tracked files#17932

Uh oh!

API, Core: Add ManifestFile adapter for v4 tracked files #17932

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

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!

Uh oh!

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

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!

Uh oh!

rdblueSep 4, 2026•
edited
Loading