Skip to content

Feature: Write to branches #941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Feature: Write to branches #941

wants to merge 24 commits into from
+266 −43

Conversation

vinjai
Copy link

@vinjai vinjai commented Jul 18, 2024

Fixes: #306

kevinjqliu and others added 4 commits July 15, 2024 03:36
@vinjai vinjai changed the title Feature/write to branch Feature: Write to branches Jul 18, 2024
@vinjai vinjai marked this pull request as ready for review October 16, 2024 08:15
@vinjai
Copy link
Author

vinjai commented Oct 16, 2024

@sungwy @kevinjqliu
This PR is ready for review

@vinjai
Copy link
Author

vinjai commented Oct 16, 2024 •
edited
Loading

Fixed another bug.
Writes with same name but different ref types were being successful. This is a bug in the current release version too

Please review whenever you get some time.

@kevinjqliu
Copy link
Contributor

Thanks for the contribution! I'll take a look.
I remember adding support for branch is complicated since we need to consider different edge cases.

@vinjai
Copy link
Author

vinjai commented Oct 19, 2024 •
edited
Loading

I have mostly tried to cover all edge cases.
The idea is that the branch is just another iceberg table where the snapshots append independently of the main branch.

I also agree with your concern.
If it helps, we can add more test cases

@kevinjqliu kevinjqliu added this to the PyIceberg 0.9.0 release milestone Oct 30, 2024
@vinjai
Copy link
Author

vinjai commented Nov 7, 2024

Hey @kevinjqliu
Did you get a chance to look at this?

kevinjqliu
kevinjqliu reviewed Nov 10, 2024
View reviewed changes
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.

Thanks for the PR! And sorry for the delay, I was running the 0.8.0 release.
Generally LGTM, I left a comment about add more tests integrating with Spark

@vinjai
Copy link
Author

vinjai commented Nov 14, 2024

Thank you for the review @kevinjqliu

@vinjai
Copy link
Author

vinjai commented Nov 20, 2024

@kevinjqliu What are the next steps to get this merged?

@vinjai vinjai requested a review from kevinjqliu November 20, 2024 10:00
kevinjqliu
kevinjqliu reviewed Nov 21, 2024
View reviewed changes
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.

apologies for the delay here, was working on releasing 0.8.0

Added a few comments

Since this is a huge feature, I want to make sure we have thought through all the edge cases

tests/table/test_init.py Outdated

with pytest.raises(
CommitFailedException,
match="Requirement failed: branch or tag not_exist_branch is missing, expected 1",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, we know this is referencing a branch

Copy link
Author

Choose a reason for hiding this comment

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

We are not aware if it is a branch or tag as it is non-existent
This is the error message from the AssertRefSnapshotId when the ref is non-existing

tests/table/test_init.py Outdated

with pytest.raises(
CommitFailedException,
match="Requirement failed: branch or tag not_exist_tag is missing, expected 1",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, same here with tag

pyiceberg/table/update/__init__.py Outdated
@@ -609,11 +609,14 @@ class AssertRefSnapshotId(ValidatableTableRequirement):

type: Literal["assert-ref-snapshot-id"] = Field(default="assert-ref-snapshot-id")
ref: str = Field(...)
ref_type: SnapshotRefType = Field(...)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit instead of adding this extra field, can we do something like the java side?

https://github.com/apache/iceberg/blob/90be5d7360bc7ff274e7d00cb7259afbf78f223b/core/src/main/java/org/apache/iceberg/UpdateRequirement.java#L91-L127

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Let me get back on how I can introduce the change

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/apache/iceberg/blob/90be5d7360bc7ff274e7d00cb7259afbf78f223b/core/src/main/java/org/apache/iceberg/UpdateRequirement.java#L110-L112

this is looked up at runtime

Copy link
Author

Choose a reason for hiding this comment

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

Resolved the comments

@kevinjqliu
Copy link
Contributor

cc @HonahX / @Fokko / @sungwy

@kevinjqliu kevinjqliu requested a review from Fokko December 2, 2024 18:10
@kevinjqliu
Copy link
Contributor

@vinjai coming back to this after the 0.8.1 release :)
Feel free to tag me again once the comments above are addressed. Thanks again for the contribution!

@kevinjqliu kevinjqliu removed this from the PyIceberg 0.9.0 release milestone Feb 1, 2025
@kevinjqliu
Copy link
Contributor

hey @vinjai are you interested to pick this back up?

@vinjai
Copy link
Author

vinjai commented Feb 1, 2025

Hey @kevinjqliu
I was traveling for the past two months and couldn’t complete the review comments.
I’ll be back in a week, will address the comments then, and get this moving.

@kevinjqliu
Copy link
Contributor

thank you! @vinjai feel free to tag me again for review :)

@vinjai
Copy link
Author

vinjai commented Feb 23, 2025

Hey @kevinjqliu
I have resolved all comments.
Will resolve the merge conflicts once you have gone through these changes (to avoid rework).

Let me know once you have reviewed the above comments

@malcolmbovey malcolmbovey mentioned this pull request Feb 28, 2025
Open
@mjclarke94 mjclarke94 mentioned this pull request Mar 7, 2025
Open
@vinjai
Copy link
Author

vinjai commented Mar 8, 2025

Hey @kevinjqliu
Did you get a chance to look at the PR again?

@malcolmbovey
Copy link

Hi, just wondering if there is an update on this PR?

@vinjai
Copy link
Author

vinjai commented Mar 26, 2025

Hey @kevinjqliu
Bumping up for review

@dbuades dbuades mentioned this pull request Apr 24, 2025
Open
@Fokko
Copy link
Contributor

Fokko commented Apr 24, 2025

Thanks @dbuades for pinging me, and sorry for letting this one linger for so long. Let me review this tomorrow morning.

@vinjai Do you have time to resolve the conflicts by any chance?

@vinjai
Copy link
Author

vinjai commented Apr 28, 2025

Hey @Fokko
Will try to resolve the conflicts over the weekend.

@SebastienN15
Copy link

Hey @vinjai, did you get a chance to fix those conflicts? 🙂

@SebastienN15
Copy link

@vinjai I fixed the conflicts on this branch, feel free to cherry pick the last commit #2009
Otherwise I can open a PR on my side and take it from there 🙂

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

@kevinjqliu kevinjqliu kevinjqliu left review comments

@Fokko Fokko Awaiting requested review from Fokko

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support writing to a branch
5 participants
@vinjai @kevinjqliu @malcolmbovey @Fokko @SebastienN15