Core: Adjustments for TrackedFileBuilder by gaborkaszab · Pull Request #16964 · apache/iceberg

gaborkaszab · 2026-06-25T18:59:44Z

Original PR: #16769
This one is a follow-up with adjustments addressing further comments.

gaborkaszab

Carried over the unclosed comments from the original PR + some random extra explanation

gaborkaszab · 2026-06-25T19:08:39Z


  // optional fields
  private Integer specId = null;
+  private PartitionData partition = null;


carry-over comment: "PartitionData should be StructLike. We don't want to rely on concrete classes here."

TrackedFileStruct constructor seem to accept PartitionData so we have to do the conversion at some point. Seemed straightforward to keep the same type in the builder as expected by the constructed class.

gaborkaszab · 2026-06-25T19:09:37Z

-  TrackedFileBuilder partition(PartitionData newPartitionData) {
-    Preconditions.checkArgument(newPartitionData != null, "Invalid partition: null");
-    this.partitionData = newPartitionData;
+  TrackedFileBuilder partition(PartitionSpec spec, StructLike newPartition) {


carry-over comment: "I think that this should be combined with specId: partition(int newSpecId, StructLike partitionTuple). If the tuple is set, then spec ID is required so that we know how to interpret it. I suspect that this should also be responsible for wrapping the partition tuple with a projection to the union type."

carry-over answer: "I probably miss something here, but I see that TrackedFile has a PartitionData member. If we want to produce that, is specId and partitionTuple enough? Don't we need a PartitionSpec to produce PartitionData?"

gaborkaszab · 2026-06-25T19:10:22Z

    this.recordCount = source.recordCount();
    this.fileSizeInBytes = source.fileSizeInBytes();
-    this.partitionData = (PartitionData) source.partition();
+    this.partition = (PartitionData) source.partition();


carry-over comment: "We should not need unchecked casts like this."

carry-over answer: "
The difficulty here is the TrackedFile.partition() return StructLike while the TrackedFileStruct constructor accepts PartitionData. Since internally in TrackedFile we also have PartitionData this seemed safe"

Casting partition() to PartitionData where the concrete type is actually needed is already the established pattern in core. For example, ManifestFilterManager:

PartitionData partition = (PartitionData) file.partition();

We can't narrow TrackedFile.partition() to return PartitionData to avoid the cast either. Every file and scan type in the codebase exposes partition() as StructLike — ContentFile, BaseFile, V1Metadata/V2Metadata, PartitionScanTask, ContentScanTask, etc.

gaborkaszab · 2026-06-25T19:11:34Z

  private static final PartitionData PARTITION = partition("books");

  // Tracking field ordinals, looked up from the schema so the tests do not hard-code offsets.
+  private static final int STATUS_ORDINAL = ordinalOf(Tracking.schema(), "status");


I reverted the part of the test that constructs Tracking to the state before the builder, because that seemed independent of this change. Hence these show as appeared, but in fact restored

stevenzwu · 2026-06-25T21:43:39Z

    this.recordCount = source.recordCount();
    this.fileSizeInBytes = source.fileSizeInBytes();
-    this.partitionData = (PartitionData) source.partition();
+    this.partition = (PartitionData) source.partition();


Casting partition() to PartitionData where the concrete type is actually needed is already the established pattern in core. For example, ManifestFilterManager:

PartitionData partition = (PartitionData) file.partition();

We can't narrow TrackedFile.partition() to return PartitionData to avoid the cast either. Every file and scan type in the codebase exposes partition() as StructLike — ContentFile, BaseFile, V1Metadata/V2Metadata, PartitionScanTask, ContentScanTask, etc.

anoopj · 2026-06-25T22:18:45Z

    Preconditions.checkArgument(
-        this.deletionVector == null || !this.deletionVector.equals(newDeletionVector),
-        "The same deletion vector already added");
+        deletionVector == null || deletionVector.cardinality() < newDeletionVector.cardinality(),


I don't believe the spec requiring increasing cardinality. A writer could reserialize the DVs into a new puffin blob. Should we allow equal cardinality?

this is a very interesting scenario, like Puffin file compaction. It may not be useful to compact DV Puffin files due to the access pattern to DV blobs.

Additional context: This was the original comment.
The intention was to prevent adding the same DV through the builder, because that bumps the dv_snapshot_id while not changing the DV itself. I first introduced an override for equals in DeletionVectorStruct but we decided to avoid such overrides.
As a workaround @rdblue suggested that we can "mimic" equality checks here by requiring 2 things:

if DV cardinality doesn't increase than we probably add the same DV again

if cardinality increased, that's great, but expectation is that we don't write the new DV in-place.

I haven't thought about DV compaction, seems something we might want to discuss in the future. If we decide to support it, we can still remove/restructure the conditions here. WDYT?

+1 to keep the stricter validation for now and relax in the future if needed.

anoopj · 2026-06-25T22:21:08Z

+        deletionVector == null
+            || !deletionVector.location().equals(newDeletionVector.location())
+            || deletionVector.offset() != newDeletionVector.offset(),
+        "Invalid DV update: same location and offset has changed cardinality");


This is moot if we allow equal cardinality, but the error message part about cardinality is dependent on prior precondition check. It can be a bit confusing.

I agree. Simplified the message here, and don't mention cardinality

anoopj · 2026-06-25T22:22:20Z

+    if (spec.isUnpartitioned()) {
+      Preconditions.checkArgument(
+          newPartition == null, "Invalid partition: must be null for unpartitioned spec");
+      this.specId = spec.specId();


Minor: this is in both branches of the if. Hoist it out?

thx, was going to, but then I forgot :)

anoopj · 2026-06-25T22:23:29Z

-        "The same deletion vector already added");
+        deletionVector == null || deletionVector.cardinality() < newDeletionVector.cardinality(),
+        "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+        deletionVector == null ? null : deletionVector.cardinality(),


