Core: Add EntryStatus.MODIFIED and TrackingBuilder status derivation by anoopj · Pull Request #16689 · apache/iceberg

anoopj · 2026-06-05T08:16:14Z

Add MODIFIED to EntryStatus for entries whose file was modified by
a column update or DV change.
Update TrackingBuilder such that it derives the right status
Added tests

gaborkaszab

Thanks for the addition, @anoopj!
I went through this, seems goon in general, just left some nits.

gaborkaszab · 2026-06-05T12:32:33Z

+      return EntryStatus.ADDED;
+    }
+
+    boolean sameSnapshot = source.snapshotId() != null && source.snapshotId() == newSnapshotId;


Reading this I have the impression that this PR contains 2 different functionalities:

the "sameSnapshot" case for ADDED status

Introduction of MODIFIED and all the transitions to/from
No strong opinion, but I usually prefer keeping a single purpose for my PRs. LMK WDYT

Just for my benefit: What is the use-case for the "sameSnapshot" case? I figure that Tracking is created whenever we create a new TrackedFile. Not sure I see, within the same snapshot where we want to change Tracking again (or recreate the TrackedFile with different Tracking). I probably miss, how this integrates into the big picture.

The added/same snapshot is not a separate scope. This is for the use case we discussed in #16408 where a a writer constructs an ADDED tracking and pipelines a DV attach in the same commit.

