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 · 2026-06-12T14:39:56Z

+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.
+ this.dataSequenceNumber = null;


We discussed bumping the data sequence number when adding column files. We haven't mentioned file seq num, so I'm not bumping it here.
This works if the manifest owning this data file entry bumps its own seq num when adding column files. Let me know if there is any other way achieving this.

In a previous google doc discussion, @pvary raised the question if we should just bump up the dataSequenceNumber which captures the logical age of the row. Column file should materialize the _last_updated_sequence_number for unmodified rows and leave the modified rows with null value for inheritance. From row lineage perspective, bumping up dataSequenceNumber is correct and simpler semantically.
data sequence number is only used for v2 equality and position delete matching. it seems that we might be able to forbid writing new equality deletes for v4 tables. I also remember some previous discussion on rewriting equality delete and v2 position delete files when adding a new column file. With the writer requirement, it is safe to just bump up the dataSequenceNumber here.
But the comment line is a bit confusing. I would write as following: Reset to null to inherit from the new snapshot sequence number. It is safe to bump up the dataSequenceNumber as writers are required to rewrite v2 equality and position deletes to DVs when applying column update.

Thanks for the comment suggestion! Added

gaborkaszab · 2026-07-23T13:49:18Z

Rebased with latest main to resolve git conflicts

amogh-jahagirdar

Still need to do a pass over tests, but had some comments on the changes.
The field IDs look reasonable to me. In terms of spec stuff on the entry structure, I remember there was an open quesiton around if we needed split offsets per column file or not. My take is yes, the files would have fundamentally different schemas and there may be more appropriate boundary split points in the file depending on that, but not sure if this had settled. Apologies if this had already reached a consensus and I missed it. @gaborkaszab @anuragmantri

amogh-jahagirdar · 2026-07-29T23:38:17Z

+ ByteBuffer keyMetadata();
+
+ /** Returns the list of recommended split locations for this column file, or null. */
+ List<Long> splitOffsets();


I remember there was a discussion around if we need split offsets per column file or not, what was the conclusion on that? I feel like it makes sense to have them, there may be different files with varying schemas and as a result there would be different boundaries that would be set appropriately for each file. It's additional complexity, but not much?

I brought this up once on the sync, we didn't have a deep dive but there were no objections either. Rational might be that when we project fields in a way that we don't have to read the base file, we can use the split offsets of one of the column files.
Now, with regular column updates, the column files usually contain a column or two and they have a single row group. However, later for column families we can have wider column files with more than one row groups where we can have split offsets.

amogh-jahagirdar · 2026-07-29T23:43:30Z

 }

 /** Copy constructor. */
+ @SuppressWarnings("CyclomaticComplexity")


Any reasonable way to tighten this, or is it just inherent complexity from all the null handling we need to do?

Yes, this is because the null checks. I didn't want to extract any of these into separate methods as that would have harmed readability.

amogh-jahagirdar · 2026-07-29T23:44:07Z

 class TrackingBuilder {
 private final long newSnapshotId;
 private final Long snapshotId;
- private final Long dataSequenceNumber;


Does this need to move?

It's no longer final, I moved it to the non-final section.

amogh-jahagirdar · 2026-07-29T23:50:51Z

+ case 0 -> formatVersion;
+ case 1 -> fieldIds();
+ case 2 -> location;
+ case 3 -> fileFormat != null ? fileFormat.toString() : null;


I think I've noticed this on other PRs for TrackedFile for instance, when would fileFormat be null? It's required right?

I took this from TrackedFileStruct where we guard against fileFormat and contentType being null, however both of them are required. Maybe makes sense when we have a projected read and we don't project those fields?

amogh-jahagirdar · 2026-07-30T00:00:35Z

 toCopy.equalityIds != null
 ? Arrays.copyOf(toCopy.equalityIds, toCopy.equalityIds.length)
 : null;
+ this.columnFiles =


Minor: Could we express this without the Java stream? It's just a bit on the hot path when reading entries so all those allocations from the additional objects that streams create may add up.

Sure, done.

gaborkaszab · 2026-07-30T19:55:20Z

Rebased with main to resolve conflicts

gaborkaszab

Thanks for taking a look, @amogh-jahagirdar !

gaborkaszab · 2026-07-30T19:11:32Z

+ ByteBuffer keyMetadata();
+
+ /** Returns the list of recommended split locations for this column file, or null. */
+ List<Long> splitOffsets();


I brought this up once on the sync, we didn't have a deep dive but there were no objections either. Rational might be that when we project fields in a way that we don't have to read the base file, we can use the split offsets of one of the column files.
Now, with regular column updates, the column files usually contain a column or two and they have a single row group. However, later for column families we can have wider column files with more than one row groups where we can have split offsets.

gaborkaszab · 2026-07-30T19:14:58Z

+ case 0 -> formatVersion;
+ case 1 -> fieldIds();
+ case 2 -> location;
+ case 3 -> fileFormat != null ? fileFormat.toString() : null;


I took this from TrackedFileStruct where we guard against fileFormat and contentType being null, however both of them are required. Maybe makes sense when we have a projected read and we don't project those fields?

gaborkaszab · 2026-07-30T19:16:30Z

 }

 /** Copy constructor. */
