Hive: Unwrap RuntimeException for Hive TException with alter table (#…

…9432)

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

@@ -251,9 +251,14 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
     } catch (NoSuchObjectException e) {
       throw new NoSuchTableException("Table does not exist: %s", from);
 
-    } catch (AlreadyExistsException e) {
-      throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-          "Table already exists: %s", to);
+    } catch (InvalidOperationException e) {
+      if (e.getMessage() != null
+          && e.getMessage().contains(String.format("new table %s already exists", to))) {
+        throw new org.apache.iceberg.exceptions.AlreadyExistsException(
+            "Table already exists: %s", to);
+      } else {
+        throw new RuntimeException("Failed to rename " + from + " to " + to, e);
+      }
 
     } catch (TException e) {
       throw new RuntimeException("Failed to rename " + from + " to " + to, e);

hive-metastore/src/main/java/org/apache/iceberg/hive/MetastoreUtil.java

@@ -26,6 +26,7 @@
 import org.apache.iceberg.common.DynMethods;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.thrift.TException;
 
 public class MetastoreUtil {
   private static final DynMethods.UnboundMethod ALTER_TABLE =
@@ -54,7 +55,7 @@ private MetastoreUtil() {}
    * context will be set in a way that turns off stats updates to avoid recursive file listing.
    */
   public static void alterTable(
-      IMetaStoreClient client, String databaseName, String tblName, Table table) {
+      IMetaStoreClient client, String databaseName, String tblName, Table table) throws TException {
     alterTable(client, databaseName, tblName, table, ImmutableMap.of());
   }
 
@@ -67,11 +68,21 @@ public static void alterTable(
       String databaseName,
       String tblName,
       Table table,
-      Map<String, String> extraEnv) {
+      Map<String, String> extraEnv)
+      throws TException {
     Map<String, String> env = Maps.newHashMapWithExpectedSize(extraEnv.size() + 1);
     env.putAll(extraEnv);
     env.put(StatsSetupConst.DO_NOT_UPDATE_STATS, StatsSetupConst.TRUE);
 
-    ALTER_TABLE.invoke(client, databaseName, tblName, table, new EnvironmentContext(env));
+    try {
+      ALTER_TABLE.invoke(client, databaseName, tblName, table, new EnvironmentContext(env));
+    } catch (RuntimeException e) {
+      // TException would be wrapped into RuntimeException during reflection
+      if (e.getCause() instanceof TException) {
+        throw (TException) e.getCause();
+      } else {
+        throw e;
+      }
+    }
   }
 }

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -52,7 +52,6 @@
 import org.apache.iceberg.DataFile;
 import org.apache.iceberg.DataFiles;
 import org.apache.iceberg.FileFormat;
-import org.apache.iceberg.HasTableOperations;
 import org.apache.iceberg.PartitionSpec;
 import org.apache.iceberg.PartitionSpecParser;
 import org.apache.iceberg.Schema;
@@ -82,7 +81,6 @@
 import org.apache.iceberg.types.Types;
 import org.apache.iceberg.util.JsonUtil;
 import org.apache.thrift.TException;
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -1181,55 +1179,4 @@ public void testDatabaseLocationWithSlashInWarehouseDir() {
 
     assertThat(database.getLocationUri()).isEqualTo("s3://bucket/database.db");
   }
-
-  // TODO: This test should be removed after fix of https://github.com/apache/iceberg/issues/9289.
-  @Test
-  @Override
-  public void testRenameTableDestinationTableAlreadyExists() {
-    Namespace ns = Namespace.of("newdb");
-    TableIdentifier renamedTable = TableIdentifier.of(ns, "table_renamed");
-
-    if (requiresNamespaceCreate()) {
-      catalog.createNamespace(ns);
-    }
-
-    Assertions.assertThat(catalog.tableExists(TABLE))
-        .as("Source table should not exist before create")
-        .isFalse();
-
-    catalog.buildTable(TABLE, SCHEMA).create();
-    Assertions.assertThat(catalog.tableExists(TABLE))
-        .as("Source table should exist after create")
-        .isTrue();
-
-    Assertions.assertThat(catalog.tableExists(renamedTable))
-        .as("Destination table should not exist before create")
-        .isFalse();
-
-    catalog.buildTable(renamedTable, SCHEMA).create();
-    Assertions.assertThat(catalog.tableExists(renamedTable))
-        .as("Destination table should exist after create")
-        .isTrue();
-
-    // With fix of issues#9289,it should match with CatalogTests and expect
-    // AlreadyExistsException.class
-    // and message should contain as "Table already exists"
-    Assertions.assertThatThrownBy(() -> catalog.renameTable(TABLE, renamedTable))
-        .isInstanceOf(RuntimeException.class)
-        .hasMessageContaining("new table newdb.table_renamed already exists");
-    Assertions.assertThat(catalog.tableExists(TABLE))
-        .as("Source table should still exist after failed rename")
-        .isTrue();
-    Assertions.assertThat(catalog.tableExists(renamedTable))
-        .as("Destination table should still exist after failed rename")
-        .isTrue();
-
-    String sourceTableUUID =
-        ((HasTableOperations) catalog.loadTable(TABLE)).operations().current().uuid();
-    String destinationTableUUID =
-        ((HasTableOperations) catalog.loadTable(renamedTable)).operations().current().uuid();
-    Assertions.assertThat(sourceTableUUID)
-        .as("Source and destination table should remain distinct after failed rename")
-        .isNotEqualTo(destinationTableUUID);
-  }
 }