Core: Basic fields and schemas for column files by gaborkaszab · Pull Request #16285 · apache/iceberg

gaborkaszab · 2026-05-11T13:52:03Z

This change introduces the interface for column files and also integrates it to the schema for TrackedFile.

gaborkaszab · 2026-05-11T16:02:14Z

First piece of the column update work: introducing the basic interface of the column updates files, aka column files
cc @anuragmantri @rdblue @pvary @RussellSpitzer @amogh-jahagirdar @anoopj @nastra

gaborkaszab · 2026-05-21T13:56:27Z

I opened a thread on dev@ to discuss the metadata structs for column files. Once that's finalized, I'll incorporate the changes here.

gaborkaszab · 2026-06-02T13:13:44Z

cc @amogh-jahagirdar @rdblue @anoopj

gaborkaszab · 2026-06-08T12:33:22Z

Adjusted field IDs because 157 is going to be allocated for writer_format_version in this PR.

gaborkaszab

Thanks for the reviews @anuragmantri and @stevenzwu ! I believe I addressed all your comments. Would you mind taking another look?

stevenzwu

LGTM overall

gaborkaszab · 2026-08-24T10:49:41Z

Thanks for the approval @stevenzwu and for the reviews @anuragmantri , @amogh-jahagirdar , @RussellSpitzer!
Are there anything else before we merge this? @rdblue Would you like to take a look yourself too?

Defines the column_file element struct referenced by the column_files field (158) in the v4 content entry, matching the ColumnFile schema added in apache#16285. Co-authored-by: Gabor Kaszab <gaborkaszab@gmail.com> Co-authored-by: Anurag Mantripragada <amantripragada@apple.com>

Makes the column_files field type list<159: column_file> to match the inline element-id convention used by other list fields in the content entry, matching apache#16285. Co-authored-by: Gabor Kaszab <gaborkaszab@gmail.com> Co-authored-by: Anurag Mantripragada <amantripragada@apple.com>

laskoviymishka

Nice work on this. The ColumnFile / ColumnFileStruct split fits the existing DeletionVector / Tracking pattern, and the schema wiring looks consistent with the rest of the file.

I’d still hold the merge on one thing: columnFilesUpdated() resets dataSequenceNumber, and the new inheritFrom() path can then assign a newer value to a MODIFIED entry.

That means attaching a column file can move the data sequence number forward even though the row data did not change. Since delete planning uses that sequence number, this could make existing deletes stop applying and bring deleted rows back.

The comment says this is safe because writers rewrite old deletes to DVs during a column update, but that is not enforced here yet. I think we should either keep the original sequence number for MODIFIED entries, or enforce that precondition here. A short spec note would also help.

A few smaller things:

no manifest round-trip test covers a populated column_files list
internalSet does not copy columnFiles, unlike the constructor and sibling fields
the copy constructor drops null elements instead of preserving the list shape
a couple of small builder-message / javadoc nits inline

The sequence-number behavior is the main one for me. Once that is settled, I’m happy to take another look.

laskoviymishka

LGTM! Nice work in general.

I think we can merge this to unblock further work on column-updates.

gaborkaszab · 2026-10-05T13:42:58Z

+        .toString();
+  }
+
+  static class Builder {


Since ColumnFile is something being passed in by the users through the table API, at some point we would want to make some builder public for them. I'd keep this here as long as it's possible and then I'd introduce something like a ColumnFiles class that can be used to produce the ColumnFile objects, similarly to DataFiles

gaborkaszab · 2026-10-05T13:50:17Z

Latest changes:
Rebased with latest main and resolved conflicts
Renames lates_column_file_snapshot_id to column_file_snapshot_id. Moved it right after dv_snapshot_id and changed the field ID to 8.
Removed split_offsets from ColumnFile

mkroll-db · 2026-10-05T03:51:41Z

+    Builder splitOffsets(List<Long> newSplitOffsets) {
+      Preconditions.checkArgument(newSplitOffsets != null, "Invalid split offsets: null");
+      this.splitOffsets = newSplitOffsets;
+      return this;
+    }


Since this field is optional and null means not present, should this function not just ignore it?
For AVRO this code would currently throw:

builder.splitOffsets(appender.splitOffsets())

and DataFile.Builder.withSplitOffsets handles null.

As far as I remember the offsets need to be sorted. Should the be enforced here?

Suggested change

Builder splitOffsets(List<Long> newSplitOffsets) {

Preconditions.checkArgument(newSplitOffsets != null, "Invalid split offsets: null");

this.splitOffsets = newSplitOffsets;

return this;

}

Builder splitOffsets(List<Long> newSplitOffsets) {

if (newSplitOffsets != null) {

Preconditions.checkArgument(

Ordering.natural().isStrictlyOrdered(newSplitOffsets),

"Invalid split offsets: not sorted");

}

this.splitOffsets = newSplitOffsets;

return this;

}

@mkroll-db did you take a look at the latest state of this PR? split offsets are removed now from column files :)

