-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Core: Add test for renaming table to a non-existing namespace #9895
Core: Add test for renaming table to a non-existing namespace #9895
Conversation
@@ -226,6 +226,12 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) { | |||
throw new NoSuchTableException("Invalid identifier: %s", from); | |||
} | |||
|
|||
if (!namespaceExists(originalTo.namespace())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While reviewing #9852 I noticed that there's no such check in Hive that checks whether that namespace exists and Hive would just fail with a RuntimeException
a5980ca
to
3142109
Compare
3142109
to
1b91b10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing it.
@@ -228,6 +228,10 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) { | |||
|
|||
TableIdentifier to = removeCatalogName(originalTo); | |||
Preconditions.checkArgument(isValidIdentifier(to), "Invalid identifier: %s", to); | |||
if (!namespaceExists(to.namespace())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pvary could you review this small change please?
thanks everyone for reviewing |
For view catalogs we already test the same behavior in https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java#L589-L611 but we haven't done so for table catalogs.