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

anoopj · 2026-09-02T21:54:56Z

Reintroduces the ManifestFile adapter closed in #16867, which stalled because ManifestFile had no way to represent a manifest deletion vector. The V4 scan task planner (#17541) plans over manifests through the ManifestFile API, so this adds ManifestFile.deletionVector() and has the adapter expose the tracked file's manifest DV.

Reintroduces the ManifestFile adapter closed in apache#16867, which stalled because ManifestFile had no way to represent a manifest deletion vector. The V4 scan task planner (apache#17541) plans over manifests through the ManifestFile API, so this adds ManifestFile.deletionVector() and has the adapter expose the tracked file's manifest DV.

anoopj · 2026-09-02T21:57:09Z

+ Preconditions.checkArgument(
+ tracking.dataSequenceNumber() != null, "Invalid data sequence number: null");
+ Preconditions.checkArgument(


Note to reviewers: It is unclear whether we need to do these validations during read time here. Also, we don't validate snapshot IDs currently. I'm OK to just remove it.

This has been unclear for me too. I'm not sure how we guard against data corruption on the read side and how we guarantee our internal invariants like this. On the read side we apparently assume that these invariants are satisfied, however, I'm not sure this is safe against some malformed data files.
Anyway, even if we want to check against these constraints, I think we should introduce those checks on the reader, not on the adapter. Reaching this point, I guess we should just assume that TrackedFile is correct and invariants are satisfied.
So probably we are fine not checking them here.

Removing them.

anoopj · 2026-09-02T21:58:33Z

 }