+ @SuppressWarnings("CyclomaticComplexity")


Yes, this is because the null checks. I didn't want to extract any of these into separate methods as that would have harmed readability.

gaborkaszab · 2026-07-30T19:21:59Z

 toCopy.equalityIds != null
 ? Arrays.copyOf(toCopy.equalityIds, toCopy.equalityIds.length)
 : null;
+ this.columnFiles =


Sure, done.

gaborkaszab · 2026-07-30T19:23:04Z

 class TrackingBuilder {
 private final long newSnapshotId;
 private final Long snapshotId;
- private final Long dataSequenceNumber;


It's no longer final, I moved it to the non-final section.

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>

anuragmantri

I did another round after adding key_metdata and split_offsets. I think this is ready to be merged.

anuragmantri · 2026-08-07T18:13:03Z

+ if (status == EntryStatus.EXISTING) {
+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.


Should this comment be?
Suggested change
// Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.
// Clears dataSequenceNumber so it re-inherits from the manifest at read time.

Thanks for the suggestion! Steven also had one, I went with that.

anuragmantri · 2026-08-07T18:19:15Z

+ assertThat(withDeletedPositions.latestColumnFileSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.dvSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.deletedPositions()).isEqualTo(deletedBytes);
+


Should we verify the dataSequenceNumber is null?
Suggested change
assertThat(withDeletedPositions.dataSequenceNumber()).isNull();
Same on L314 and in manifestPositionsWithColumnFilesUpdated() test

stevenzwu · 2026-08-07T19:13:51Z

+ case 5 -> this.keyMetadata = ByteBuffers.toByteArray((ByteBuffer) value);
+ case 6 -> this.splitOffsets = ArrayUtil.toLongArray((List<Long>) value);
+ default -> {
+ // ignore the object, it must be from a newer version of the format


nit: should the comment say `ignore the unknown positions, as they must come from a newer version of the format"

This comment is inline with the same in TrackedFileStruct and TrackingStruct. I'd rather keep consistency with these.

that's fine for consistency. I found "ignore the object" not very accurate.

stevenzwu · 2026-08-07T19:54:18Z

+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.
+ this.dataSequenceNumber = null;


In a previous google doc discussion, @pvary raised the question if we should just bump up the dataSequenceNumber which captures the logical age of the row. Column file should materialize the _last_updated_sequence_number for unmodified rows and leave the modified rows with null value for inheritance. From row lineage perspective, bumping up dataSequenceNumber is correct and simpler semantically.
data sequence number is only used for v2 equality and position delete matching. it seems that we might be able to forbid writing new equality deletes for v4 tables. I also remember some previous discussion on rewriting equality delete and v2 position delete files when adding a new column file. With the writer requirement, it is safe to just bump up the dataSequenceNumber here.
But the comment line is a bit confusing. I would write as following: Reset to null to inherit from the new snapshot sequence number. It is safe to bump up the dataSequenceNumber as writers are required to rewrite v2 equality and position deletes to DVs when applying column update.

stevenzwu · 2026-08-07T20:15:59Z

+ Tracking withDeletedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .deletedPositions(deletedBytes)


deletedPositions bitmap is only meant for leaf manifest entry in the root manifest file? Ae we testing the scenario of column update for a leaf manifest file in this test?

I don't think technically we want to avoid providing deleted/replaced positions together with column files. I just wanted to pin this down with a test.
Giving this some further thought, I think you're right: Such a Tracking that has these positions is an entry in the root manifest pointing to a leaf manifest. I don't think we plan to add column files for leaf manifest at this point, but it seems too strict to reject such a setting.
Could such a test remain? WDYT @stevenzwu ?

I don't think we plan to add column files for leaf manifest at this point
We will use column files for leaf manifests in v4. we should keep this test.
I was mainly alluding to if we should cover the column update for data file cases, where deletedPositions is not applicable.

In Tracking and TrackingBuilder we don't really know if it belongs to a data file entry or a manifest entry. There might be implications like presence of deleted positions or dv_snapshot_id but nothing decisive. We can add a separate test where we don't set deleted/replaced positions, but probably it doesn't add much to the coverage.

gaborkaszab

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

gaborkaszab · 2026-08-08T11:06:34Z

+ case 5 -> this.keyMetadata = ByteBuffers.toByteArray((ByteBuffer) value);
+ case 6 -> this.splitOffsets = ArrayUtil.toLongArray((List<Long>) value);
+ default -> {
+ // ignore the object, it must be from a newer version of the format


This comment is inline with the same in TrackedFileStruct and TrackingStruct. I'd rather keep consistency with these.

gaborkaszab · 2026-08-08T11:12:22Z

+ if (status == EntryStatus.EXISTING) {
+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.


Thanks for the suggestion! Steven also had one, I went with that.

gaborkaszab · 2026-08-08T11:12:39Z

+ this.status = EntryStatus.MODIFIED;
+ }
+ // Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.
+ this.dataSequenceNumber = null;


Thanks for the comment suggestion! Added

gaborkaszab · 2026-08-08T11:54:32Z

+ assertThat(withDeletedPositions.latestColumnFileSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.dvSnapshotId()).isEqualTo(999L);
+ assertThat(withDeletedPositions.deletedPositions()).isEqualTo(deletedBytes);
+


gaborkaszab · 2026-08-08T11:57:55Z

+ Tracking withDeletedPositions =
+ TrackingBuilder.from(manifestSourceTracking(), 999L)
+ .columnFilesUpdated()
+ .deletedPositions(deletedBytes)


I don't think technically we want to avoid providing deleted/replaced positions together with column files. I just wanted to pin this down with a test.
Giving this some further thought, I think you're right: Such a Tracking that has these positions is an entry in the root manifest pointing to a leaf manifest. I don't think we plan to add column files for leaf manifest at this point, but it seems too strict to reject such a setting.
Could such a test remain? WDYT @stevenzwu ?

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?

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

github-actionsBot added the core label May 11, 2026

gaborkaszab commented May 11, 2026
View reviewed changes

Comment threadcore/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 ca3259eCompare 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 e6f7cf6Compare May 12, 2026 09:35

gaborkaszab commented May 13, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/ColumnFileInfo.java Outdated
Comment threadcore/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 813d5c0Compare May 13, 2026 12:52

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

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

gaborkaszab force-pushed the main_column_file_interface branch from 6a1cbe9 to c683e72Compare 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 6222fadCompare June 8, 2026 12:32

stevenzwu reviewed Jun 9, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileStruct.java
Comment threadcore/src/main/java/org/apache/iceberg/TrackingStruct.java

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

gaborkaszab commented Jun 11, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileStruct.java Outdated
Comment threadcore/src/main/java/org/apache/iceberg/TrackingBuilder.java

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

gaborkaszab commented Jun 12, 2026
View reviewed changes

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

gaborkaszab force-pushed the main_column_file_interface branch from 87d43a9 to 56028eeCompare July 23, 2026 13:47

gaborkaszab force-pushed the main_column_file_interface branch from 56028ee to 30f42acCompare July 23, 2026 14:40

amogh-jahagirdar reviewed Jul 30, 2026
View reviewed changes

gaborkaszab mentioned this pull request Jul 30, 2026
Core: Extend V4 DeletionVector with key_metadata field #17438
Merged

gaborkaszab force-pushed the main_column_file_interface branch 2 times, most recently from 870edbd to c303670Compare July 30, 2026 19:54

gaborkaszab commented Jul 30, 2026
View reviewed changes

gaborkaszab requested review from RussellSpitzer and amogh-jahagirdar July 30, 2026 20:07

gaborkaszab force-pushed the main_column_file_interface branch from c303670 to 5d96c17Compare August 6, 2026 07:56

anuragmantri approved these changes Aug 7, 2026
View reviewed changes

stevenzwu reviewed Aug 7, 2026
View reviewed changes

gaborkaszab force-pushed the main_column_file_interface branch from 5d96c17 to 2bdf7bbCompare August 8, 2026 12:11

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 d0509a7Compare 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 dd2433cCompare August 26, 2026 16:11

Core: Basic fields and schemas for column files
b0712b8
This change introduces the interface for column files and also integrates it to the schema for TrackedFile.

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

	// Bumping 'dataSequenceNumber' to avoid having both equality deletes and column files.
	// Clears dataSequenceNumber so it re-inherits from the manifest at read time.


	assertThat(withDeletedPositions.dataSequenceNumber()).isNull();

Uh oh!

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!

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 commented Jul 23, 2026

Uh oh!

amogh-jahagirdar left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab commented Jul 30, 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!

anuragmantri 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

amogh-jahagirdar left a comment •
edited
Loading