Skip to content

Commit

Permalink
Hive: Refactor TestHiveCatalog tests to use the core CatalogTests
Browse files Browse the repository at this point in the history
  • Loading branch information
@nk1506
nk1506 committed Oct 29, 2023
1 parent e2b56da commit c9b0078
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 64 deletions.
1 change: 1 addition & 0 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,7 @@ project(':iceberg-hive-metastore') {
}

testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
}
}

Expand Down
8 changes: 8 additions & 0 deletions core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ protected boolean supportsNamesWithSlashes() {
return true;
}

protected boolean supportsNamesWithDot() {
return true;
}

@Test
public void testCreateNamespace() {
C catalog = catalog();
Expand Down Expand Up @@ -469,6 +473,8 @@ public void testNamespaceWithSlash() {

@Test
public void testNamespaceWithDot() {
Assumptions.assumeTrue(supportsNamesWithDot());

C catalog = catalog();

Namespace withDot = Namespace.of("new.db");
Expand Down Expand Up @@ -546,6 +552,8 @@ public void testTableNameWithSlash() {

@Test
public void testTableNameWithDot() {
Assumptions.assumeTrue(supportsNamesWithDot());

C catalog = catalog();

TableIdentifier ident = TableIdentifier.of("ns", "ta.ble");
Expand Down
14 changes: 13 additions & 1 deletion hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
129 changes: 67 additions & 62 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -82,19 +85,64 @@
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<HiveCatalog> {
private static ImmutableMap meta =
ImmutableMap.of(
"owner", "apache",
"group", "iceberg",
"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(
Expand Down Expand Up @@ -166,8 +214,8 @@ public void testInitialize() {
assertThatNoException()
.isThrownBy(
() -> {
HiveCatalog catalog = new HiveCatalog();
catalog.initialize("hive", Maps.newHashMap());
HiveCatalog hiveCatalog = new HiveCatalog();
hiveCatalog.initialize("hive", Maps.newHashMap());
});
}

Expand All @@ -176,8 +224,8 @@ public void testToStringWithoutSetConf() {
assertThatNoException()
.isThrownBy(
() -> {
HiveCatalog catalog = new HiveCatalog();
catalog.toString();
HiveCatalog hiveCatalog = new HiveCatalog();
hiveCatalog.toString();
});
}

Expand All @@ -186,11 +234,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");
}

Expand Down Expand Up @@ -354,7 +403,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());
Expand All @@ -363,12 +412,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);
Expand All @@ -382,7 +431,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());
}

Expand Down Expand Up @@ -507,30 +556,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(
Expand Down Expand Up @@ -712,27 +737,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(
Expand Down Expand Up @@ -859,7 +863,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();
Expand Down Expand Up @@ -1172,10 +1177,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");
}
Expand Down

0 comments on commit c9b0078

Please sign in to comment.