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

gaborkaszab · 2026-06-29T10:09:39Z

Thanks for taking a look @stevenzwu and @anoopj ! I believe all your comments so far have been addressed.

@rdblue Would you mind taking a look? The PartitionData vs StructType conversion questions aren't concluded, and I'm a bit unsure with the partition(PartitionSpec spec, StructLike newPartition) setter function.

gaborkaszab · 2026-06-29T11:23:24Z

Note, I added an extra check that setting partition is not allowed for manifest entries. Let me know if I got this requirement wrong. Once the "optional partition" PR is merged, some of the expected values in the builder tests will be null instead of empty partition.

dramaticlly · 2026-06-29T17:05:58Z

+      Preconditions.checkArgument(
+          deletionVector.cardinality() < newDeletionVector.cardinality(),
+          "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+          deletionVector == null ? null : deletionVector.cardinality(),


is deletionVector nullable here? I think line 236 ensure only the nonnull will lands here? So we can use deletionVector.cardinality() directly

also noticed there's similar flag before in #16964 (comment), but drop the redundant null-ternary

Suggested change

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

deletionVector.cardinality(),

in the errorMessages seem to pass all UTs

you're right. Now that I moved the Precondition into the "is not null" if, the ternary is not needed anymore.

dramaticlly · 2026-06-29T21:49:17Z

    Preconditions.checkArgument(recordCount != null, "Missing required field: record count");
    Preconditions.checkArgument(
        fileSizeInBytes != null, "Missing required field: file size in bytes");
-    Preconditions.checkArgument(partitionData != null, "Missing required field: partition data");


curious, do we need anything to help check if forgot to set the partition spec.

Something like

Preconditions.checkArgument( isLeafManifest(contentType) || specId != null, "Missing required field: spec ID");

I think it's valid not to set partition if the data file is unpartitioned. See my other PR that sets partition optional in the schema for exactly this.

dramaticlly · 2026-06-29T21:50:02Z

+            .fileFormat(FileFormat.PARQUET)
+            .recordCount(2000L)
+            .fileSizeInBytes(12345L)
+            .partition(unpartitioned, null)


I think if we omit this line .partition(unpartitioned, null), build will succeed as well without spec id

it will, I verify here that specId could be null if the PartitionSpec is unpartitioned. Alternatively, we could omit calling partition on the builder, but this test is not for that purpose.

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

Open

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

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

gaborkaszab force-pushed the main_trackedfilebuilder_adjustments branch from 116b838 to a0f84d3 Compare June 29, 2026 10:04

gaborkaszab force-pushed the main_trackedfilebuilder_adjustments branch from a0f84d3 to 36cf104 Compare June 29, 2026 11:21

gaborkaszab mentioned this pull request Jun 29, 2026

Core: Basic fields and schemas for column files #16285

Open

dramaticlly reviewed Jun 29, 2026

View reviewed changes

stevenzwu moved this to In review in V4: metadata tree Jun 29, 2026

dramaticlly reviewed Jun 29, 2026

View reviewed changes

Core: Adjustments for TrackedFileBuilder

7ef4aeb

gaborkaszab force-pushed the main_trackedfilebuilder_adjustments branch from 36cf104 to 7ef4aeb Compare June 30, 2026 07:39

gaborkaszab mentioned this pull request Jun 30, 2026

Core: Add setter for firstRowId in V4 metadata #16849

Open

dramaticlly approved these changes Jun 30, 2026

View reviewed changes

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab commented Jun 29, 2026

Uh oh!

gaborkaszab commented Jun 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dramaticlly Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

dramaticlly Jun 29, 2026 •

edited

Loading