Skip to content
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

Merged

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Mar 7, 2024

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.

@@ -226,6 +226,12 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
throw new NoSuchTableException("Invalid identifier: %s", from);
}

if (!namespaceExists(originalTo.namespace())) {
Copy link
Contributor Author

@nastra nastra Mar 7, 2024

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

@nastra nastra force-pushed the test-for-missing-target-namespace-on-table-rename branch from a5980ca to 3142109 Compare March 7, 2024 17:02
@nastra nastra force-pushed the test-for-missing-target-namespace-on-table-rename branch from 3142109 to 1b91b10 Compare March 7, 2024 17:04
Copy link
Contributor

@nk1506 nk1506 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 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())) {
Copy link
Contributor Author

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?

@nastra
Copy link
Contributor Author

nastra commented Mar 8, 2024

thanks everyone for reviewing

@nastra nastra merged commit afb2000 into apache:main Mar 8, 2024
41 checks passed
@nastra nastra deleted the test-for-missing-target-namespace-on-table-rename branch March 8, 2024 08:29
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
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