Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hive: Refactor TestHiveCatalog tests to use the core CatalogTests #8918

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Hive: Refactor TestHiveCatalog tests to use the core CatalogTests #8918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,7 @@ project(':iceberg-hive-metastore') {
}

testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts')
testImplementation libs.awaitility
}
}
Expand Down
8 changes: 8 additions & 0 deletions core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ protected boolean supportsNamesWithSlashes() {
return true;
}

protected boolean supportsNamesWithDot() {
return true;
}

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

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

C catalog = catalog();

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

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

C catalog = catalog();

TableIdentifier ident = TableIdentifier.of("ns", "ta.ble");
Expand Down
5 changes: 4 additions & 1 deletion hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,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 +500,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]);
pvary marked this conversation as resolved.
Show resolved Hide resolved
} 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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,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
182 changes: 87 additions & 95 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

import java.io.IOException;
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -65,10 +64,10 @@
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableOperations;
import org.apache.iceberg.TableProperties;
import org.apache.iceberg.TestHelpers;
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 @@ -83,14 +82,19 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.JsonUtil;
import org.apache.thrift.TException;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
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 {
/**
* Run all the tests from abstract of {@link CatalogTests} with few specific tests related to HIVE.
*/
public class TestHiveCatalog extends CatalogTests<HiveCatalog> {
private static ImmutableMap meta =
ImmutableMap.of(
"owner", "apache",
Expand All @@ -104,10 +108,10 @@ public class TestHiveCatalog {

@RegisterExtension
private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
HiveMetastoreExtension.builder().withDatabase(DB_NAME).build();
HiveMetastoreExtension.builder().build();

@BeforeEach
public void before() {
public void before() throws TException {
catalog =
(HiveCatalog)
CatalogUtil.loadCatalog(
Expand All @@ -117,6 +121,34 @@ public void before() {
CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
String.valueOf(TimeUnit.SECONDS.toMillis(10))),
HIVE_METASTORE_EXTENSION.hiveConf());
String dbPath = HIVE_METASTORE_EXTENSION.metastore().getDatabasePath(DB_NAME);
Database db = new Database(DB_NAME, "description", dbPath, Maps.newHashMap());
HIVE_METASTORE_EXTENSION.metastoreClient().createDatabase(db);
}

@AfterEach
public void cleanup() throws Exception {
HIVE_METASTORE_EXTENSION.metastore().reset();
}

@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() {
Expand Down Expand Up @@ -379,7 +411,7 @@ public void testCreateTableCustomSortOrder() throws Exception {
}

@Test
public void testCreateNamespace() throws Exception {
public void testDatabaseAndNamespaceWithLocation() throws Exception {
nastra marked this conversation as resolved.
Show resolved Hide resolved
Namespace namespace1 = Namespace.of("noLocation");
catalog.createNamespace(namespace1, meta);
Database database1 =
Expand All @@ -394,7 +426,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);
Expand Down Expand Up @@ -493,21 +525,6 @@ private void createNamespaceAndVerifyOwnership(
assertThat(db.getOwnerType()).isEqualTo(expectedOwnerType);
}

@Test
public void testListNamespace() throws TException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testListNamespace -> testListNamespaces .

List<Namespace> namespaces;
Namespace namespace1 = Namespace.of("dbname1");
catalog.createNamespace(namespace1, meta);
namespaces = catalog.listNamespaces(namespace1);
assertThat(namespaces).as("Hive db not hive the namespace 'dbname1'").isEmpty();

Namespace namespace2 = Namespace.of("dbname2");
catalog.createNamespace(namespace2, meta);
namespaces = catalog.listNamespaces();

assertThat(namespaces).as("Hive db not hive the namespace 'dbname2'").contains(namespace2);
}

@Test
public void testLoadNamespaceMeta() throws TException {
Namespace namespace = Namespace.of("dbname_load");
Expand All @@ -534,30 +551,6 @@ public void testNamespaceExists() throws TException {
.isFalse();
}

@Test
public void testSetNamespaceProperties() throws TException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testSetNamespaceProperties -> testSetNamespaceProperties

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 = HIVE_METASTORE_EXTENSION.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 @@ -739,27 +732,6 @@ private void setNamespaceOwnershipAndVerify(
assertThat(database.getOwnerType()).isEqualTo(expectedOwnerTypePostSet);
}

@Test
public void testRemoveNamespaceProperties() throws TException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testRemoveNamespaceProperties -> testRemoveNamespaceProperties

Namespace namespace = Namespace.of("dbname_remove");

catalog.createNamespace(namespace, meta);

catalog.removeProperties(namespace, ImmutableSet.of("comment", "owner"));

Database database = HIVE_METASTORE_EXTENSION.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 @@ -886,7 +858,7 @@ private void removeNamespaceOwnershipAndVerify(
}

@Test
public void testDropNamespace() throws TException {
public void dropNamespace() {
Namespace namespace = Namespace.of("dbname_drop");
TableIdentifier identifier = TableIdentifier.of(namespace, "table");
Schema schema = getTestSchema();
Expand Down Expand Up @@ -1210,34 +1182,54 @@ public void testDatabaseLocationWithSlashInWarehouseDir() {
assertThat(database.getLocationUri()).isEqualTo("s3://bucket/database.db");
}

// TODO: This test should be removed after fix of https://github.com/apache/iceberg/issues/9289.
@Test
public void testRegisterTable() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testRegisterTable -> testRegisterTable

TableIdentifier identifier = TableIdentifier.of(DB_NAME, "t1");
catalog.createTable(identifier, getTestSchema());
Table registeringTable = catalog.loadTable(identifier);
catalog.dropTable(identifier, false);
TableOperations ops = ((HasTableOperations) registeringTable).operations();
String metadataLocation = ((HiveTableOperations) ops).currentMetadataLocation();
Table registeredTable = catalog.registerTable(identifier, metadataLocation);
assertThat(registeredTable).isNotNull();
TestHelpers.assertSerializedAndLoadedMetadata(registeringTable, registeredTable);
String expectedMetadataLocation =
((HasTableOperations) registeredTable).operations().current().metadataFileLocation();
assertThat(metadataLocation).isEqualTo(expectedMetadataLocation);
assertThat(catalog.loadTable(identifier)).isNotNull();
assertThat(catalog.dropTable(identifier)).isTrue();
}
@Override
public void testRenameTableDestinationTableAlreadyExists() {
Namespace ns = Namespace.of("newdb");
TableIdentifier renamedTable = TableIdentifier.of(ns, "table_renamed");

@Test
public void testRegisterExistingTable() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testRegisterExistingTable -> testRegisterExistingTable

TableIdentifier identifier = TableIdentifier.of(DB_NAME, "t1");
catalog.createTable(identifier, getTestSchema());
Table registeringTable = catalog.loadTable(identifier);
TableOperations ops = ((HasTableOperations) registeringTable).operations();
String metadataLocation = ((HiveTableOperations) ops).currentMetadataLocation();
assertThatThrownBy(() -> catalog.registerTable(identifier, metadataLocation))
.isInstanceOf(AlreadyExistsException.class)
.hasMessage("Table already exists: hivedb.t1");
assertThat(catalog.dropTable(identifier, true)).isTrue();
if (requiresNamespaceCreate()) {
catalog.createNamespace(ns);
}

Assertions.assertThat(catalog.tableExists(TABLE))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a TODO and mention the issue that addresses this. Also it's not clear what exactly is different from the test in CatalogTests, so please add a comment

.as("Source table should not exist before create")
.isFalse();

catalog.buildTable(TABLE, SCHEMA).create();
Assertions.assertThat(catalog.tableExists(TABLE))
.as("Source table should exist after create")
.isTrue();

Assertions.assertThat(catalog.tableExists(renamedTable))
.as("Destination table should not exist before create")
.isFalse();

catalog.buildTable(renamedTable, SCHEMA).create();
Assertions.assertThat(catalog.tableExists(renamedTable))
.as("Destination table should exist after create")
.isTrue();

// With fix of issues#9289,it should match with CatalogTests and expect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be good to add a newline before this line for better readability

// AlreadyExistsException.class
// and message should contain as "Table already exists"
Assertions.assertThatThrownBy(() -> catalog.renameTable(TABLE, renamedTable))
.isInstanceOf(RuntimeException.class)
.hasMessageContaining("new table newdb.table_renamed already exists");
Assertions.assertThat(catalog.tableExists(TABLE))
.as("Source table should still exist after failed rename")
.isTrue();
Assertions.assertThat(catalog.tableExists(renamedTable))
.as("Destination table should still exist after failed rename")
.isTrue();

String sourceTableUUID =
((HasTableOperations) catalog.loadTable(TABLE)).operations().current().uuid();
String destinationTableUUID =
((HasTableOperations) catalog.loadTable(renamedTable)).operations().current().uuid();
Assertions.assertThat(sourceTableUUID)
.as("Source and destination table should remain distinct after failed rename")
.isNotEqualTo(destinationTableUUID);
}
}