Skip to content

Conversation

vinjai
Copy link
Contributor

@vinjai vinjai commented Sep 18, 2025

Closes #2428

Rationale for this change

Support to add files to iceberg branches. Currently, you can only add files to the main branch

Are these changes tested?

Yes

Are there any user-facing changes?

New optional paramater for branch in the add_files method of the Table API

@vinjai
Copy link
Contributor Author

vinjai commented Sep 18, 2025

@kevinjqliu @Fokko Request your review

Copy link
Contributor

@ForeverAngry ForeverAngry left a comment

Choose a reason for hiding this comment

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

This pr is 🔥 ! I pulled it down and tested it on a few large tables in my own dev env - and it worked well. Really great work, I'm excited to use this in prod!

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to add a negative test that attempts to add files to a non-existent branch, just to make sure that exceptions are handled gracefully and that meaningful errors are surfaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be same as the test for appending to non-existing branch:

def test_append_to_non_existing_branch(session_catalog: Catalog, arrow_table_with_null: pa.Table) -> None:

If seen from the POV of a snapshot, the flow for add_files and append operation is same as only a new snapshot with new files is being appended.
Since, we are not adding any different code, it just introduces another test which goes through the same flow thus increasing test time.

My suggestion would be to not bloat the test time with similar tests
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me!

Copy link
Contributor

@ForeverAngry ForeverAngry left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Nice one @vinjai 🙌 I've left one comment, but apart from that it is good to go 👍

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM

slight nit on the test comment consistency

@Fokko Fokko merged commit a8df020 into apache:main Sep 23, 2025
10 checks passed
@vinjai vinjai deleted the feature/add-files-branch branch September 24, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update add_files operation to support branches

4 participants