From ae19dab3578903b5a34dac948116c576cfab7502 Mon Sep 17 00:00:00 2001 From: nk1506 Date: Thu, 28 Mar 2024 20:45:03 +0530 Subject: [PATCH] Addressed comments --- .../org/apache/iceberg/hive/HiveCatalog.java | 34 +++++++------------ .../apache/iceberg/hive/TestHiveCatalog.java | 22 ++++++++++++ .../iceberg/hive/TestHiveViewCatalog.java | 23 ------------- 3 files changed, 35 insertions(+), 44 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java index 7aca552c8af5..42b98ddc70c7 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java @@ -334,6 +334,7 @@ private List listIcebergTables( .collect(Collectors.toList()); } + @SuppressWarnings("checkstyle:CyclomaticComplexity") private void renameTableOrView( TableIdentifier from, TableIdentifier originalTo, @@ -354,6 +355,16 @@ private void renameTableOrView( "Cannot rename %s to %s. Namespace does not exist: %s", from, to, to.namespace()); } + if (tableExists(to)) { + throw new org.apache.iceberg.exceptions.AlreadyExistsException( + "Cannot rename %s to %s. Table already exists", from, to); + } + + if (viewExists(to)) { + throw new org.apache.iceberg.exceptions.AlreadyExistsException( + "Cannot rename %s to %s. View already exists", from, to); + } + String toDatabase = to.namespace().level(0); String fromDatabase = from.namespace().level(0); String fromName = from.name(); @@ -384,7 +395,8 @@ private void renameTableOrView( } catch (InvalidOperationException e) { if (e.getMessage() != null && e.getMessage().contains(String.format("new table %s already exists", to))) { - throwErrorForExistedToContent(from, removeCatalogName(to)); + throw new org.apache.iceberg.exceptions.AlreadyExistsException( + "Table already exists: %s", to); } else { throw new RuntimeException("Failed to rename " + from + " to " + to, e); } @@ -409,26 +421,6 @@ private void validateTableIsIcebergTableOrView( } } - private void throwErrorForExistedToContent(TableIdentifier from, TableIdentifier to) { - String toDatabase = to.namespace().level(0); - try { - Table table = clients.run(client -> client.getTable(toDatabase, to.name())); - throw new org.apache.iceberg.exceptions.AlreadyExistsException( - "Cannot rename %s to %s. %s already exists", - from, - to, - table.getTableType().equalsIgnoreCase(TableType.VIRTUAL_VIEW.name()) - ? HiveOperationsBase.ContentType.VIEW.value() - : HiveOperationsBase.ContentType.TABLE.value()); - } catch (TException e) { - throw new RuntimeException("Failed to load content " + to, e); - - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException("Interrupted in call to load content", e); - } - } - @Override public void createNamespace(Namespace namespace, Map meta) { Preconditions.checkArgument( diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 369ad46c8e49..3389dc24e8df 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -73,6 +73,7 @@ import org.apache.iceberg.exceptions.NamespaceNotEmptyException; import org.apache.iceberg.exceptions.NoSuchNamespaceException; import org.apache.iceberg.exceptions.NoSuchTableException; +import org.apache.iceberg.exceptions.NoSuchViewException; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; import org.apache.iceberg.relocated.com.google.common.collect.Maps; @@ -155,6 +156,27 @@ private Schema getTestSchema() { required(2, "data", Types.StringType.get())); } + @Test + public void testInvalidIdentifiersWithRename() { + TableIdentifier invalidFrom = TableIdentifier.of(Namespace.of("l1", "l2"), "view"); + TableIdentifier validTo = TableIdentifier.of(Namespace.of("l1"), "renamedView"); + assertThatThrownBy(() -> catalog.renameView(invalidFrom, validTo)) + .isInstanceOf(NoSuchViewException.class) + .hasMessageContaining("Invalid identifier: " + invalidFrom); + assertThatThrownBy(() -> catalog.renameTable(invalidFrom, validTo)) + .isInstanceOf(NoSuchTableException.class) + .hasMessageContaining("Invalid identifier: " + invalidFrom); + + TableIdentifier validFrom = TableIdentifier.of(Namespace.of("l1"), "view"); + TableIdentifier invalidTo = TableIdentifier.of(Namespace.of("l1", "l2"), "view"); + assertThatThrownBy(() -> catalog.renameView(validFrom, invalidTo)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid identifier: " + invalidTo); + assertThatThrownBy(() -> catalog.renameTable(validFrom, invalidTo)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid identifier: " + invalidTo); + } + @Test public void testCreateTableBuilder() throws Exception { Schema schema = getTestSchema(); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java index b08c3b24d581..40a4b86ce491 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java @@ -36,8 +36,6 @@ import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.NoSuchIcebergViewException; -import org.apache.iceberg.exceptions.NoSuchTableException; -import org.apache.iceberg.exceptions.NoSuchViewException; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; @@ -89,27 +87,6 @@ protected boolean requiresNamespaceCreate() { return true; } - @Test - public void testInvalidIdentifiersWithRename() { - TableIdentifier invalidFrom = TableIdentifier.of(Namespace.of("l1", "l2"), "view"); - TableIdentifier validTo = TableIdentifier.of(Namespace.of("l1"), "renamedView"); - assertThatThrownBy(() -> catalog.renameView(invalidFrom, validTo)) - .isInstanceOf(NoSuchViewException.class) - .hasMessageContaining("Invalid identifier: " + invalidFrom); - assertThatThrownBy(() -> catalog.renameTable(invalidFrom, validTo)) - .isInstanceOf(NoSuchTableException.class) - .hasMessageContaining("Invalid identifier: " + invalidFrom); - - TableIdentifier validFrom = TableIdentifier.of(Namespace.of("l1"), "view"); - TableIdentifier invalidTo = TableIdentifier.of(Namespace.of("l1", "l2"), "view"); - assertThatThrownBy(() -> catalog.renameView(validFrom, invalidTo)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid identifier: " + invalidTo); - assertThatThrownBy(() -> catalog.renameTable(validFrom, invalidTo)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid identifier: " + invalidTo); - } - @Test public void testHiveViewAndIcebergViewWithSameName() throws TException, IOException { String dbName = "hivedb";