Suggested change

deletionVector == null ? null : deletionVector.cardinality(),

deletionVector.cardinality(),

ternary is not needed because your precondition check already validates null

That's what I thought too, but apparently these Preconditions parameters aren't lazily evaluated. Without the ternary every test that wanted to set a DV gave NPE on this.

stevenzwu · 2026-06-26T06:23:32Z

 class TrackedFileBuilder {
-  private final long snapshotId;
  private final FileContent contentType;
+  private final TrackingBuilder trackingBuilder;


I can align my PR with this new shape to avoid the individual setters that you commented.
#16936

technically, nothing changes from the user's perspective, it's just how we create Tracking internally.

gaborkaszab

Thanks for taking a look, @stevenzwu @anoopj !

One open question remained from this set of comments: Do we want to allow came dv cardinality for DV compaction?

gaborkaszab · 2026-06-26T10:34:44Z

 class TrackedFileBuilder {
-  private final long snapshotId;
  private final FileContent contentType;
+  private final TrackingBuilder trackingBuilder;


technically, nothing changes from the user's perspective, it's just how we create Tracking internally.

gaborkaszab · 2026-06-26T11:07:42Z

+    if (spec.isUnpartitioned()) {
+      Preconditions.checkArgument(
+          newPartition == null, "Invalid partition: must be null for unpartitioned spec");
+      this.specId = spec.specId();


thx, was going to, but then I forgot :)

gaborkaszab · 2026-06-26T12:09:24Z

    Preconditions.checkArgument(
-        this.deletionVector == null || !this.deletionVector.equals(newDeletionVector),
-        "The same deletion vector already added");
+        deletionVector == null || deletionVector.cardinality() < newDeletionVector.cardinality(),


Additional context: This was the original comment.
The intention was to prevent adding the same DV through the builder, because that bumps the dv_snapshot_id while not changing the DV itself. I first introduced an override for equals in DeletionVectorStruct but we decided to avoid such overrides.
As a workaround @rdblue suggested that we can "mimic" equality checks here by requiring 2 things:

if DV cardinality doesn't increase than we probably add the same DV again

if cardinality increased, that's great, but expectation is that we don't write the new DV in-place.

I haven't thought about DV compaction, seems something we might want to discuss in the future. If we decide to support it, we can still remove/restructure the conditions here. WDYT?

gaborkaszab · 2026-06-26T12:11:45Z

-        "The same deletion vector already added");
+        deletionVector == null || deletionVector.cardinality() < newDeletionVector.cardinality(),
+        "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+        deletionVector == null ? null : deletionVector.cardinality(),


That's what I thought too, but apparently these Preconditions parameters aren't lazily evaluated. Without the ternary every test that wanted to set a DV gave NPE on this.

gaborkaszab · 2026-06-26T12:41:33Z

+        deletionVector == null
+            || !deletionVector.location().equals(newDeletionVector.location())
+            || deletionVector.offset() != newDeletionVector.offset(),
+        "Invalid DV update: same location and offset has changed cardinality");


I agree. Simplified the message here, and don't mention cardinality

stevenzwu · 2026-06-26T16:17:14Z

-        !isLeafManifest(source.contentType()),
-        "Manifest entries cannot transition to REPLACED, but entry type is: %s",
-        source.contentType());
+        !isLeafManifest(source.contentType()), "Cannot transition manifest files to REPLACED");


Suggested change

!isLeafManifest(source.contentType()), "Cannot transition manifest files to REPLACED");

!isLeafManifest(source.contentType()), "Cannot transition manifest entries to REPLACED status");

stevenzwu · 2026-06-26T16:35:29Z

    Preconditions.checkArgument(
-        this.deletionVector == null || !this.deletionVector.equals(newDeletionVector),
-        "The same deletion vector already added");
+        deletionVector == null || deletionVector.cardinality() < newDeletionVector.cardinality(),


+1 to keep the stricter validation for now and relax in the future if needed.

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

gaborkaszab mentioned this pull request Jun 25, 2026

Core: Introduce builder for TrackedFile #16769

Merged

gaborkaszab commented Jun 25, 2026

View reviewed changes

stevenzwu added this to V4: metadata tree Jun 25, 2026

stevenzwu reviewed Jun 25, 2026

View reviewed changes

anoopj reviewed Jun 25, 2026

View reviewed changes

stevenzwu reviewed Jun 26, 2026

View reviewed changes

stevenzwu mentioned this pull request Jun 26, 2026

Core: V4 write direction wrappers #16936

Draft

gaborkaszab force-pushed the main_trackedfilebuilder_adjustments branch from 0514210 to 390dc8c Compare June 26, 2026 12:45

gaborkaszab commented Jun 26, 2026

View reviewed changes

Core: Adjustments for TrackedFileBuilder

116b838

gaborkaszab force-pushed the main_trackedfilebuilder_adjustments branch from 390dc8c to 116b838 Compare June 26, 2026 13:27

stevenzwu reviewed Jun 26, 2026

View reviewed changes

stevenzwu approved these changes Jun 26, 2026

View reviewed changes

	deletionVector == null ? null : deletionVector.cardinality(),
	deletionVector.cardinality(),

	!isLeafManifest(source.contentType()), "Cannot transition manifest files to REPLACED");
	!isLeafManifest(source.contentType()), "Cannot transition manifest entries to REPLACED status");

Uh oh!

Conversation

gaborkaszab commented Jun 25, 2026

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!

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants