Addressed review comments

apache · Nov 25, 2023 · 13e4a4f · 13e4a4f
1 parent 6c78294
commit 13e4a4f
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 23 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
@@ -265,7 +265,9 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
       // in case of table already exists,
       // Hive rename operation throws exception as
       // java.lang.RuntimeException:InvalidOperationException(message:new table <> already exists)
-      if (e.getMessage().contains(String.format("new table %s already exists)", to))) {
+      if (e.getCause() instanceof InvalidOperationException
+          && e.getCause().getMessage() != null
+          && e.getCause().getMessage().contains(String.format("new table %s already exists", to))) {
         throw new org.apache.iceberg.exceptions.AlreadyExistsException(
             "Table already exists: %s", to);
       }

diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
@@ -209,7 +209,7 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
       String baseMetadataLocation = base != null ? base.metadataFileLocation() : null;
       if (!Objects.equals(baseMetadataLocation, metadataLocation)) {
         throw new CommitFailedException(
-            "Cannot commit, Base metadata location '%s' is not same as the current table metadata location '%s' for %s.%s",
+            "Cannot commit: Base metadata location '%s' is not same as the current table metadata location '%s' for %s.%s",
             baseMetadataLocation, metadataLocation, database, tableName);
       }
 

diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreTest.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreTest.java
@@ -31,9 +31,6 @@
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 
-/*
- * This meta-setup has been deprecated; use {@link HiveMetastoreExtension} instead.
- * */
 @Deprecated
 public abstract class HiveMetastoreTest {
 

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
@@ -92,12 +92,6 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
-/**
- * Run all the tests from abstract of {@link CatalogTests}. Also, a few specific tests for HIVE too.
- * There could be some duplicated tests that are already being covered with {@link CatalogTests}
- * //TODO: remove duplicate tests with {@link CatalogTests}.Also use the DB/TABLE/SCHEMA from {@link
- * CatalogTests}
- */
 public class TestHiveCatalog extends CatalogTests<HiveCatalog> {
   private static ImmutableMap meta =
       ImmutableMap.of(
@@ -115,7 +109,7 @@ public class TestHiveCatalog extends CatalogTests<HiveCatalog> {
       new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
 
   @BeforeEach
-  public void setup() {
+  public void beforeEach() {
     catalog =
         (HiveCatalog)
             CatalogUtil.loadCatalog(
@@ -217,8 +211,8 @@ public void testInitialize() {
     assertThatNoException()
         .isThrownBy(
             () -> {
-              HiveCatalog catalog = new HiveCatalog();
-              catalog.initialize("hive", Maps.newHashMap());
+              HiveCatalog hiveCatalog = new HiveCatalog();
+              hiveCatalog.initialize("hive", Maps.newHashMap());
             });
   }
 
@@ -227,8 +221,8 @@ public void testToStringWithoutSetConf() {
     assertThatNoException()
         .isThrownBy(
             () -> {
-              HiveCatalog catalog = new HiveCatalog();
-              catalog.toString();
+              HiveCatalog hiveCatalog = new HiveCatalog();
+              hiveCatalog.toString();
             });
   }
 
@@ -237,11 +231,12 @@ public void testInitializeCatalogWithProperties() {
     Map<String, String> properties = Maps.newHashMap();
     properties.put("uri", "thrift://examplehost:9083");
     properties.put("warehouse", "/user/hive/testwarehouse");
-    HiveCatalog catalog = new HiveCatalog();
-    catalog.initialize("hive", properties);
+    HiveCatalog hiveCatalog = new HiveCatalog();
+    hiveCatalog.initialize("hive", properties);
 
-    assertThat(catalog.getConf().get("hive.metastore.uris")).isEqualTo("thrift://examplehost:9083");
-    assertThat(catalog.getConf().get("hive.metastore.warehouse.dir"))
+    assertThat(hiveCatalog.getConf().get("hive.metastore.uris"))
+        .isEqualTo("thrift://examplehost:9083");
+    assertThat(hiveCatalog.getConf().get("hive.metastore.warehouse.dir"))
         .isEqualTo("/user/hive/testwarehouse");
   }
 
@@ -1184,10 +1179,10 @@ public void testDatabaseLocationWithSlashInWarehouseDir() {
     // With a trailing slash
     conf.set("hive.metastore.warehouse.dir", "s3://bucket/");
 
-    HiveCatalog catalog = new HiveCatalog();
-    catalog.setConf(conf);
+    HiveCatalog hiveCatalog = new HiveCatalog();
+    hiveCatalog.setConf(conf);
 
-    Database database = catalog.convertToDatabase(Namespace.of("database"), ImmutableMap.of());
+    Database database = hiveCatalog.convertToDatabase(Namespace.of("database"), ImmutableMap.of());
 
     assertThat(database.getLocationUri()).isEqualTo("s3://bucket/database.db");
   }