Core: Refactor v4 struct builders to improve validation by anoopj · Pull Request #16408 · apache/iceberg

anoopj · 2026-05-19T00:26:30Z

This is a follow-up to last round of feedback on #16092 Improves the validation in the v4 struct builders to do early errors guide the state transition.

anoopj · 2026-05-20T16:59:01Z


-  static Builder builder() {
-    return new Builder();
+  /** Creates a builder for a newly added file in the given snapshot. */


Should we have a replaced() builder also?

Good question. I think that we probably do since the caller is going to create one corresponding to every MODIFIED entry. We should probably also have a modified builder.

Added a replaced() builder. I'll add MODIFIED, corresponding builder etc as followup

rdblue · 2026-05-21T23:17:51Z

+    private Builder(EntryStatus status, Tracking source, Long snapshotId) {
+      Preconditions.checkArgument(
+          source.dataSequenceNumber() != null,
+          "Source tracking has no data sequence number; cannot mark as %s",


What about Invalid tracking source: data sequence number is null? I don't think that we need to know what was intended for the new status. The main issue is that the source tracking was not read from a manifest (or some other issue leading to missing sequence numbers).

Agree. Done.

rdblue · 2026-05-21T23:21:56Z

+          source.fileSequenceNumber() != null,
+          "Source tracking has no file sequence number; cannot mark as %s",
+          status);
+      this.status = status;


Can we verify status with a private checkStatus(EntryStatus from, EntryStatus to)?

Allowed transitions are:

ADDED to EXISTING, MODIFIED, DELETED, and REPLACED

EXISTING to MODIFIED, DELETED, or REPLACED

MODIFIED to EXISTING, DELETED, or REPLACED

I don't think we would allow transitions from DELETED and REPLACED, or to ADDED.

Done. Added a TODO to handle MODIFIED. Another question is whether we should allow EXISTING to EXISTING. The current code blocks it, but we might need it for manifest compaction?

It depends on whether we want to build a new tracking for every written file or not. There's a good argument that we do want to so that we can remove things that don't need to be carried forward like the deleted and replaced bitmaps. But EXISTING entries probably don't need to be updated (just MODIFIED entries). It may still be cleaner to build a new one every time though.

This seems fine for now.

rdblue · 2026-05-21T23:22:09Z

-    Builder fileSequenceNumber(Long sequenceNumber) {
-      this.fileSequenceNumber = sequenceNumber;
-      return this;
+    private Builder(EntryStatus status, Tracking source, Long snapshotId) {


Isn't snapshotId always known?

snapshotId is not allowed to be passed for EXISTING or MODIFIED because those inherit the snapshot ID from source. That means we should add another constructor for this case. I know that the current existing(...) method passes source.snapshotId(), but that moves logic from the builder into the builder factory methods. I would prefer to keep it all in the builder.

I don't have a preference for whether the new constructor calls this after verifying status or if it is a different constructor.

Another option is to always pass the current operation's snapshot. Then we could use it to automatically set dvSnapshotId (and later, columnUpdateSnapshotId) when this is configured to have changed the DV or column set.

I kind of like this idea because it puts more of the correctness work on this builder.

For now, I added a new constructor. I like the idea of passing the current operation's snapshot, but probably will tackle when we add MODIFIED.

rdblue · 2026-05-21T23:23:28Z

+      this.snapshotId = snapshotId;
+      this.dataSequenceNumber = source.dataSequenceNumber();
+      this.fileSequenceNumber = source.fileSequenceNumber();
+      this.firstRowId = source.firstRowId();


This is also always non-null for data and manifest files. We should consider adding something to signal that intent, but I think it is correct for now not to verify it is non-null.

rdblue · 2026-05-21T23:25:54Z

+      this.fileSequenceNumber = source.fileSequenceNumber();
+      this.firstRowId = source.firstRowId();
+      this.dvSnapshotId = source.dvSnapshotId();
+      ByteBuffer sourceDeleted = source.deletedPositions();


These DVs (both deleted and replaced) should never be carried forward into the next Tracking object. They represent the positions of a manifest that were deleted (or, similarly, replaced) in the current commit. Subsequent commits clear these so they can be set for those commits.

That makes sense. Fixed, and added a test testSourceDvPositionsAreNotCarriedForward()

rdblue · 2026-05-21T23:38:07Z

-
    Builder deletedPositions(ByteBuffer positions) {
+      Preconditions.checkState(
+          status != EntryStatus.DELETED, "Cannot set deleted positions on a DELETED entry");


Deleted or replaced positions are only valid for manifest files that have been changed. That means that ADDED is also not allowed here. When the status is DELETED or REPLACED, the tracking does not need to include these bitmaps (even if the copied entry did) so I would also not allow them for those statuses.

If the status is EXISTING then it must not have changed, so it also doesn't allow these bitmaps. This one is slightly different though, because we could choose to automatically detect and set EXISTING or MODIFIED based on changes to the status. If a DV is updated by calling dvSnapshotId then we could change the status to MODIFIED, or if these MDV methods are called we could also update the status to MODIFIED.

Lastly, this is used only for manifests and dvSnapshotId is used only for data files, so we should ensure that they are not set on the same builder.

Done. I added some TODOs to handle MODIFIED.

anoopj · 2026-05-26T15:07:35Z

-    Builder dv(byte[] buffer) {
-      this.dv = buffer;
+      Preconditions.checkArgument(buffer != null, "Invalid DV: null");
+      this.dv = ByteBuffers.toByteArray(buffer);


This is not guaranteed to be a deep copy, but consistent with BaseFile, and it would be unusual for DV arrays to be mutated

rdblue · 2026-05-26T22:33:25Z

-      this.fileSequenceNumber = sequenceNumber;
-      return this;
+    // TODO: extend allowed transitions once MODIFIED status is added.
+    private static void checkStatus(EntryStatus from, EntryStatus to) {


I think the most readable way to write this method is to have a Precondition.checkState for each from status. That will allow us to update it easily and it also makes more sense when you're trying to figure out what is valid.

anoopj · 2026-06-02T19:51:15Z

@stevenzwu Thanks for the review! I've updated the PR based on your feedback.

(follow-up to apache#16092)

stevenzwu · 2026-06-02T21:01:23Z

    Builder dvCardinality(long cardinality) {
      Preconditions.checkArgument(
-          cardinality > 0, "Invalid DV cardinality: %s (must be positive)", cardinality);
+          cardinality >= 0, "Invalid DV cardinality: %s (must be >= 0)", cardinality);


I am actually wondering if it needs to be strictly positive. Does it make much sense to add an inline manifest DV with zero bits set? just leave the dv and dvCardinality as null in this case.

We shouldn't worry about this. The field is going to be removed.

stevenzwu · 2026-06-02T22:17:18Z

@@ -210,38 +427,85 @@ void testProjectedStructLike() {
  }


While we're on the projection / set path: TrackingStruct.internalSet silently ignores unknown ordinals to support reading newer formats (the default branch with the "must be from a newer version" comment). That forward-compat behavior has no test in any of the three structs (TrackingStruct, ManifestInfoStruct, DeletionVectorStruct). One regression test that calls set with an out-of-range ordinal and asserts no throw and no observable state change would protect the contract.

Added test covering the forward-compat in all 3 structs.

gaborkaszab · 2026-06-03T09:49:36Z

+    validateStatusTransition(source.status(), to);
+    return new TrackingStruct(
+        to,
+        newSnapshotId,


nit: this is against the description of Tracking.SNAPSHOT_ID that says Snapshot ID where the file was added or deleted. In fact, I think we should include REPLACED and MODIFIED too in the description based on the design here.

gaborkaszab · 2026-06-03T10:54:37Z

+
+  @Test
+  void testSourceBuildersRejectSourceWithoutSequenceNumbers() {
+    Tracking missingBoth = TrackingBuilder.added(42L).build();


Let me know what I miss here: Isn't it valid to have Tracking with ADDED status and null sequence numbers? That case the design doc says we'll inherit the sequence numbers. This test says that we can't use such a Tracking as a source in the builder.
What am I missing here?

After giving this some extra thought: The 'source' is a result of reading the manifest through a ManifestReader, that I assume will read the sequence number from disk (null if ADDED) and if null it inherits the sequence number from the manifest before setting it to Tracking. Is this right? Then the 'source' won't have null as sequence numbers.

gaborkaszab · 2026-06-03T11:02:20Z

+
+  @Test
+  void testInternalSetIgnoresUnknownOrdinal() {
+    TrackingStruct tracking = new TrackingStruct(Tracking.schema());


nit: use sourceTracking + set the extra fields?

I feel current code is fine since it tests internalSet anyway. I am going to merge this PR. we can follow up on this later if needed

stevenzwu · 2026-06-03T18:30:07Z

Thanks @anoopj for the contribution, and @rdblue and @gaborkaszab for the review

rdblue · 2026-06-03T20:20:41Z

Thanks for reviewing and merging, @stevenzwu!

anoopj marked this pull request as draft May 19, 2026 00:26

github-actions Bot added the core label May 19, 2026

anoopj force-pushed the v4-struct-builder-fixes branch 2 times, most recently from 443982e to 2a42fb0 Compare May 19, 2026 16:49

anoopj mentioned this pull request May 19, 2026

Core: Add builders for v4 structs #16092

Merged

anoopj force-pushed the v4-struct-builder-fixes branch 3 times, most recently from 8570d98 to fbbdd10 Compare May 19, 2026 18:25

anoopj marked this pull request as ready for review May 19, 2026 18:25

anoopj changed the title ~~Core: Refactor v4 struct builders for fail-early validation~~ Core: Refactor v4 struct builders to improve validation May 19, 2026

anoopj force-pushed the v4-struct-builder-fixes branch 2 times, most recently from 506f8a5 to b6da983 Compare May 19, 2026 18:48

anoopj added this to V4: metadata tree May 19, 2026

anoopj moved this to In review in V4: metadata tree May 19, 2026

anoopj mentioned this pull request May 19, 2026

Core: Add v4 TrackedFileAdapters to bridge Data/Delete Files #16100

Merged

anoopj force-pushed the v4-struct-builder-fixes branch 3 times, most recently from 261a302 to e6bc278 Compare May 20, 2026 16:58

anoopj commented May 20, 2026

View reviewed changes

rdblue reviewed May 21, 2026

View reviewed changes

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

rdblue reviewed May 21, 2026

View reviewed changes

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

rdblue reviewed May 21, 2026

View reviewed changes

anoopj requested a review from rdblue May 23, 2026 00:24

anoopj commented May 26, 2026

View reviewed changes

rdblue reviewed May 26, 2026

View reviewed changes

anoopj requested a review from stevenzwu June 2, 2026 19:51

anoopj added 10 commits June 2, 2026 13:56

Core: Refactor v4 struct builders for fail-early validation

9f206ff

(follow-up to apache#16092)

Incorporate feedback from Ryan

ff6e9f2

improve tests

d96fe36

Fix test

a938ade

Feedback from Ryan

19a3d08

Restructure v4 Tracking factories and builder

9a82f7d

cleanup

0a03713

Clean up comments per Gabor's feedback

07d90ca

Incorporate feedback

687699b

PR feedback from Steven

012d74d

anoopj force-pushed the v4-struct-builder-fixes branch from c7b4a59 to 012d74d Compare June 2, 2026 20:56

stevenzwu reviewed Jun 2, 2026

View reviewed changes

Rename factory classes to past tense (deleted/replaced)

764c325

stevenzwu reviewed Jun 3, 2026

View reviewed changes

Fix test feedback

a3189e9

anoopj requested a review from stevenzwu June 3, 2026 06:52

gaborkaszab reviewed Jun 3, 2026

View reviewed changes

gaborkaszab approved these changes Jun 3, 2026

View reviewed changes

stevenzwu approved these changes Jun 3, 2026

View reviewed changes

stevenzwu merged commit c646404 into apache:main Jun 3, 2026
53 checks passed

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

anoopj deleted the v4-struct-builder-fixes branch June 3, 2026 18:41

anoopj mentioned this pull request Jun 5, 2026

Core: Add EntryStatus.MODIFIED and TrackingBuilder status derivation #16689

Merged

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

Uh oh!

Conversation

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

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!

Uh oh!

rdblue May 21, 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!

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!

anoopj commented Jun 2, 2026

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!

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!

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!

stevenzwu commented Jun 3, 2026

anoopj commented May 19, 2026 •

edited

Loading

rdblue May 21, 2026 •

edited

Loading