mkroll-db · 2026-10-05T04:01:26Z

+
+  private int formatVersion = -1;
+  private int[] fieldIds = null;
+  private String location = null;


Side Question: Did we actually formalize the location and naming of column files?

I had a offline discussion with @gaborkaszab and it's not specified on purpose. We'll update the doc accordingly.

Added a comment in the spec doc

Thanks for the comment!

mkroll-db · 2026-10-11T04:55:33Z

+
+    Builder fieldIds(List<Integer> newFieldIds) {
+      Preconditions.checkArgument(newFieldIds != null, "Invalid field IDs: null");
+      Preconditions.checkArgument(!newFieldIds.isEmpty(), "Invalid field IDs: empty");


Small addition to the above comment, without removing the unused columns the we would have unbounded (linear) growth of the metadata. With this we have at most #columns new column files per commit.

stevenzwu · 2026-10-13T05:25:26Z

+    if (toCopy.columnFiles != null) {
+      this.columnFiles = Lists.newArrayListWithCapacity(toCopy.columnFiles.size());
+      for (ColumnFile columnFile : toCopy.columnFiles) {
+        this.columnFiles.add(columnFile != null ? columnFile.copy() : null);


the list element shouldn't be null, right?

Probably I'm over cautious here. On the creation side, we should verify items are non-null, and not here.
Removed the null check here.

stevenzwu · 2026-10-13T05:36:13Z

+  @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+  void serializationRoundTrip(TestHelpers.RoundTripSerializer<ColumnFile> roundTripSerializer)
+      throws IOException, ClassNotFoundException {
+    ColumnFile columnFile =


this column file object can also be a private static constant.

the Comparators.forType(ColumnFile.schema()) can also be a static constant.

I'm hesitant here. I had the impression that in other test suites for V4 we preferred to spell out the creation of the class we test in each test case. I can create a static constant for the ColumnFile that we reuse across tests, but then the tests wouldn't be self-contained.

stevenzwu · 2026-10-13T05:37:38Z

+      Preconditions.checkArgument(newFieldIds != null, "Invalid field IDs: null");
+      Preconditions.checkArgument(!newFieldIds.isEmpty(), "Invalid field IDs: empty");
+      Preconditions.checkArgument(
+          Sets.newHashSet(newFieldIds).size() == newFieldIds.size(),


why not just make the arg a Set?

I'm not sure, because I wanted this to be consistent with equality field IDs, where we pass lists.
But this reminds me that it's not just item uniqueness what matters but I recall we wanted to make this ordered. Enforcing that now.

More general question: This builder is currently used only in tests. Technically we could drop this similarly to other readers. We'll probably need a user-facing builder later on though, so that users can pass ColumnFile objects through table API, so the plan is to move this to a public class in core/ as similarly we have DataFiles we could have ColumnFiles.
The question is, should I drop the builder now, or can this stay until we move this to a public builder class?

Equality field IDs are a list because their order is the column order of the equality-delete file. Column-file field IDs do not have that role.

field_ids identifies which columns are live in the file. Readers map columns by field ID in the file schema, not by position in this list, so field IDs do not need to be ordered.

Since uniqueness is part of the contract, fieldIds() should return a Set and the builder should take a Set. Schema.identifierFieldIds() is the closer precedent: its constructor take a Set and its accessor returns a Set.

Regarding the builder, if it is only used by test code, we can remove it for now and add it later when main code needs it.

I started rewriting this to Set, however, realized that in the schema (also in the spec) we define field IDs as a list. I can change the internal representation to Set, but with getByPos() and internalSet() we still have to return/accept it as a list. I'd find it misleading, TBH. Still leaning towards representing it as a list. WDYT @stevenzwu ?

Physical representation/storage is always list/array. there is no set in the physical layer. But I think the Java API should match the uniqueness contract and use Set. I think the Schema class is making the same choice regarding the identifier field ids.

We can also let others weigh in. @nastra @rdblue @RussellSpitzer

Hmm, I just checked the identifier field ID implementation in Schema: it is provided and returned as Set, however, initially stored as int[] and lazily populated as a Set. It could work here too.
However, ContentFile that is more "internal representation facing", returns them as "List" and the internal representation seem to be int[] in BaseFile without any API returning it as Set. Also BaseFile receives them as int[].

I still think that with ColumnFile we should be following BaseFile/ContentFile instead of Schema, where we represent it as int[] we output it as List and receive it as either List or int[]. This also suits getByPos() and internalSet() better.
But would be nice to hear other opinions here: cc @nastra @rdblue @RussellSpitzer

I think there are two separate questions here.

Where uniqueness gets enforced. Non-empty and unique field IDs are a write-time invariant of a column file, so I think the ColumnFileStruct constructor is the right place for it, even if subsequent PRs end up adding a builder. The constructor is the one place every write path would go through.

The return type. I agree with @stevenzwu, I think fieldIds() should return a Set. It's the most general type that makes the uniqueness invariant clear just from the signature, and it still leaves the implementation free to track an int[] or however else it wants. I know ContentFile returns lists for similar fields, but I don't think we need to stay consistent with that if there's a clearer way to express the contract.

I disagree with the constructor being the right place. TrackedFile and Tracking also have their own invariants that we don't verify in their constructors. A future builder is the right place for verification and the constructor should just accept whatever the builder passes.

Changed that we input and output fieldIds as Set, but internally still keep it as int[]. WDYT?

stevenzwu · 2026-10-13T05:43:40Z

            ByteBuffer.wrap(new byte[] {1, 2, 3}), // key metadata
-            ImmutableList.of(50L, 100L));
+            ImmutableList.of(50L, 100L),
+            null); // column files


I know there is a resolved comment on this topic already. but I want to ask it again.

if the V4ManifestReader follow-up is not big, might be better to include it in this PR.

I'd prefer doing that as a follow-up. We seem to lack reviewer bandwidth already on this one, wouldn't increase the scope with the reader part.

I agree with doing the V4ManifestReader change now. All these nulls seem unnecessary and I think the fix for this is pretty small. I think the net prod changes is just

// TrackedFile.java int COLUMN_FILES_ELEMENT_ID = 159; Types.NestedField COLUMN_FILES = Types.NestedField.optional( 158, "column_files", Types.ListType.ofRequired(COLUMN_FILES_ELEMENT_ID, ColumnFile.schema()), "Column files"); // V4ManifestReader.open() .setCustomType(TrackedFile.COLUMN_FILES_ELEMENT_ID, ColumnFileStruct.class)

These, and resolving the location of the column files, and whatever else I haven't noticed so far :) Including tests that would be an additional ~100-150 lines of code and complexity on top of this pretty simple metadata structure addition.
I'd still like to avoid merging the 2 PRs, and also they logically separate easily: introducing the structs + wiring them in to the reader.

gaborkaszab

Thanks for looking into, @stevenzwu !

gaborkaszab · 2026-10-15T07:37:28Z

            ByteBuffer.wrap(new byte[] {1, 2, 3}), // key metadata
-            ImmutableList.of(50L, 100L));
+            ImmutableList.of(50L, 100L),
+            null); // column files


I'd prefer doing that as a follow-up. We seem to lack reviewer bandwidth already on this one, wouldn't increase the scope with the reader part.

gaborkaszab · 2026-10-15T07:51:29Z

+      Preconditions.checkArgument(newFieldIds != null, "Invalid field IDs: null");
+      Preconditions.checkArgument(!newFieldIds.isEmpty(), "Invalid field IDs: empty");
+      Preconditions.checkArgument(
+          Sets.newHashSet(newFieldIds).size() == newFieldIds.size(),


I'm not sure, because I wanted this to be consistent with equality field IDs, where we pass lists.
But this reminds me that it's not just item uniqueness what matters but I recall we wanted to make this ordered. Enforcing that now.

More general question: This builder is currently used only in tests. Technically we could drop this similarly to other readers. We'll probably need a user-facing builder later on though, so that users can pass ColumnFile objects through table API, so the plan is to move this to a public class in core/ as similarly we have DataFiles we could have ColumnFiles.
The question is, should I drop the builder now, or can this stay until we move this to a public builder class?

gaborkaszab · 2026-10-15T09:01:25Z

+    if (toCopy.columnFiles != null) {
+      this.columnFiles = Lists.newArrayListWithCapacity(toCopy.columnFiles.size());
+      for (ColumnFile columnFile : toCopy.columnFiles) {
+        this.columnFiles.add(columnFile != null ? columnFile.copy() : null);


Probably I'm over cautious here. On the creation side, we should verify items are non-null, and not here.
Removed the null check here.

gaborkaszab · 2026-10-15T09:10:03Z

+  @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+  void serializationRoundTrip(TestHelpers.RoundTripSerializer<ColumnFile> roundTripSerializer)
+      throws IOException, ClassNotFoundException {
+    ColumnFile columnFile =


I'm hesitant here. I had the impression that in other test suites for V4 we preferred to spell out the creation of the class we test in each test case. I can create a static constant for the ColumnFile that we reuse across tests, but then the tests wouldn't be self-contained.

nastra · 2026-10-15T13:03:20Z

+  protected <T> void internalSet(int pos, T value) {
+    switch (pos) {
+        // always coerce to String for Serializable
+      case 0 -> this.location = value.toString();


is the location always non-null here? I think we could also just cast directly to a string?

I wanted to make this consistent with the same in TrackedFileStruct and DeletionVectorStruct.

nastra · 2026-10-15T13:04:54Z

+    }
+
+    Builder fieldIds(List<Integer> newFieldIds) {
+      Preconditions.checkArgument(newFieldIds != null, "Invalid field IDs: null");


I think we typically do the null & empty check in a single call and usually use Invalid ...: null or empty as the error msg

Makes sense. However, I dropped this builder for now because it was only used in tests. Once I add this back as a user-facing functionality, I'll do the adjustment (it's already on a backup branch for me).

nastra · 2026-10-15T13:06:23Z

  }

+  /** Indicates that the column files list has been updated for the new Tracking. */
+  TrackingBuilder columnFilesUpdated() {


when would this typically be called?

This is meant to be used on the write part, similarly as dvUpdated() above. When creating the new entry with MODIFIED status using an EXISTING entry, this is the way to adjust Tracking content such as the status or the modifiedSnapshotId accordingly.
This operation might be user initiated, e.g. committing column files through table API, or internally initiated like updating the leaf manifest using column files, and not rewriting the leaf manifest itself.

gaborkaszab

Thanks for the reviews, @stevenzwu @nastra !

I decided to drop the builder for ColumnFileStruct because now it was only used by tests. Later, when introducing a user-facing API to create ColumnFiles, I'll publish that as a separate PR. This made some of the comments obsolete.

@stevenzwu about fieldIds being List or Set. In the schema we define it as a list. Seems uncomfortable to use Set as internal representation and return list or access list in getter/setters.

amogh-jahagirdar · 2026-10-20T17:46:30Z

            ByteBuffer.wrap(new byte[] {1, 2, 3}),
-            ImmutableList.of(50L));
+            ImmutableList.of(50L),
+            null); // ColumnFile has its own serialization tests


I think we should pass actual column files here instead of null. ColumnFileStruct has its own serialization tests, but the list field is owned by TrackedFileStruct, so right now nothing checks that column_files behaves as expected after serialization.

Could we add a couple of real ColumnFileStructs and compare them with the ColumnFile.schema() comparator? Same idea as these two comments on the original TrackedFileStruct PR: real values instead of nulls and serialization tests having the same validations as copy.

I checked those 2 comments, both came from @rdblue on the initial PR for TrackedFileStruct. Then it was himself who introduced mocks for the tests of that class and in this particular test used null for DeletionVector and ManifestInfo above. I followed the same pattern.
We can revisit this, but let's make it in a follow-up covering all the nested structs in case we still want that coverage to avoid the test being asymmetrical.

amogh-jahagirdar · 2026-10-20T17:46:30Z

+
+    ColumnFile copy = columnFile.copy();
+
+    assertThat((StructLike) copy).usingComparator(COLUMN_FILE_COMPARATOR).isEqualTo(columnFile);


Could we also assert that the copy doesn't share keyMetadata with the original? Right now equality would pass even if the implementation incorrectly did a shallow copy. Since the getter wraps the internal array, checking keyMetadata().array() with isNotSameAs should catch it, same as this comment on the dv copy test.

Thanks for the pointer! Done

amogh-jahagirdar · 2026-10-20T17:46:30Z

  }
+
+  @Test
+  void manifestPositionsWithColumnFilesUpdated() {


If I'm not mistaken, this test passes for status and snapshot ID even if columnFilesUpdated() does nothing, since the positions setters already set MODIFIED and modifiedSnapshotId. Could we add a separate test that only calls columnFilesUpdated() on an existing entry and checks status, snapshot ID and both sequence numbers? Right now only the data sequence number assertion depends on the new method.

Aaah, you're right. I haven't adjusted the test after we merged the 2 dedicated snapshot IDs. Added a dedicated test.
Maybe this test still makes sense to simulate a manifest entry pointing to a leaf manifest having both column files and deleted/replaced positions.

This change introduces the interface for column files and also integrates it to the schema for TrackedFile.

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

gaborkaszab commented May 11, 2026

View reviewed changes

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

anuragmantri added this to V4: Efficient Column Updates May 11, 2026

anuragmantri moved this to Backlog in V4: Efficient Column Updates May 11, 2026

anuragmantri moved this from Backlog to In progress in V4: Efficient Column Updates May 11, 2026

gaborkaszab force-pushed the main_column_file_interface branch from 630b00e to ca3259e Compare May 11, 2026 17:04

anuragmantri reviewed May 11, 2026

View reviewed changes

gaborkaszab force-pushed the main_column_file_interface branch from ca3259e to e6f7cf6 Compare May 12, 2026 09:35

gaborkaszab commented May 13, 2026

View reviewed changes

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

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

gaborkaszab force-pushed the main_column_file_interface branch 3 times, most recently from 681633b to 813d5c0 Compare May 13, 2026 12:52

gaborkaszab force-pushed the main_column_file_interface branch 3 times, most recently from 596f6a4 to 6a1cbe9 Compare June 2, 2026 13:12

gaborkaszab changed the title ~~Core: Introduce interface for column files~~ Core: Basic fields and schemas for column files Jun 3, 2026

gaborkaszab force-pushed the main_column_file_interface branch from 6a1cbe9 to c683e72 Compare June 4, 2026 06:55

pvary moved this to In review in V4: metadata tree Jun 8, 2026

pvary added this to V4: metadata tree Jun 8, 2026

gaborkaszab force-pushed the main_column_file_interface branch from c683e72 to 6222fad Compare June 8, 2026 12:32

stevenzwu reviewed Jun 9, 2026

View reviewed changes

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

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

gaborkaszab force-pushed the main_column_file_interface branch from 6222fad to 5c04f55 Compare June 11, 2026 09:27

gaborkaszab commented Jun 11, 2026

View reviewed changes

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

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

gaborkaszab force-pushed the main_column_file_interface branch from 5c04f55 to b6ae446 Compare June 12, 2026 14:37

gaborkaszab commented Jun 12, 2026

View reviewed changes

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

gaborkaszab force-pushed the main_column_file_interface branch from b6ae446 to 0a252e8 Compare June 12, 2026 15:09

gaborkaszab commented Aug 8, 2026

View reviewed changes

gaborkaszab requested review from anuragmantri and stevenzwu August 8, 2026 12:14

gaborkaszab force-pushed the main_column_file_interface branch from 2bdf7bb to d0509a7 Compare August 10, 2026 20:09

stevenzwu approved these changes Aug 14, 2026

View reviewed changes

gaborkaszab force-pushed the main_column_file_interface branch 4 times, most recently from c26a609 to dd2433c Compare August 26, 2026 16:11

gaborkaszab force-pushed the main_column_file_interface branch from dd2433c to b0712b8 Compare September 7, 2026 11:11

gaborkaszab force-pushed the main_column_file_interface branch 2 times, most recently from abbdc88 to 0d37967 Compare September 16, 2026 16:00

laskoviymishka requested changes Sep 22, 2026

View reviewed changes

laskoviymishka approved these changes Sep 23, 2026

View reviewed changes

gaborkaszab commented Oct 5, 2026

View reviewed changes

mkroll-db reviewed Oct 11, 2026

View reviewed changes

stevenzwu reviewed Oct 13, 2026

View reviewed changes

gaborkaszab commented Oct 15, 2026

View reviewed changes

nastra reviewed Oct 15, 2026

View reviewed changes

gaborkaszab commented Oct 16, 2026

View reviewed changes

amogh-jahagirdar reviewed Oct 20, 2026

View reviewed changes

gaborkaszab added 2 commits October 20, 2026 22:06

Core: Basic fields and schemas for column files

7498f8e

This change introduces the interface for column files and also integrates it to the schema for TrackedFile.

Core: Add columnFiles() to write-path adapters

89a8846


		ColumnFile copy = columnFile.copy();

		assertThat((StructLike) copy).usingComparator(COLUMN_FILE_COMPARATOR).isEqualTo(columnFile);

Conversation

gaborkaszab commented May 11, 2026

Uh oh!

Uh oh!

gaborkaszab commented May 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaborkaszab commented May 21, 2026

Uh oh!

gaborkaszab commented Jun 2, 2026

Uh oh!

gaborkaszab commented Jun 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

gaborkaszab commented Aug 24, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab commented Oct 5, 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!

mkroll-db Oct 11, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkroll-db Oct 11, 2026 •

edited

Loading

amogh-jahagirdar Oct 20, 2026 •

edited

Loading

gaborkaszab left a comment •

edited

Loading