Addressed comments

nk1506 · Mar 29, 2024 · ae19dab · ae19dab
1 parent 10f0eb9
commit ae19dab
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 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
@@ -334,6 +334,7 @@ private List<TableIdentifier> 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<String, String> 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
@@ -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
@@ -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";