From d7456e9cf0051d67f8e46b75fff8b0edd44dc2c2 Mon Sep 17 00:00:00 2001 From: nk1506 Date: Wed, 25 Oct 2023 16:25:47 +0530 Subject: [PATCH] Hive: Refactor TestHiveCatalog tests to use the core CatalogTests --- build.gradle | 1 + .../apache/iceberg/catalog/CatalogTests.java | 8 ++ .../org/apache/iceberg/hive/HiveCatalog.java | 14 ++- .../iceberg/hive/HiveTableOperations.java | 2 +- .../apache/iceberg/hive/TestHiveCatalog.java | 105 +++++++++--------- 5 files changed, 77 insertions(+), 53 deletions(-) diff --git a/build.gradle b/build.gradle index 2698b00d384d..fd2665855bef 100644 --- a/build.gradle +++ b/build.gradle @@ -717,6 +717,7 @@ project(':iceberg-hive-metastore') { } testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts') + testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts') } } diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java index 835115ad4d9c..28a2a8b9c86a 100644 --- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java @@ -163,6 +163,10 @@ protected boolean supportsNamesWithSlashes() { return true; } + protected boolean supportsNamesWithDot() { + return true; + } + @Test public void testCreateNamespace() { C catalog = catalog(); @@ -469,6 +473,8 @@ public void testNamespaceWithSlash() { @Test public void testNamespaceWithDot() { + Assumptions.assumeTrue(supportsNamesWithDot()); + C catalog = catalog(); Namespace withDot = Namespace.of("new.db"); @@ -546,6 +552,8 @@ public void testTableNameWithSlash() { @Test public void testTableNameWithDot() { + Assumptions.assumeTrue(supportsNamesWithDot()); + C catalog = catalog(); TableIdentifier ident = TableIdentifier.of("ns", "ta.ble"); 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 22f5b0b5cf37..33954c7f792c 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 @@ -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 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); 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 index f4b96822d42c..34ef1a796106 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/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); } 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 7ff2bd78a665..b8f0fad7b74a 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 @@ -37,12 +37,14 @@ 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; import java.util.UUID; import org.apache.hadoop.conf.Configuration; 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.hadoop.hive.metastore.api.PrincipalType; import org.apache.hadoop.security.UserGroupInformation; @@ -68,6 +70,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; @@ -82,12 +85,20 @@ import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; import org.apache.thrift.TException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; 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 { private static ImmutableMap meta = ImmutableMap.of( "owner", "apache", @@ -95,6 +106,43 @@ public class TestHiveCatalog extends HiveMetastoreTest { "comment", "iceberg hiveCatalog test"); @TempDir private Path temp; + private HiveCatalog catalog; + private HiveMetaStoreClient metastoreClient; + private HiveConf hiveConf; + private static final String DB_NAME = "hivedb"; + + @BeforeEach + public void before() throws Exception { + HiveMetastoreTest.startMetastore(Collections.emptyMap()); + catalog = HiveMetastoreTest.catalog; + metastoreClient = HiveMetastoreTest.metastoreClient; + hiveConf = HiveMetastoreTest.hiveConf; + } + + @AfterEach + public void after() throws Exception { + HiveMetastoreTest.stopMetastore(); + } + + @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( @@ -354,7 +402,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()); @@ -363,12 +411,12 @@ public void testCreateNamespace() throws Exception { assertThat(database1.getParameters()).containsEntry("group", "iceberg"); assertThat(defaultUri(namespace1)) - .as("There no same location for db and namespace") + .as("Database and namespace don't have the same location") .isEqualTo(database1.getLocationUri()); 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); @@ -382,7 +430,7 @@ public void testCreateNamespace() throws Exception { catalog.createNamespace(namespace2, newMeta); Database database2 = metastoreClient.getDatabase(namespace2.toString()); assertThat(hiveLocalDir) - .as("There no same location for db and namespace") + .as("Database and namespace don't have the same location") .isEqualTo(database2.getLocationUri()); } @@ -507,30 +555,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 +736,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 +862,7 @@ private void removeNamespaceOwnershipAndVerify( } @Test - public void testDropNamespace() throws TException { + public void testDropNamespace() { Namespace namespace = Namespace.of("dbname_drop"); TableIdentifier identifier = TableIdentifier.of(namespace, "table"); Schema schema = getTestSchema();