Skip to content

Conversation

@ConeyLiu
Copy link
Contributor

Currently, we only support creating branches with a given snapshot ID. However, there is no snapshot for an empty table. This PR achieves this by creating a newly empty snapshot for an empty table. And binding the branch to the new snapshot.

@ConeyLiu
Copy link
Contributor Author

Hi @rdblue @jackye1995, I see you have discussed this at here #7075, could you help to review this when you are free? Thanks a lot.

@zhangbutao
Copy link
Contributor

+1 for this change. This will solve this issue #7593 at its root.

"1.3.0":
org.apache.iceberg:iceberg-api:
- code: "java.method.addedToInterface"
new: "method org.apache.iceberg.ManageSnapshots org.apache.iceberg.ManageSnapshots::createBranch(java.lang.String)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to break the API just by adding a new method. Rather than breaking the API, we typically add a default implementation that throws an UOE, similar to how it's done in Snapshot#removedDeleteFiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nastra, updated.

@ConeyLiu ConeyLiu force-pushed the create-empty-branch branch from e267ac3 to d55339f Compare July 22, 2023 09:13
@ConeyLiu
Copy link
Contributor Author

@nastra would you mind taking a look at this again? Thanks a lot.

@rdblue
Copy link
Contributor

rdblue commented Aug 1, 2023

@amogh-jahagirdar and @jackye1995, FYI

*
* @param name branch name
* @return this for method chaining
* @throws IllegalArgumentException if a branch with the given name already exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me do a follow-up.

Copy link
Contributor Author

@ConeyLiu ConeyLiu Aug 1, 2023

Choose a reason for hiding this comment

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

@rdblue I opened #8197 for a follow-up. And I am sorry, this does miss the checking.

@rdblue rdblue merged commit f98c641 into apache:master Aug 1, 2023
@rdblue
Copy link
Contributor

rdblue commented Aug 1, 2023

Thanks, @ConeyLiu!

@ConeyLiu
Copy link
Contributor Author

ConeyLiu commented Aug 1, 2023

Thanks @rdblue for merging this. And also thanks @nastra @zhangbutao for reviewing.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants