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

nk1506 · 2023-10-25T10:56:20Z

Currently HiveTests are not using the Core catalog tests. Ideally unlike other catalog Nessie , JDBC, Hive should also extent the core CatalogTests.
Depends upon feedback other duplicate tests will be removed and remaining will be moved to this new class.

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java

ajantha-bhat · 2023-10-25T11:02:42Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

@@ -261,6 +261,12 @@ 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) {


Can you paste the complete callstack? I am wondering why it is not coming as AlreadyExistsException in line 254

java.lang.AssertionError: Expecting actual throwable to be an instance of: org.apache.iceberg.exceptions.AlreadyExistsException but was: java.lang.RuntimeException: InvalidOperationException(message:new table newdb.table_renamed already exists) at org.apache.iceberg.relocated.com.google.common.base.Throwables.propagate(Throwables.java:231) at org.apache.iceberg.common.DynMethods$UnboundMethod.invoke(DynMethods.java:75) at org.apache.iceberg.hive.MetastoreUtil.alterTable(MetastoreUtil.java:75)

So, this change is needed now because CatalogTests is expecting that particular type of message?

Taking a quick look, I do not like this change.

Why is this RuntimeException?

This code tries to capture this case:

} catch (AlreadyExistsException e) { throw new org.apache.iceberg.exceptions.AlreadyExistsException( "Table already exists: %s", to); }

Do we have checks for the other cases as well?
My first guess would be that MetastoreUtil.alterTable messes up the Hive exceptions, and maybe all of them are off?

Also maybe the exceptions thrown are different with different Hive versions?

@pvary @nk1506 what's the outcome of this? Are we ok with the changes in HiveCatalog or not?
This PR is required for #8907

From my side:

I am not sure Remove redundant error propagation check. #9143 is generally ok - maybe there were some reason behind the RuntimeException, which I am not aware of.

I prefer not to have to unwrap the RuntimeException in exception handling - We need to understand why it is needed, and remove the root cause

@pvary @nastra is it a good idea for now to override the corresponding test cases at TestHiveCatalog. and remove this specific RuntimeException handling ?
Currently there is only one test cases which is failing testRenameTableDestinationTableAlreadyExists.
It is failing here.

With a backlog issue we can fix this RuntimeException issue.
WDYT ?

I'm ok ignoring testRenameTableDestinationTableAlreadyExists() for TestHiveCatalog for now and revert the changes here and then open an issue to follow-up on improving this behavior.

Created an issue to track the same. @nastra , Please assign this issue to me. Meantime, I will look into this.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalogNew.java

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalogNew.java

ajantha-bhat · 2023-10-27T07:02:57Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

@@ -261,6 +261,12 @@ 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) {


So, this change is needed now because CatalogTests is expecting that particular type of message?

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java

ajantha-bhat · 2023-10-27T07:06:08Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+/**
+ * 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


Can you please check and remove in this PR only? I don't think it should be a separate change.

do we still need this comment?

I have another patch in progress which will do these cleanup.

independently from the other PR we can remove the entire comment block here as I don't think we need it

@nk1506 can you please go ahead and remove this entire comment block?

ajantha-bhat · 2023-10-27T08:10:53Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

    String location = temp.resolve("tbl").toString();

    try {
      Table table =
-          catalog
+          catalog()


If you keep a local variable HiveCatalog catalog and initialize it in before(), you don't have to change to a method call everywhere;

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

nk1506 · 2023-10-29T05:34:19Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -859,7 +862,7 @@ private void removeNamespaceOwnershipAndVerify(
  }

  @Test
-  public void testDropNamespace() throws TException {
+  public void testDropNamespace() {


Since it's overriding with Base class testDropNamespace

it's better to rename the method rather than override it, since it's testing custom logic here

As per previous comment , I reverted the renaming.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

nastra · 2023-10-31T08:01:01Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

+      // 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))) {


I think here we should examine the exact cause rather than just looking at the exception message.

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

pvary · 2023-11-06T14:21:01Z

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreTest.java

+/*
+ * This meta-setup has been deprecated; use {@link HiveMetastoreExtension} instead.
+ * */
+@Deprecated


After this could you please create a PR to remove this class?
Or in this one, if it is trivial...

Thanks @pvary for reviewing this . Yes we have another change is in progress with all the tests related cleanup.
WIP patch

@nk1506 can you please open a proper PR against the Iceberg repo with those changes where subclasses of HiveMetastoreTest are moved to JUnit5?
That being said, I don't think the deprecation here is necessary, since HiveMetastoreTest could also be seen as a Junit4-thing, whereas the HiveMetastoreExtension is what you would use for JUnit5.

Created a PR with jUnit5.

ajantha-bhat

We can have a less disruptive change by having a new constructor for HiveMetastoreExtension that doesn't take DB_NAME and skips DB creation if it is not set.

Only use the new constructor for TestHiveCatalog and manually recreate DB in beforeEach()

nk1506 · 2023-12-13T07:03:12Z

We can have a less disruptive change by having a new constructor for HiveMetastoreExtension that doesn't take DB_NAME and skips DB creation if it is not set.

Only use the new constructor for TestHiveCatalog and manually recreate DB in beforeEach()

I was in the impression to make HiveMetastoreExtension specific to only metastore setup and teardown. All the clients of HiveMetastoreExtension should have responsibility to manage databases and tables.

With overloaded constructor it will have hybrid flavour.

Please share your thoughts.

nastra · 2023-12-13T07:29:34Z

I agree that creating the database inside the extension should be made configurable

nastra · 2023-12-13T07:40:49Z

@nk1506 @ajantha-bhat I've opened #9288 to make the extension configurable.

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

nk1506 · 2023-12-13T09:11:58Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

-
-  @ParameterizedTest
-  @ValueSource(ints = {1, 2})
-  public void testReplaceTxnBuilder(int formatVersion) {


testReplaceTxnBuilder -> testReplaceTransaction

nk1506 · 2023-12-13T09:15:02Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -494,21 +356,6 @@ private void createNamespaceAndVerifyOwnership(
    assertThat(db.getOwnerType()).isEqualTo(expectedOwnerType);
  }

-  @Test
-  public void testListNamespace() throws TException {


testListNamespace -> testListNamespaces .

nk1506 · 2023-12-13T09:15:58Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -535,30 +382,6 @@ public void testNamespaceExists() throws TException {
        .isFalse();
  }

-  @Test
-  public void testSetNamespaceProperties() throws TException {


testSetNamespaceProperties -> testSetNamespaceProperties

nk1506 · 2023-12-13T09:17:13Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -740,27 +563,6 @@ private void setNamespaceOwnershipAndVerify(
    assertThat(database.getOwnerType()).isEqualTo(expectedOwnerTypePostSet);
  }

-  @Test
-  public void testRemoveNamespaceProperties() throws TException {


testRemoveNamespaceProperties -> testRemoveNamespaceProperties

nk1506 · 2023-12-13T09:18:12Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

@@ -1210,35 +1012,4 @@ public void testDatabaseLocationWithSlashInWarehouseDir() {

    assertThat(database.getLocationUri()).isEqualTo("s3://bucket/database.db");
  }
-
-  @Test
-  public void testRegisterTable() {


testRegisterTable -> testRegisterTable

nk1506 · 2023-12-13T09:18:46Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

-  }
-
-  @Test
-  public void testRegisterExistingTable() {


testRegisterExistingTable -> testRegisterExistingTable

nk1506 · 2023-12-13T09:41:40Z

Thanks @ajantha-bhat and @nastra for recommending configurable approach for HiveMetastoreExtension and fixing this. Although I had to make a small fix to extend CatalogTests.

ajantha-bhat

We just have to conclude #8918 (comment)

May be ask in #dev slack channel or in a mailing list.
We can work on it even as a follow up IMO.

rest of the change LGTM.

nastra · 2023-12-13T14:34:39Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+      catalog.createNamespace(ns);
+    }
+
+    Assertions.assertThat(catalog.tableExists(TABLE))


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

nastra

LGTM, @pvary can you also take a final look on this please?

nastra · 2023-12-13T15:23:25Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+    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


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

pvary · 2023-12-14T10:44:38Z

Thanks for all the work you put into this @nk1506 and @nastra!

github-actions bot added core build hive labels Oct 25, 2023

nastra reviewed Oct 25, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreSetup.java Outdated Show resolved Hide resolved

ajantha-bhat reviewed Oct 25, 2023

View reviewed changes

nastra reviewed Oct 25, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalogNew.java Outdated Show resolved Hide resolved

nk1506 mentioned this pull request Oct 26, 2023

Hive: Add View support for HIVE catalog #8907

Closed

nk1506 force-pushed the hive_tests_refactor branch from 34be422 to e9668f8 Compare October 26, 2023 08:51

nk1506 changed the title ~~Hive: Refactor HiveCatalog tests to use the core CatalogTests~~ Hive: Refactor TestHiveCatalog tests to use the core CatalogTests Oct 26, 2023

nk1506 marked this pull request as ready for review October 26, 2023 08:52

ajantha-bhat reviewed Oct 27, 2023

View reviewed changes

nastra reviewed Oct 27, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 27, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 27, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_tests_refactor branch from e9668f8 to d7456e9 Compare October 29, 2023 05:30

nk1506 commented Oct 29, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Show resolved Hide resolved

nk1506 force-pushed the hive_tests_refactor branch from d7456e9 to c9b0078 Compare October 29, 2023 05:45

nastra reviewed Oct 30, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 30, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_tests_refactor branch 4 times, most recently from 530933f to 0c54560 Compare October 31, 2023 07:25

nastra reviewed Oct 31, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2023

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2023

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

nk1506 commented Oct 31, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Nov 2, 2023

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

pvary reviewed Nov 6, 2023

View reviewed changes

nk1506 force-pushed the hive_tests_refactor branch 2 times, most recently from 5fdb2f5 to 13e4a4f Compare November 25, 2023 02:42

nastra mentioned this pull request Dec 12, 2023

Hive: Introduce HiveMetastoreExtension for Hive tests #9282

Merged

nk1506 force-pushed the hive_tests_refactor branch 2 times, most recently from 59d89a4 to 768d7c6 Compare December 12, 2023 18:15

ajantha-bhat suggested changes Dec 13, 2023

View reviewed changes

nastra reviewed Dec 13, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated Show resolved Hide resolved

nastra mentioned this pull request Dec 13, 2023

Hive: Make HiveMetastoreExtension configurable #9288

Merged

nk1506 force-pushed the hive_tests_refactor branch from 768d7c6 to 51a616b Compare December 13, 2023 09:36

nk1506 commented Dec 13, 2023

View reviewed changes

nk1506 force-pushed the hive_tests_refactor branch from 51a616b to 3f69333 Compare December 13, 2023 09:47

ajantha-bhat approved these changes Dec 13, 2023

View reviewed changes

nk1506 mentioned this pull request Dec 13, 2023

Hive: Unwrap RuntimeException for Hive InvalidOperationException #9289

Open

nk1506 force-pushed the hive_tests_refactor branch from 3f69333 to e95be19 Compare December 13, 2023 14:31

nastra reviewed Dec 13, 2023

View reviewed changes

nk1506 force-pushed the hive_tests_refactor branch from e95be19 to 7174c7f Compare December 13, 2023 15:08

nastra approved these changes Dec 13, 2023

View reviewed changes

Hive: Refactor TestHiveCatalog tests to use the core CatalogTests

e9722db

pvary approved these changes Dec 14, 2023

View reviewed changes

nk1506 force-pushed the hive_tests_refactor branch from 7174c7f to e9722db Compare December 14, 2023 14:57

nastra merged commit 5487c17 into apache:main Dec 14, 2023
41 checks passed

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024

Hive: Refactor TestHiveCatalog tests to use CatalogTests (apache#8918)

e393efa

geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024

Hive: Refactor TestHiveCatalog tests to use CatalogTests (apache#8918)

808ce48

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

Hive: Refactor TestHiveCatalog tests to use CatalogTests (apache#8918)

799c10e

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

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

Conversation

nk1506 commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajantha-bhat left a comment • edited Loading

Choose a reason for hiding this comment

nk1506 commented Dec 13, 2023

nastra commented Dec 13, 2023

nastra commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nk1506 commented Dec 13, 2023

ajantha-bhat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Dec 14, 2023

nk1506 commented Oct 25, 2023 •

edited

Loading

nastra Oct 31, 2023 •

edited

Loading

nastra Dec 11, 2023 •

edited

Loading

ajantha-bhat left a comment •

edited

Loading