Do you mean this conversation? If yes, it was about whether to return Tracking or TrackingBuilder from added(long). The argument was to return the latter, because we might want to add some DVs on top using the same builder.
However, here it seems slightly different: We first build a Tracking object with ADDED status, then use it as a source to add DVs on top using another builder. For this, we could use Tracking as a return value from added(long)
(I'll think this through tomorrow again)

rdblue · 2026-06-05T16:47:16Z

-  REPLACED(3);
+  /**
+   * Non-live entry recording that a prior file version was superseded by another live entry. Added
+   * in v4.


We need a term that is better than "non-live". It's probably implied by "superceded by another live entry", although I think we can improve on that still.

What about this?

The starting (replaced) state of an entry that is modified.

Is old better than starting in this conext?

The old (replaced) state of en entry that is modified. Paired with MODIFIED. Added in v4.

Done. Went with the old/new pairing Steven proposed above.

rdblue · 2026-06-05T16:49:48Z

          "snapshot_id",
          Types.LongType.get(),
-          "Snapshot ID where the file was added or deleted");
+          "Snapshot ID where the file was added, deleted, replaced, or modified");


Have we agreed to modify the snapshot ID for a replaced entry? I thought that we were not going to change replaced entries.

We change the snapshot ID for deleted entries, but not for existing entries so there's precedent both ways. If you're scanning for changes, the snapshot ID is useful for filtering out changes that are left-over from older snapshots. For instance, I may rewrite a manifest and delete a file in it. If I'm later scanning that file for changes, I would be able to check whether the delete entry is for the snapshot ID I'm getting changes for.

The counter-argument is that the manifest would probably only be scanned for changes if you're looking for changes that would match. In order to scan that manifest, you'd first check its snapshot ID (when it was added) and not scan otherwise.

Overall, I think the right thing is to update the snapshot ID as you have here. That way if any implementation reads files it doesn't need to, it has enough information to filter out the entries.

Good to note in the spec @stevenzwu and @amogh-jahagirdar.

TrackingBuilder.terminal() writes newSnapshotId for both DELETED and REPLACED today, so the doc matches the current code.

To me, deleted and replaced entries should behave the same. replaced is just a special flavor of deleted. Since v1-v3 already modify the snapshot ID for a deleted entry, it seems reasonable to maintain the same behavior.

Also for change detection, it is probably better to update the snapshot id in tracking for deleted or replaced entries. If the tracking snapshot_id doesn't match the current snapshot id, the entries should be ignored for change detection. This is important because the current spec is silent on lifetime of deleted entries.

stevenzwu · 2026-06-05T17:22:15Z

          "snapshot_id",
          Types.LongType.get(),
-          "Snapshot ID where the file was added or deleted");
+          "Snapshot ID where the file was added, deleted, replaced, or modified");


TrackingBuilder.terminal() writes newSnapshotId for both DELETED and REPLACED today, so the doc matches the current code.

To me, deleted and replaced entries should behave the same. replaced is just a special flavor of deleted. Since v1-v3 already modify the snapshot ID for a deleted entry, it seems reasonable to maintain the same behavior.

Also for change detection, it is probably better to update the snapshot id in tracking for deleted or replaced entries. If the tracking snapshot_id doesn't match the current snapshot id, the entries should be ignored for change detection. This is important because the current spec is silent on lifetime of deleted entries.

stevenzwu · 2026-06-05T17:25:25Z

-  REPLACED(3);
+  /**
+   * Non-live entry recording that a prior file version was superseded by another live entry. Added
+   * in v4.


Is old better than starting in this conext?

The old (replaced) state of en entry that is modified. Paired with MODIFIED. Added in v4.

rdblue · 2026-06-05T20:33:00Z

 class TrackingBuilder {
-  private final EntryStatus status;
-  private final Long snapshotId;
+  // null for the fresh-added path; non-null for the source-based path.


Let's remove this. I'm not sure what it means.

Done. Removed with the revert of the refactor.

rdblue · 2026-06-05T20:35:04Z

  TrackingBuilder deletedPositions(ByteBuffer positions) {
-    Preconditions.checkState(
-        status == EntryStatus.EXISTING, "Cannot set deleted positions on %s entry", status);
+    Preconditions.checkState(source != null, "Cannot set deleted positions on ADDED entry");


The error message should agree with the check. In this case, I think the check is better if you use status == ADDED.

Done. TrackingBuilder now holds status and snapshotId as fields instead of source.

rdblue · 2026-06-05T20:36:15Z

  }

  private TrackingBuilder(long newSnapshotId) {
-    this.status = EntryStatus.ADDED;


I think this should still use status rather than mutated and source. There's no need to keep the source around instead of storing its snapshot ID individually. And we don't want to make inferences like state is added when source is null.

Fixed. (same as above)

rdblue · 2026-06-05T20:38:48Z

  }

+  /** Derives the output status from the source, the snapshot, and any mutations. */
+  private EntryStatus deriveStatus() {


I think that this should go back to updating status in the builder config methods rather than this. The impulse to co-locate logic for entry status is good, but I think it is more readable not to do this refactor. When modifications are made, the status can be validated and updated inline. That's simpler and more clear, in my opinion, rather than trying to detect what happened in the configuration phase and produce the correct status.

Done. Reverted the refactor.

Co-authored-by: Steven Zhen Wu <stevenz3wu@gmail.com>

stevenzwu

LGTM. Just a minor comment for Javadoc

gaborkaszab

LGTM, just nits

gaborkaszab · 2026-06-08T13:18:34Z

-  /** Indicates an entry that has been replaced by a column update or DV change. Added in v4. */
-  REPLACED(3);
+  /**
+   * The old (replaced) state of an entry that has been modified. Paired with MODIFIED. Added in v4.


I recall on the last V4 AMT sync there was an idea to make the leaf manifests MODIFIED when adding a new DV, without having a corresponding REPLACED entry. Not sure how far we got with that idea, but it seems to go against the comments here, where the comments seem to be relevant for data entries only.

Manifest entries will not have a paired REPLACED entry to compare against. This statement is still correct, but the one below (that MODIFIED is paired with REPLACED) is misleading.

The current comment state seems right to me and implicitly covers the case of leaf manifests that @gaborkaszab was talking about. REPLACED is paired with MODIFIED, but MODIFIED is not neccessarily paired with REPLACED (don't need to explicitly say that last part).

gaborkaszab · 2026-06-08T13:40:33Z

-    assertThat(tracking.isLive()).isFalse();
+    assertThat(TrackingBuilder.added(42L).build().isLive()).isTrue();
+    assertThat(TrackingBuilder.from(source, 999L).build().isLive()).isTrue();
+    assertThat(TrackingBuilder.from(source, 999L).dvUpdated().build().isLive()).isTrue();


nit: maybe add an assert on the status of the entry too to make it easier for the readers to understand which status is live and which one isn't?

Removed the test after I moved isLive to enum.

rdblue · 2026-06-08T19:48:18Z

+   * The old (replaced) state of an entry that has been modified. Paired with MODIFIED. Added in v4.
+   */
+  REPLACED(3),
+  /** The new (live) state of an entry that has been modified. Paired with REPLACED. Added in v4. */


Instead of trying to use documentation to handle live status, why not just have a isLive() method on this enum?

rdblue · 2026-06-08T19:50:40Z

-    return status() == EntryStatus.ADDED || status() == EntryStatus.EXISTING;
+    return status() == EntryStatus.ADDED
+        || status() == EntryStatus.EXISTING
+        || status() == EntryStatus.MODIFIED;


As I said above, we should probably just add isLive() to EntryStatus. Then we can delegate here.

Added it to EntryStatus and removed the method since it's no longer needed here.

Maybe for convenience we can still have Tracked.isLive() that simply delegates to EntryStatus as Ryan suggested:

default boolean isLive() { return status().isLive(); }

I was not sure if we want another method while we can just call tracking.status().isLive(), so I removed it. Added it back.

rdblue · 2026-06-08T19:54:52Z


+  /**
+   * Promotes an EXISTING entry to MODIFIED on mutation. Fresh-add builders (status = ADDED) are
+   * preserved — covers the same-commit append + DV attach case without a special branch.


AI artifacts like — are a red flag for me that indicates the comment was not edited for correctness and clarity. Could you revise this?

This was added as a review suggestion. I applied it through Github without editing it. We have removed the method, so we don't need it anymore.

rdblue · 2026-06-08T19:57:43Z

+   * preserved — covers the same-commit append + DV attach case without a special branch.
+   */
+  private void promoteToModified() {
+    if (status == EntryStatus.EXISTING) {


I don't understand this check. If the status is ADDED, would we not update to MODIFIED? I know that ADDED will never get here, but that's half the point: this assumes status checks elsewhere. Why split status handling in multiple places? This also implies that it's perfectly fine to call this on a DELETED entry, which I also do not expect.

The code here is not worth the confusion over why this logic is here. This is two lines (and snapshotId should not be modified).

if the status is ADDED (the same-commit append + DV attach case), we want to keep the status as ADDED.

Flink streaming writer can hit this scenario of adding a new data file with DV attached.

if the status is ADDED (the same-commit append + DV attach case), we want to keep the status as ADDED.

Yes, my point is really that this is dividing the logic of how status evolves across multiple places and the only benefit is not copying one line of code? I don't think this method is a good idea.

Got your point now. You are suggesting just removing this method and directly code the status change in the mutation method like dvUpdated, deletedPositions, and replacedPositions. That makes sense.

Thanks for the feedback/discussion! Incorporated.

rdblue · 2026-06-08T19:58:44Z

+  private void promoteToModified() {
+    if (status == EntryStatus.EXISTING) {
+      this.status = EntryStatus.MODIFIED;
+      this.snapshotId = newSnapshotId;


Why is this setting the snapshot ID of a MODIFIED entry? The modification should set a specific snapshot ID, like the dvSnapshotId. We don't set the entry's snapshot ID when it is changed because we still want to know when the base file itself was added.

good catch.

We also discussed offline that the deletePositions and replacedPositions should also advance the dvSnapshotId to the newSnapshotId. both are only used for leaf manifest entries in the root manifest file.

We are unlikely to compute and set them for data file entries in the root or leaf manifest files. Since data DVs are stored in external Puffin files, it is too expensive to compute and store the delta deletedPositions.

In the future, when column files metadata are added, we will have new transition/mutation APIs and new latestColumnFileSnapshotId.

Thank you. Incorporated. I removed the method and added tests to validate dvSnapshotId

gaborkaszab

Thank you @anoopj !
LGTM, just left some minor comments.

gaborkaszab · 2026-06-10T14:32:29Z

Thanks for the quick turnaround, @anoopj !
I think this comment was unnoticed and there is an open question on Tracking.isLive().
Other than these, LGTM.

rdblue · 2026-06-10T20:26:08Z

    Preconditions.checkState(
-        status == EntryStatus.EXISTING, "Cannot set deleted positions on %s entry", status);
-    // DV applies to data files; deleted positions apply to manifest files
-    Preconditions.checkState(


Why was this state check removed? Is it no longer valid?

the dvSnapshotId == null check is removed.

the other check is updated from status == EntryStatus.EXISTING to status != EntryStatus.ADDED. The change seems correct to me.

I don't think we can checkdvSnapshotId == null anymore, because both deletedPositions() and replacedPositions() set it. We wouldn't be able to set both of them through the same builder.
When building a TrackedFile (I'm putting together a PR for that) we can enforce that deleted and replaced positions are only valid for manifests.

Anyway, setting dvSnapshotId for these is a new behavior, not entirely related to introducing the MODIFIED status. Was this discussed somewhere that I missed? I checked both the spec PR and the proposal doc, and they say that dvSnapshotId is when the deletion_vector is added, they don't mention deletedPositions o replacedPositions.
Update: I see the comment for this, so it's intentional. Would be nice to document it, cc @amogh-jahagirdar

My question: These deleted and replaced positions aren't carried over when creating a new Tracking, and by definition are relevant only for the current snapshot (Bitmap of positions deleted in this snapshot). Is there any benefit setting dvSnapshotId for them?
Additionaly, we carry over dvSnapshotId when creating from source, but we don't the positions.

I don't think we can checkdvSnapshotId == null anymore, because both deletedPositions() and replacedPositions() set it.

Thanks, I think this is the answer I was looking for.

Capture rdblue's instruction from PR #16689: Javadoc should describe a class or method's purpose as if it were defined by an interface, not restate or leak implementation details that create churn when the implementation changes. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

amogh-jahagirdar

Overall looks good to me after all the recent updates, just noticed some typos on the docs

amogh-jahagirdar · 2026-06-11T00:12:52Z

-  /** Indicates an entry that has been replaced by a column update or DV change. Added in v4. */
-  REPLACED(3);
+  /**
+   * The old (replaced) state of an entry that has been modified. Paired with MODIFIED. Added in v4.


The current comment state seems right to me and implicitly covers the case of leaf manifests that @gaborkaszab was talking about. REPLACED is paired with MODIFIED, but MODIFIED is not neccessarily paired with REPLACED (don't need to explicitly say that last part).

rdblue · 2026-06-11T21:22:22Z

+    TrackingStruct tracking = new TrackingStruct(Tracking.schema());
+    tracking.set(STATUS_ORDINAL, EntryStatus.MODIFIED.id());
+    tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, 5L);
+    tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, 6L);


Because these are set, this wouldn't be inherited even if MODIFIED inherited the values.

rdblue · 2026-06-11T21:23:09Z


  @Test
-  void testAddedBuilder() {
+  void testAddedWithSameCommitDvStaysAdded() {


Style: Abbreviations should not be converted to CamelCase in the project. DV should stay DV in names.

rdblue

This seems about ready to me.

Core: Add EntryStatus.MODIFIED and TrackingBuilder status derivation

dc1bd24

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

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

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

anoopj mentioned this pull request Jun 5, 2026

Add MODIFIED status anoopj/iceberg#4

Closed

3 tasks

gaborkaszab reviewed Jun 5, 2026

View reviewed changes

Feedback from Gabor

78a686b

anoopj requested a review from gaborkaszab June 5, 2026 14:31

rdblue reviewed Jun 5, 2026

View reviewed changes

stevenzwu reviewed Jun 5, 2026

View reviewed changes

rdblue reviewed Jun 5, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackingStruct.java Outdated

stevenzwu reviewed Jun 5, 2026

View reviewed changes

anoopj and others added 2 commits June 6, 2026 07:00

Update core/src/main/java/org/apache/iceberg/EntryStatus.java

63d831d

Co-authored-by: Steven Zhen Wu <stevenz3wu@gmail.com>

Incorporate PR feedbak

40f4588

anoopj requested review from rdblue and stevenzwu June 6, 2026 03:31

stevenzwu approved these changes Jun 6, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackingBuilder.java Outdated

stevenzwu mentioned this pull request Jun 6, 2026

v4: add manifest write support #16694

Open

3 tasks

gaborkaszab reviewed Jun 8, 2026

View reviewed changes

rdblue reviewed Jun 8, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackingBuilder.java

rdblue reviewed Jun 8, 2026

View reviewed changes

anoopj force-pushed the v4-modified-status branch from cd0e558 to 446f24e Compare June 9, 2026 15:17

anoopj requested a review from rdblue June 9, 2026 15:43

stevenzwu reviewed Jun 9, 2026

View reviewed changes

PR feedback

31cf256

anoopj requested a review from stevenzwu June 10, 2026 02:14

stevenzwu approved these changes Jun 10, 2026

View reviewed changes

gaborkaszab reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackingBuilder.java Outdated

gaborkaszab reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestEntryStatus.java Outdated

gaborkaszab reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackingStruct.java Outdated

Comment thread core/src/test/java/org/apache/iceberg/TestTrackingStruct.java Outdated

Comment thread core/src/test/java/org/apache/iceberg/TestTrackingStruct.java Outdated

More feedbak

536dcb8

anoopj requested a review from gaborkaszab June 10, 2026 13:37

add a delegated isLive() to Tracking

6a8ee95

gaborkaszab approved these changes Jun 10, 2026

View reviewed changes

rdblue reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/EntryStatus.java Outdated

rdblue reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/Tracking.java Outdated

rdblue reviewed Jun 10, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackingBuilder.java Outdated

rdblue reviewed Jun 10, 2026

View reviewed changes

stevenzwu mentioned this pull request Jun 10, 2026

Docs: Add Javadoc guidance to AGENTS.md #16764

Merged

1 task

PR feedback

7415213

amogh-jahagirdar reviewed Jun 11, 2026

View reviewed changes

Remove commment

d5eca25

rdblue reviewed Jun 11, 2026

View reviewed changes

rdblue approved these changes Jun 11, 2026

View reviewed changes

rdblue merged commit cc7fec9 into apache:main Jun 11, 2026
53 checks passed

github-project-automation Bot moved this from In review to Done in V4: metadata tree Jun 11, 2026

anoopj deleted the v4-modified-status branch June 13, 2026 02:27

nssalian added this to the Iceberg 1.12.0 milestone Jun 23, 2026

Uh oh!

Conversation

anoopj commented Jun 5, 2026

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

rdblue Jun 5, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

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!

rdblue Jun 5, 2026 •

edited

Loading

rdblue Jun 8, 2026 •

edited

Loading

anoopj Jun 9, 2026 •

edited

Loading