Skip to content

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Jul 17, 2023

Currently, executing ALTER TABLE x CREATE OR REPLACE TAG xyz will fail with Tag does not exist: xyz.

As a user I'd expect this to create the tag due to the CREATE OR REPLACE usage. The same issue happens with branches.

}

@Test
public void replaceBranch() throws NoSuchTableException {
Copy link
Contributor Author

@nastra nastra Jul 17, 2023

Choose a reason for hiding this comment

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

there was a test gap for branches where a pure replace and a replace on a non-existing branch wasn't exercised, therefore I added replaceBranch() / replaceBranchDoesNotExist() / createOrReplace() (similar tests already exist in TestTagDDL)

@nastra nastra force-pushed the create-or-replace-branch-tag branch 2 times, most recently from e9e31d0 to b501628 Compare July 17, 2023 16:18
@nastra nastra requested a review from danielcweeks July 18, 2023 08:17

val manageSnapshot = iceberg.table.manageSnapshots()
if (!replace) {
if (create && replace && null == iceberg.table().refs().get(tag)) {
Copy link
Contributor

@danielcweeks danielcweeks Jul 18, 2023

Choose a reason for hiding this comment

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

This is a little hard to reason through, what about simplifying to something more like:

val exists = null == iceberg.table().refs().get(tag)

if (create && !exists) {
  if (exists && ifNotExists) {
    return Nil
  }
  manageSnapshot.createTag(tag, snapshotId)
} else if (replace && exists) {
  manageSnapshot.replaceTag(tag, snapshotId)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will change the expected behavior unfortunaly. Tests are expecting ALTER TABLE x REPLACE TAG non-existing to throw an exception, therefore we need to distinguish whether CREATE OR REPLACE is used here vs REPLACE

Copy link
Contributor

Choose a reason for hiding this comment

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

Would removing the exists variable from the second if statement achieve that? Then it would always call replaceTag if it's not a create and result in the expected throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this would restore the behavior I mentioned above, but it would change a different behavior where ALTER TABLE x CREATE TAG t is expected to fail when t already exists. The conditions are quite tricky unfortunately and from what I've seen is that we need to explicitly handle create && replace && !refExists

@nastra nastra force-pushed the create-or-replace-branch-tag branch 2 times, most recently from fe5a55f to 763dfcd Compare July 20, 2023 08:56
Currently, executing `ALTER TABLE x CREATE OR REPLACE TAG xyz` will fail
with `Tag does not exist: xyz`.

As a user I'd expect this to create the tag due to the `CREATE OR
REPLACE` usage. The same issue happens with branches.
@nastra nastra force-pushed the create-or-replace-branch-tag branch from 763dfcd to bc2addc Compare July 20, 2023 08:57
@danielcweeks danielcweeks merged commit 9204b2d into apache:master Jul 20, 2023
@nastra nastra deleted the create-or-replace-branch-tag branch July 20, 2023 15:42
manuzhang added a commit to manuzhang/iceberg that referenced this pull request Aug 3, 2023
This is based on apache#7097, and back-ports bug fixes apache#7652 and apache#8086
nastra pushed a commit that referenced this pull request Aug 3, 2023
This is based on #7097, and back-ports bug fixes #7652 and #8086
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants