Skip to content

Core: Add setter for firstRowId in V4 metadata - #16849

Closed
gaborkaszab wants to merge 1 commit into
apache:mainfrom
gaborkaszab:main_first_row_id_setter
Closed

Core: Add setter for firstRowId in V4 metadata#16849
gaborkaszab wants to merge 1 commit into
apache:mainfrom
gaborkaszab:main_first_row_id_setter

Conversation

@gaborkaszab

@gaborkaszabgaborkaszab commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@gaborkaszab

gaborkaszab commented Jun 17, 2026

Copy link
Copy Markdown
ContributorAuthor

Currently, there is no mechanism to set Tracking.firstRowId. Following the PR for Tracking builder, it was commented that setting firstRowId shouldn't be through the builder but via inheritance.
This PR adds a simple setter function that can be used for this. If I understand correctly, it wouldn't suite inheritFrom(manifestTracking) because it's not a direct inheritance, but based calculated based on other manifest entries that has null firstRowId + the manifest's firstRowId.

@gaborkaszab

gaborkaszab commented Jun 17, 2026

Copy link
Copy Markdown
ContributorAuthor

cc @stevenzwu@rdblue@anoopj@amogh-jahagirdar

anoopj
anoopj approved these changes Jun 17, 2026
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TrackingStruct.java Outdated
}

void setFirstRowId(Long newFirstRowId) {
Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null");

@anoopjanoopjJun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make the parameter long since it's required anyway?

@ebyhrebyhrJun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to make the parameter to long.

@gaborkaszabgaborkaszabJun 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! done

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

void setFirstRowId(Long newFirstRowId) {
Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null");
Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative");

@anoopjanoopjJun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative");
Preconditions.checkArgument(
newFirstRowId >= 0, "Invalid first row ID: %s (must be >= 0)", newFirstRowId);

@gaborkaszabgaborkaszabJun 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, done

ebyhr
ebyhr approved these changes Jun 18, 2026
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TrackingStruct.java Outdated
}

void setFirstRowId(Long newFirstRowId) {
Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null");

@ebyhrebyhrJun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to make the parameter to long.

Comment threadcore/src/test/java/org/apache/iceberg/TestTrackingStruct.java
@gaborkaszab
gaborkaszabforce-pushed the main_first_row_id_setter branch from fe964f0 to a0b0dddCompareJune 18, 2026 09:50
gaborkaszab
gaborkaszab commented Jun 18, 2026
View reviewed changes

@gaborkaszabgaborkaszab left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! I addressed all the comments. Additionally, there are 2 extra modifications I figured would make sense:

  1. Not allow to set first row id for ADDED entries. I think for these null is the desired value
  2. I also added a setter method to TrackedFileStruct for convenience. With this the user doesn't have to be aware of the actual representation being in Tracking and can simply invoke TrackedFileStruct.setFirstRowId() without knowing how it is represented internally.

Let me know what you think!

Comment threadcore/src/main/java/org/apache/iceberg/TrackingStruct.java Outdated
}

void setFirstRowId(Long newFirstRowId) {
Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null");

@gaborkaszabgaborkaszabJun 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! done

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

void setFirstRowId(Long newFirstRowId) {
Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null");
Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative");

@gaborkaszabgaborkaszabJun 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, done

Comment threadcore/src/test/java/org/apache/iceberg/TestTrackingStruct.java
@gaborkaszab
gaborkaszabforce-pushed the main_first_row_id_setter branch from a0b0ddd to ab3b98aCompareJune 18, 2026 10:02
@gaborkaszabgaborkaszab changed the title Core: Add setter for TrackingStruct.firstRowIdCore: Add setter for firstRowId in V4 metadataJun 18, 2026
@gaborkaszab
gaborkaszab requested a review from anoopjJune 18, 2026 14:17
@gaborkaszabgaborkaszab mentioned this pull request Jun 23, 2026
Open
@RussellSpitzerRussellSpitzer added the Iceberg V4 Iceberg Table Format Version 4 label Jul 16, 2026
dramaticlly
dramaticlly reviewed Jul 22, 2026
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileStruct.java
public void setFirstRowId(int firstRowId) {
Preconditions.checkState(
contentType == FileContent.DATA || contentType == FileContent.DATA_MANIFEST,
"Cannot set first row id on an equality delete or delete manifest entry");

@dramaticllydramaticllyJul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of saying what's not allowed. Would stating only allow the supported content type be more explicit?

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

/** Sets the first row id on this {@link TrackedFileStruct} */
public void setFirstRowId(int firstRowId) {

@dramaticllydramaticllyJul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, should firstRowId be long type instead, like in TrackingStruct?

@gaborkaszab

gaborkaszab commented Jul 23, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking, @dramaticlly ! Your comments make sense. In the meantime there might be a different approach to set firstRowId, so I'll change this to draft until we decide.

@gaborkaszab
gaborkaszab marked this pull request as draft July 23, 2026 06:36
@github-actions

github-actionsBot commented Aug 23, 2026

Copy link
Copy Markdown

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions

github-actionsBot commented Aug 30, 2026

Copy link
Copy Markdown

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anoopjanoopjAwaiting requested review from anoopj
2 more reviewers
@dramaticllydramaticllydramaticlly left review comments
@ebyhrebyhrebyhr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coreIceberg V4Iceberg Table Format Version 4stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gaborkaszab@anoopj@dramaticlly@ebyhr@RussellSpitzer