From 5fdb2f5bfadf6ce01d9a779f3afe698cf013d753 Mon Sep 17 00:00:00 2001 From: nk1506 Date: Tue, 31 Oct 2023 14:49:40 +0530 Subject: [PATCH] Addressed review comments --- .../org/apache/iceberg/hive/HiveCatalog.java | 4 ++- .../iceberg/hive/HiveTableOperations.java | 2 +- .../iceberg/hive/HiveMetastoreTest.java | 3 -- .../apache/iceberg/hive/TestHiveCatalog.java | 31 ++++++++----------- 4 files changed, 17 insertions(+), 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 index 33954c7f792c..513948dec6eb 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 @@ -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 index 34ef1a796106..e23afce89e75 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( - "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 index c82517cea02f..96e32d181a96 100644 --- 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 index 43d52a294438..0818f22e42ca 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 @@ -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 { private static ImmutableMap meta = ImmutableMap.of( @@ -115,7 +109,7 @@ public class TestHiveCatalog extends CatalogTests { 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 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"); }