From b3c8edcb0eec59a4de1b2ce96e595e4324227f6c Mon Sep 17 00:00:00 2001 From: nk1506 Date: Sat, 28 Oct 2023 22:27:21 +0530 Subject: [PATCH] Addressed review comments. --- .../apache/iceberg/view/ViewCatalogTests.java | 45 +++++- .../org/apache/iceberg/hive/HiveCatalog.java | 96 +++++------ .../iceberg/hive/HiveMetastoreSetup.java | 73 --------- .../iceberg/hive/TestHiveViewCatalog.java | 153 +----------------- 4 files changed, 81 insertions(+), 286 deletions(-) delete mode 100644 hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java diff --git a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java index 551ecc5755bb..9cabe4f8536c 100644 --- a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java @@ -365,8 +365,15 @@ public void createTableViaTransactionThatAlreadyExistsAsView() { assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); assertThatThrownBy(transaction::commitTransaction) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("View with same name already exists: ns.view"); + .satisfiesAnyOf( + throwable -> + assertThat(throwable) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageStartingWith("Table already exists: ns.view"), + throwable -> + assertThat(throwable) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageStartingWith("View with same name already exists: ns.view")); } @Test @@ -400,8 +407,15 @@ public void replaceTableViaTransactionThatAlreadyExistsAsView() { .buildTable(viewIdentifier, SCHEMA) .replaceTransaction() .commitTransaction()) - .isInstanceOf(NoSuchTableException.class) - .hasMessageStartingWith("Table does not exist: ns.view"); + .satisfiesAnyOf( + throwable -> + assertThat(throwable) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageStartingWith("View with same name already exists: ns.view"), + throwable -> + assertThat(throwable) + .isInstanceOf(NoSuchTableException.class) + .hasMessageStartingWith("Table does not exist: ns.view")); } @Test @@ -465,8 +479,15 @@ public void replaceViewThatAlreadyExistsAsTable() { .withDefaultNamespace(tableIdentifier.namespace()) .withQuery("spark", "select * from ns.tbl") .replace()) - .isInstanceOf(NoSuchViewException.class) - .hasMessageStartingWith("View does not exist: ns.table"); + .satisfiesAnyOf( + throwable -> + assertThat(throwable) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageStartingWith("Table with same name already exists: ns.table"), + throwable -> + assertThat(throwable) + .isInstanceOf(NoSuchViewException.class) + .hasMessageStartingWith("View does not exist: ns.table")); } @Test @@ -715,8 +736,16 @@ public void renameTableTargetAlreadyExistsAsView() { assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); assertThatThrownBy(() -> tableCatalog().renameTable(tableIdentifier, viewIdentifier)) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageContaining("Cannot rename ns.table to ns.view. View already exists"); + .satisfiesAnyOf( + throwable -> + assertThat(throwable) + .isInstanceOf(RuntimeException.class) + .hasMessageContaining("new table ns.view already exists"), + throwable -> + assertThat(throwable) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageContaining( + "Cannot rename ns.table to ns.view. View already exists")); } @Test 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 3d959845877f..8ed70382f728 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 @@ -21,7 +21,9 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.conf.Configurable; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hive.conf.HiveConf; @@ -124,43 +126,13 @@ public void initialize(String inputName, Map properties) { @Override public List listTables(Namespace namespace) { - Preconditions.checkArgument( - isValidateNamespace(namespace), "Missing database in namespace: %s", namespace); - String database = namespace.level(0); try { - List tableNames = clients.run(client -> client.getAllTables(database)); - List tableIdentifiers; - if (listAllTables) { - tableIdentifiers = - tableNames.stream() - .map(t -> TableIdentifier.of(namespace, t)) - .collect(Collectors.toList()); + return listContents(namespace, null, table -> true); } else { - List tableObjects = - clients.run(client -> client.getTableObjectsByName(database, tableNames)); - tableIdentifiers = - tableObjects.stream() - .filter( - table -> - table.getTableType().equalsIgnoreCase(TableType.EXTERNAL_TABLE.name()) - && table.getParameters() != null - && BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE - .equalsIgnoreCase( - table - .getParameters() - .get(BaseMetastoreTableOperations.TABLE_TYPE_PROP))) - .map(table -> TableIdentifier.of(namespace, table.getTableName())) - .collect(Collectors.toList()); + return listContents(namespace, TableType.EXTERNAL_TABLE.name(), icebergPredicate()); } - - LOG.debug( - "Listing of namespace: {} resulted in the following tables: {}", - namespace, - tableIdentifiers); - return tableIdentifiers; - } catch (UnknownDBException e) { throw new NoSuchNamespaceException("Namespace does not exist: %s", namespace); @@ -315,33 +287,8 @@ public boolean dropView(TableIdentifier identifier) { @Override public List listViews(Namespace namespace) { - Preconditions.checkArgument( - isValidateNamespace(namespace), "Missing database in namespace: %s", namespace); - String database = namespace.level(0); - try { - List tableNames = - clients.run(client -> client.getTables(database, "*", TableType.VIRTUAL_VIEW)); - List
tableObjects = - clients.run(client -> client.getTableObjectsByName(database, tableNames)); - List tableIdentifiers = - tableObjects.stream() - .filter( - table -> - table.getParameters() != null - && BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase( - table - .getParameters() - .get(BaseMetastoreTableOperations.TABLE_TYPE_PROP))) - .map(table -> TableIdentifier.of(namespace, table.getTableName())) - .collect(Collectors.toList()); - - LOG.debug( - "Listing of namespace: {} resulted in the following views: {}", - namespace, - tableIdentifiers); - return tableIdentifiers; - + return listContents(namespace, TableType.VIRTUAL_VIEW.name(), icebergPredicate()); } catch (UnknownDBException e) { throw new NoSuchNamespaceException("Namespace does not exist: %s", namespace); @@ -354,6 +301,39 @@ public List listViews(Namespace namespace) { } } + private List listContents( + Namespace namespace, String tableType, Predicate
tablePredicate) + throws TException, InterruptedException { + Preconditions.checkArgument( + isValidateNamespace(namespace), "Missing database in namespace: %s", namespace); + String database = namespace.level(0); + List tableNames = + StringUtils.isNotEmpty(tableType) + ? clients.run(client -> client.getTables(database, "*", TableType.valueOf(tableType))) + : clients.run(client -> client.getAllTables(database)); + List
tableObjects = + clients.run(client -> client.getTableObjectsByName(database, tableNames)); + List tableIdentifiers = + tableObjects.stream() + .filter(tablePredicate) + .map(table -> TableIdentifier.of(namespace, table.getTableName())) + .collect(Collectors.toList()); + + LOG.debug( + "Listing of namespace: {} for table type {} resulted in the following: {}", + namespace, + tableType, + tableIdentifiers); + return tableIdentifiers; + } + + private Predicate
icebergPredicate() { + return table -> + table.getParameters() != null + && BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase( + table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)); + } + @Override @SuppressWarnings("FormatStringAnnotation") public void renameView(TableIdentifier from, TableIdentifier originalTo) { diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java deleted file mode 100644 index 3d806a0a5925..000000000000 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * 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.iceberg.CatalogProperties; -import org.apache.iceberg.CatalogUtil; -import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; - -/** - * Setup HiveMetastore. It does not create any database. All the tests should create a database - * accordingly. It should replace the existing setUp class {@link HiveMetastoreTest} - */ -class HiveMetastoreSetup { - - protected HiveMetaStoreClient metastoreClient; - protected TestHiveMetastore metastore; - protected HiveConf hiveConf; - HiveCatalog catalog; - - HiveMetastoreSetup(Map hiveConfOverride) throws Exception { - metastore = new TestHiveMetastore(); - HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class); - if (hiveConfOverride != null) { - for (Map.Entry kv : hiveConfOverride.entrySet()) { - hiveConfWithOverrides.set(kv.getKey(), kv.getValue()); - } - } - - metastore.start(hiveConfWithOverrides); - hiveConf = metastore.hiveConf(); - metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides); - 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); - } - - void stopMetastore() throws Exception { - try { - metastoreClient.close(); - metastore.stop(); - } finally { - catalog = null; - metastoreClient = null; - metastore = null; - } - } -} diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java index 82af3d01019b..2e80fa817782 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java @@ -18,180 +18,39 @@ */ package org.apache.iceberg.hive; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - import java.util.Collections; -import org.apache.iceberg.Transaction; import org.apache.iceberg.catalog.Catalog; -import org.apache.iceberg.catalog.TableIdentifier; -import org.apache.iceberg.exceptions.AlreadyExistsException; import org.apache.iceberg.view.ViewCatalogTests; -import org.assertj.core.api.Assumptions; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; public class TestHiveViewCatalog extends ViewCatalogTests { - private HiveMetastoreSetup hiveMetastoreSetup; + private HiveCatalog catalog; @BeforeEach public void before() throws Exception { - hiveMetastoreSetup = new HiveMetastoreSetup(Collections.emptyMap()); + HiveMetastoreTest.startMetastore(Collections.emptyMap()); + this.catalog = HiveMetastoreTest.catalog; } @AfterEach public void after() throws Exception { - hiveMetastoreSetup.stopMetastore(); + HiveMetastoreTest.stopMetastore(); } @Override protected HiveCatalog catalog() { - return hiveMetastoreSetup.catalog; + return catalog; } @Override protected Catalog tableCatalog() { - return hiveMetastoreSetup.catalog; + return catalog; } @Override protected boolean requiresNamespaceCreate() { return true; } - - // Override few tests which are using AlreadyExistsException instead of NoSuchViewException - - @Override - @Test - public void replaceTableViaTransactionThatAlreadyExistsAsView() { - Assumptions.assumeThat(catalog()).as("Only valid for catalogs that support tables").isNotNull(); - - TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); - - if (requiresNamespaceCreate()) { - catalog().createNamespace(viewIdentifier.namespace()); - } - - assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); - - catalog() - .buildView(viewIdentifier) - .withSchema(SCHEMA) - .withDefaultNamespace(viewIdentifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .create(); - - assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); - - assertThatThrownBy( - () -> - catalog() - .buildTable(viewIdentifier, SCHEMA) - .replaceTransaction() - .commitTransaction()) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("View with same name already exists: ns.view"); - - assertThat(catalog().dropView(viewIdentifier)).isTrue(); - assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); - } - - @Override - @Test - public void replaceViewThatAlreadyExistsAsTable() { - Assumptions.assumeThat(tableCatalog()) - .as("Only valid for catalogs that support tables") - .isNotNull(); - - TableIdentifier tableIdentifier = TableIdentifier.of("ns", "table"); - - if (requiresNamespaceCreate()) { - catalog().createNamespace(tableIdentifier.namespace()); - } - - assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should not exist").isFalse(); - - tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); - - assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue(); - - assertThatThrownBy( - () -> - catalog() - .buildView(tableIdentifier) - .withSchema(OTHER_SCHEMA) - .withDefaultNamespace(tableIdentifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .replace()) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("Table with same name already exists: ns.table"); - } - - @Override - @Test - public void renameTableTargetAlreadyExistsAsView() { - Assumptions.assumeThat(tableCatalog()) - .as("Only valid for catalogs that support tables") - .isNotNull(); - - TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); - TableIdentifier tableIdentifier = TableIdentifier.of("ns", "table"); - - if (requiresNamespaceCreate()) { - catalog().createNamespace(tableIdentifier.namespace()); - } - - assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should not exist").isFalse(); - - tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); - - assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue(); - - assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); - - catalog() - .buildView(viewIdentifier) - .withSchema(SCHEMA) - .withDefaultNamespace(viewIdentifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .create(); - - assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); - - assertThatThrownBy(() -> tableCatalog().renameTable(tableIdentifier, viewIdentifier)) - .hasMessageContaining("new table ns.view already exists"); - } - - @Override - @Test - public void createTableViaTransactionThatAlreadyExistsAsView() { - Assumptions.assumeThat(tableCatalog()) - .as("Only valid for catalogs that support tables") - .isNotNull(); - - TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); - - if (requiresNamespaceCreate()) { - catalog().createNamespace(viewIdentifier.namespace()); - } - - assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); - - Transaction transaction = tableCatalog().buildTable(viewIdentifier, SCHEMA).createTransaction(); - - catalog() - .buildView(viewIdentifier) - .withSchema(SCHEMA) - .withDefaultNamespace(viewIdentifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .create(); - - assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); - - assertThatThrownBy(transaction::commitTransaction) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("Table already exists: ns.view"); - } }