+ /** Returns the manifest deletion vector, or null if absent. */
+ default ByteBuffer deletionVector() {


Another design decision is whether we just have a ByteBuffer or whether we introduce a class for MDVs.

This is fine for now. We have other places that will need to be updated as well.

gaborkaszab

Thanks for the PR, @anoop!

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

 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. */


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:50:15Z

+ private final TrackedFile file;
+
+ private TrackedManifestFile(TrackedFile file) {
+ Tracking tracking = file.tracking();


I see that in TrackedFileAdapter every time we reference a member in Tracking, we branch based on whether Tracking is null or not. Maybe to cover if it's not projected by the read?
Shouldn't we perform that branching in this class too?

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

+
+ @Override
+ public long minSequenceNumber() {
+ return file.manifestInfo().minSequenceNumber();


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.

gaborkaszab · 2026-09-03T11:53:55Z

 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 String MANIFEST_FILE_LOCATION = "s3://bucket/table/manifest-1.parquet";


Can't we use the existing MANIFEST_LOCATION field?

You're right. Done.

gaborkaszab · 2026-09-03T12:14:55Z

+ value = FileContent.class,
+ names = {"DATA_MANIFEST", "DELETE_MANIFEST"})
+ void manifestFileAdapterDelegation(FileContent contentType) {
+ ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {7, 8, 9});


keyMetadata is not specific to this test, can it be a private static member?

Yes. Hoisting to a constant.

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

+
+ assertThatThrownBy(manifest::partitionSpecId)
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("v4 manifests are not bound to a single partition spec");


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.

gaborkaszab · 2026-09-03T12:26:18Z

+ assertThat(copy.keyMetadata()).isEqualTo(original.keyMetadata());
+ assertThat(copy.keyMetadata().array()).isNotSameAs(original.keyMetadata().array());
+ assertThat(copy.deletionVector()).isEqualTo(original.deletionVector());
+ assertThat(copy.deletionVector().array()).isNotSameAs(original.deletionVector().array());


Shouldn't we use the mocking mechanism we do for TrackedFile tests to verify that the nested struct's copy part?

Thank you. Converting it to mocks.

gaborkaszab · 2026-09-03T12:30:41Z

+ Preconditions.checkArgument(
+ tracking.dataSequenceNumber() != null, "Invalid data sequence number: null");
+ Preconditions.checkArgument(


This has been unclear for me too. I'm not sure how we guard against data corruption on the read side and how we guarantee our internal invariants like this. On the read side we apparently assume that these invariants are satisfied, however, I'm not sure this is safe against some malformed data files.
Anyway, even if we want to check against these constraints, I think we should introduce those checks on the reader, not on the adapter. Reaching this point, I guess we should just assume that TrackedFile is correct and invariants are satisfied.
So probably we are fine not checking them here.

rdblue · 2026-09-03T21:47:00Z

+ }
+
+ @Override
+ public ByteBuffer deletionVector() {


It's a little odd that in most places we're using deletionVector that returned DeletionVector but here we're wrapping a dv() call and returning ByteBuffer. I wonder if we should use a different name to signal that it isn't the same thing. What about metadataDV or manifestDeletionVector?

Renamed to manifestDeletionVector. I think it is a bit redundant to have ManifestFile.manifestDeletionVector - let me know if you have a suggestion here.

rdblue · 2026-09-03T21:47:59Z

+ /* dvSnapshotId= */ null,
+ FIRST_ROW_ID,
+ /* deletedPositions= */ null,
+ /* replacedPositions= */ null);


Most places use // replaced positions rather than using an inline comment before null.

Changing to //

rdblue · 2026-09-03T21:49:22Z

+ null,
+ null,
+ null,
+ null,


It would be helpful to label these as well.

rdblue · 2026-09-03T21:51:11Z

+ FORMAT_VERSION_V4,
+ MANIFEST_LOCATION,
+ FileFormat.PARQUET,
+ 0L,


We should not pass values that could easily be disallowed later with more strict validation. Let's pass a real value here instead.

Changed it to 10

rdblue · 2026-09-03T21:53:30Z

+
+ @Test
+ void manifestFileAdapterPartitionSpecIdUnsupported() {
+ TrackedFile file =


This test case could be combined with the previous case. That would avoid creating a nearly identical tracked file that leaves the reader wondering what changed between the two cases.

Folded into manifestFileAdapterDelegation

rdblue · 2026-09-04T21:49:54Z

 }

+ /** Returns the manifest deletion vector, or null if absent. */
+ default ByteBuffer manifestDeletionVector() {


Originally, I thought that we could come back and update the return type here, but I was wrong because this is a public API interface. So I think rather than using ByteBuffer as a placeholder we should introduce a type to return.
That means we need to decide on a name and whether we want a generic interface for all bitmaps/DVs or if we want one for the manifest/embedded use case. We already have DeletionVector that tracks DV metadata, and PositionDeleteIndex for DVs that we've loaded into memory.
I don't think we want to reuse PositionDeleteIndex because it's mutable and represents a bitmap that could have positions that are in the range of longs. That's also in core so moving it to API to use it here would be a bigger change.
Instead, I think we should introduce EmbeddedBitmap or ManifestBitmap (suggested by @anoopj) and use int for positions. Using a name based on "bitmap" makes it distinct from DeletionVector. Here's a minimal read-only interface that I think we can use:
packageorg.apache.iceberg; importjava.nio.ByteBuffer; /** Interface for small bitmaps that are serialized in metadata files. */publicinterfaceEmbeddedBitmap { /** Number of bits set in this bitmap. */intcardinality(); /** Return whether the bit at {@code position} is set. */booleanisSet(intposition); /** Return the serialized bitmap as a {@link ByteBuffer}. */ByteBufferbuffer(); }
We can also make that smaller if we are concerned about releasing any of those methods. All we really need to decide is the name, but I think we are definitely targeting the int position range so this makes sense to me.

Sounds great. I went with ManifestBitmap, but open to changing.

rdblue · 2026-09-04T21:58:01Z

+
+ @Override
+ public ByteBuffer manifestDeletionVector() {
+ return file.manifestInfo().dv();


For now, I think we should replace this with a placeholder implementation or null:
returnnewEmbeddedBitmap() { @Overridepublicintcardinality() { thrownewUnsupportedOperationException("Bitmap decoding has not been implemented"); } @OverridepublicbooleanisSet(intposition) { thrownewUnsupportedOperationException("Bitmap decoding has not been implemented"); } @OverridepublicByteBufferbuffer() { returnfile.manifestInfo().dv(); } };
I'd be fine returning null for now. Either way, once this is in we will need to get the bitmap implementation in and update the V4ManfiestReader to use it.

Went with the placeholder implementation.

rdblue · 2026-09-04T22:02:24Z

+ new TrackingStruct(
+ EntryStatus.ADDED,
+ SNAPSHOT_ID,
+ // data and file sequence numbers must be equal


Minor: I don't think this is always true. They can be equal. But I think in the case when you add a new column file, the data sequence number gets updated, but the file sequence number would not.

Good point. Fixed.

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

+ .addedFilesCount(3)
+ .existingFilesCount(5)
+ .deletedFilesCount(2)
+ .replacedFilesCount(0)


@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.

rdblue

This looks good to me. Tests are about ready but I made a couple minor suggestions. The only blocker is introducing an interface for the bitmap since this is returning through a public API (this comment: https://github.com/apache/iceberg/pull/17932/changes#r3938099334)

github-actionsBot added API core labels Sep 2, 2026

anoopj commented Sep 2, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

anoopj commented Sep 2, 2026
View reviewed changes

gaborkaszab reviewed Sep 3, 2026
View reviewed changes

PR feedback
050f90b

anoopj requested a review from gaborkaszab September 3, 2026 16:54

rdblue reviewed Sep 3, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

rdblue reviewed Sep 3, 2026
View reviewed changes

anoopj added 2 commits September 4, 2026 13:01

Fix PR feedback
cf93c17

Merge remote-tracking branch 'upstream/main' into manifestfile-adapter-2
7f15c05

anoopj requested a review from rdblue September 4, 2026 20:29

rdblue reviewed Sep 4, 2026
View reviewed changes

rdblue requested changes Sep 4, 2026
View reviewed changes

Introduce ManifestBitmap interface
f707d7b

anoopj requested a review from rdblue September 4, 2026 23:09

tweak comment
133f3c5

rdblue approved these changes Sep 5, 2026
View reviewed changes

rdblue merged commit 3a68b70 into apache:mainSep 5, 2026
43 checks passed

anoopj added this to V4: metadata tree Sep 5, 2026

anoopj moved this to Done in V4: metadata tree Sep 5, 2026

anoopj mentioned this pull request Sep 5, 2026
v4: add wrapper for ManifestFile #16227
Closed
3 tasks

Uh oh!

Conversation

anoopj commented Sep 2, 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!

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!

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!

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

rdblueSep 4, 2026•
edited
Loading

rdblueSep 4, 2026•
edited
Loading