Hive: Refactor TestHiveCatalog tests to use the core CatalogTests

build.gradle

@@ -719,6 +719,7 @@ project(':iceberg-hive-metastore') {
     }
 
     testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
+    testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
     testImplementation libs.awaitility
   }
 }

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

@@ -164,6 +164,10 @@ protected boolean supportsNamesWithSlashes() {
     return true;
   }
 
+  protected boolean supportsNamesWithDot() {
+    return true;
+  }
+
   @Test
   public void testCreateNamespace() {
     C catalog = catalog();
@@ -470,6 +474,8 @@ public void testNamespaceWithSlash() {
 
   @Test
   public void testNamespaceWithDot() {
+    Assumptions.assumeTrue(supportsNamesWithDot());
+
     C catalog = catalog();
 
     Namespace withDot = Namespace.of("new.db");
@@ -547,6 +553,8 @@ public void testTableNameWithSlash() {
 
   @Test
   public void testTableNameWithDot() {
+    Assumptions.assumeTrue(supportsNamesWithDot());
+
     C catalog = catalog();
 
     TableIdentifier ident = TableIdentifier.of("ns", "ta.ble");

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

@@ -261,6 +261,15 @@ public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
     } catch (InterruptedException e) {
       Thread.currentThread().interrupt();
       throw new RuntimeException("Interrupted in call to rename", e);
+    } catch (RuntimeException e) {
+      // 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))) {
+        throw new org.apache.iceberg.exceptions.AlreadyExistsException(
+            "Table already exists: %s", to);
+      }
+      throw new RuntimeException("Failed to rename " + from + " to " + to, e);
     }
   }
 
@@ -288,7 +297,7 @@ public void createNamespace(Namespace namespace, Map<String, String> meta) {
 
     } catch (AlreadyExistsException e) {
       throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-          e, "Namespace '%s' already exists!", namespace);
+          e, "Namespace already exists: %s", namespace);
 
     } catch (TException e) {
       throw new RuntimeException(
@@ -500,6 +509,9 @@ protected String defaultWarehouseLocation(TableIdentifier tableIdentifier) {
         return String.format("%s/%s", databaseData.getLocationUri(), tableIdentifier.name());
       }
 
+    } catch (NoSuchObjectException e) {
+      throw new NoSuchNamespaceException(
+          e, "Namespace does not exist: %s", tableIdentifier.namespace().levels()[0]);
     } catch (TException e) {
       throw new RuntimeException(
           String.format("Metastore operation failed for %s", tableIdentifier), e);

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

@@ -217,7 +217,7 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
       String baseMetadataLocation = base != null ? base.metadataFileLocation() : null;
       if (!Objects.equals(baseMetadataLocation, metadataLocation)) {
         throw new CommitFailedException(
-            "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);
       }
 

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

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public final class HiveMetastoreExtension implements AfterEachCallback, BeforeEachCallback {
+
+  static HiveCatalog catalog;
+  static HiveMetaStoreClient metastoreClient;
+  static TestHiveMetastore metastore;
+  static HiveConf hiveConf;
+  private final Map<String, String> hiveConfOverride;
+  static final String DB_NAME = "hivedb";
+
+  public HiveMetastoreExtension(Map<String, String> hiveConfOverride) {
+    this.hiveConfOverride = hiveConfOverride;
+  }
+
+  @Override
+  public void beforeEach(ExtensionContext extensionContext) throws Exception {
+    metastore = new TestHiveMetastore();
+    HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
+    if (hiveConfOverride != null) {
+      for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
+        hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
+      }
+    }
+
+    metastore.start(hiveConfWithOverrides);
+    hiveConf = metastore.hiveConf();
+    metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+
+    String dbPath = metastore.getDatabasePath(DB_NAME);
+    Database db = new Database(DB_NAME, "description", dbPath, Maps.newHashMap());
+    metastoreClient.createDatabase(db);
+
+    catalog =
+        (HiveCatalog)
+            CatalogUtil.loadCatalog(
+                HiveCatalog.class.getName(),
+                CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
+                ImmutableMap.of(
+                    CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
+                    String.valueOf(TimeUnit.SECONDS.toMillis(10))),
+                hiveConfWithOverrides);
+  }
+
+  @Override
+  public void afterEach(ExtensionContext extensionContext) throws Exception {
+    catalog = null;
+    metastoreClient.close();
+    metastoreClient = null;
+    metastore.stop();
+    metastore = null;
+  }
+}

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

@@ -31,6 +31,10 @@
 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 {
 
   protected static final String DB_NAME = "hivedb";

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

@@ -28,6 +28,10 @@
 import static org.apache.iceberg.TableProperties.DEFAULT_SORT_ORDER;
 import static org.apache.iceberg.TableProperties.SNAPSHOT_COUNT;
 import static org.apache.iceberg.expressions.Expressions.bucket;
+import static org.apache.iceberg.hive.HiveMetastoreExtension.DB_NAME;
+import static org.apache.iceberg.hive.HiveMetastoreExtension.catalog;
+import static org.apache.iceberg.hive.HiveMetastoreExtension.hiveConf;
+import static org.apache.iceberg.hive.HiveMetastoreExtension.metastoreClient;
 import static org.apache.iceberg.types.Types.NestedField.required;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatNoException;
@@ -37,6 +41,7 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -68,6 +73,7 @@
 import org.apache.iceberg.Transaction;
 import org.apache.iceberg.UpdateSchema;
 import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.CatalogTests;
 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.catalog.TableIdentifier;
 import org.apache.iceberg.exceptions.AlreadyExistsException;
@@ -83,11 +89,18 @@
 import org.apache.iceberg.util.JsonUtil;
 import org.apache.thrift.TException;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
-public class TestHiveCatalog extends HiveMetastoreTest {
+/**
+ * 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(
           "owner", "apache",
@@ -96,6 +109,30 @@ public class TestHiveCatalog extends HiveMetastoreTest {
 
   @TempDir private Path temp;
 
+  @RegisterExtension
+  public static final HiveMetastoreExtension hiveMetastoreExtension =
+      new HiveMetastoreExtension(Collections.emptyMap());
+
+  @Override
+  protected boolean requiresNamespaceCreate() {
+    return true;
+  }
+
+  @Override
+  protected boolean supportsNamesWithSlashes() {
+    return false;
+  }
+
+  @Override
+  protected boolean supportsNamesWithDot() {
+    return false;
+  }
+
+  @Override
+  protected HiveCatalog catalog() {
+    return catalog;
+  }
+
   private Schema getTestSchema() {
     return new Schema(
         required(1, "id", Types.IntegerType.get(), "unique ID"),
@@ -354,7 +391,7 @@ public void testCreateTableCustomSortOrder() throws Exception {
   }
 
   @Test
-  public void testCreateNamespace() throws Exception {
+  public void testDatabaseAndNamespaceWithLocation() throws Exception {
     Namespace namespace1 = Namespace.of("noLocation");
     catalog.createNamespace(namespace1, meta);
     Database database1 = metastoreClient.getDatabase(namespace1.toString());
@@ -368,7 +405,7 @@ public void testCreateNamespace() throws Exception {
 
     assertThatThrownBy(() -> catalog.createNamespace(namespace1))
         .isInstanceOf(AlreadyExistsException.class)
-        .hasMessage("Namespace '" + namespace1 + "' already exists!");
+        .hasMessage(String.format("Namespace already exists: %s", namespace1));
     String hiveLocalDir = temp.toFile().toURI().toString();
     // remove the trailing slash of the URI
     hiveLocalDir = hiveLocalDir.substring(0, hiveLocalDir.length() - 1);
@@ -507,30 +544,6 @@ public void testNamespaceExists() throws TException {
         .isFalse();
   }
 
-  @Test
-  public void testSetNamespaceProperties() throws TException {
-    Namespace namespace = Namespace.of("dbname_set");
-
-    catalog.createNamespace(namespace, meta);
-    catalog.setProperties(
-        namespace,
-        ImmutableMap.of(
-            "owner", "alter_apache",
-            "test", "test",
-            "location", "file:/data/tmp",
-            "comment", "iceberg test"));
-
-    Database database = metastoreClient.getDatabase(namespace.level(0));
-    assertThat(database.getParameters()).containsEntry("owner", "alter_apache");
-    assertThat(database.getParameters()).containsEntry("test", "test");
-    assertThat(database.getParameters()).containsEntry("group", "iceberg");
-
-    assertThatThrownBy(
-            () -> catalog.setProperties(Namespace.of("db2", "db2", "ns2"), ImmutableMap.of()))
-        .isInstanceOf(NoSuchNamespaceException.class)
-        .hasMessage("Namespace does not exist: db2.db2.ns2");
-  }
-
   @Test
   public void testSetNamespaceOwnership() throws TException {
     setNamespaceOwnershipAndVerify(
@@ -712,27 +725,6 @@ private void setNamespaceOwnershipAndVerify(
     assertThat(database.getOwnerType()).isEqualTo(expectedOwnerTypePostSet);
   }
 
-  @Test
-  public void testRemoveNamespaceProperties() throws TException {
-    Namespace namespace = Namespace.of("dbname_remove");
-
-    catalog.createNamespace(namespace, meta);
-
-    catalog.removeProperties(namespace, ImmutableSet.of("comment", "owner"));
-
-    Database database = metastoreClient.getDatabase(namespace.level(0));
-
-    assertThat(database.getParameters()).doesNotContainKey("owner");
-    assertThat(database.getParameters()).containsEntry("group", "iceberg");
-
-    assertThatThrownBy(
-            () ->
-                catalog.removeProperties(
-                    Namespace.of("db2", "db2", "ns2"), ImmutableSet.of("comment", "owner")))
-        .isInstanceOf(NoSuchNamespaceException.class)
-        .hasMessage("Namespace does not exist: db2.db2.ns2");
-  }
-
   @Test
   public void testRemoveNamespaceOwnership() throws TException, IOException {
     removeNamespaceOwnershipAndVerify(
@@ -859,7 +851,8 @@ private void removeNamespaceOwnershipAndVerify(
   }
 
   @Test
-  public void testDropNamespace() throws TException {
+  @Override
+  public void testDropNamespace() {
     Namespace namespace = Namespace.of("dbname_drop");
     TableIdentifier identifier = TableIdentifier.of(namespace, "table");
     Schema schema = getTestSchema();