Core: Implement BaseMetastoreCatalog.registerTable() #5037

Mehul2500 · 2022-06-14T20:39:17Z

For migrating tables between different catalogs, need is there for a registerTable() function on destination catalog side.
Thus, planned to implement the registerTable() function in BaseMetastoreCatalog.

Closes #4995

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

ajantha-bhat · 2022-06-21T09:51:50Z

@pvary , @openinx, @szehon-ho, @RussellSpitzer , @rdblue : Can one of the maintainers please approve the first-time contributor flow for this PR?

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

pvary · 2022-06-21T11:16:12Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

@@ -356,6 +360,81 @@ public void testListTables() {
    Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
  }

+  private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {


Could we separate these tests for Nessie specific and general ones?
Could we run the "general" tests on every catalog, and move the testRegisterTable, and testRegisterExistingTable from HiveTableTest to this general place?

These test cases are specific to Nessie.

If I understand correctly, the PR moves the HiveCatalog.registerTable to the BaseMetastoreCatalog so it will be available for every Catalog which is inheriting from BaseMetastoreCatalog. So we should have a test for testing all of the catalogs that implement the registerTable that the method is working as expected.
I understand that there are some Nessie specific test cases, but I think the basic functionality should be available and tested for all of the catalogs.

I will add the said test-cases and would ping you then, thanks.

I think we still miss:

DynamoDbCatalog

GlueCatalog

I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like TestCatalog but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for registerTable() is a good candidate for these common tests.

I have put in test cases for Glue and DynamoDb as "Ignore" because I currently do not have an AWS account for testing them. I working on it.
Also, I would address the general Catalog API test in a follow-up PR.

I got my AWS account setup, and have amended the test cases in Glue and DynamoDb Catalog, @pvary please have a look over the test cases.

I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like TestCatalog but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for registerTable() is a good candidate for these common tests.

We do have CatalogTests, which is what TestRESTCatalog is built off of. It’s nice in that it has some methods for subclasses to declare if the catalog being tested supports certain features, so individual tests can exit early if they don’t support an optional feature (similar to JUnit Assume).

For example if the catalog in question doesn’t support namespace properties.

Might be good to see if that can be made to fit this need or can otherwise get some ideas from it. I believe Nessie also implements these tests.

A good suggestion of using CatalogTests, but the duplicated test code referred to under these comments above shall not be resolved using CatalogTests.
Reason:
The duplicated test code is with two of the test cases, i.e., testRegisterTable() and testRegisterExistingTable() in three of the catalog test files.

Jdbc Catalog

Ecs Catalog

Hadoop Catalog

Out of these only the Jdbc catalog is one which can use CatalogTests, others do not use CatalogTests.
Thus, it won't be helpful to consider CatalogTests under this comment.

@pvary Also has mentioned that we must consider a Catalog API test similar to CatalogTests, but this time for all the catalogs,
but as decided we must come up with a follow-up PR, here, let's only focus on the registerTable functionality in BaseMetastoreCatalog.

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java

core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java

aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

ajantha-bhat

LGTM

Manually verified the integration tests with my AWS account.
Agree with @pvary that test cases can be refactored into common class and other class should extend it to avoid duplication. But this can be done in follow up PR.

Mehul2500 · 2022-07-11T06:47:26Z

@pvary and @rdblue could you please review this PR.

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java

pvary

Let's see if all of us agree, that is is ok to do the Catalog API test in a different PR.
If no objections arrive, we can merge this PR in a few days

pvary · 2022-07-22T09:04:17Z

Merged the PR.
Thanks for the work @Mehul2500 and @ajantha-bhat for the review.

@Mehul2500 if you have time please go ahead with the CatalogAPI test PR.

Thanks,
Peter

github-actions bot added core hive NESSIE labels Jun 14, 2022

ajantha-bhat reviewed Jun 15, 2022

View reviewed changes

Mehul2500 changed the title ~~[WIP] Implementation of BaseMetastoreCatalog.registerTable()~~ Implementation of BaseMetastoreCatalog.registerTable() Jun 16, 2022

Mehul2500 changed the title ~~Implementation of BaseMetastoreCatalog.registerTable()~~ Core: Implement BaseMetastoreCatalog.registerTable() Jun 16, 2022

ajantha-bhat reviewed Jun 20, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Show resolved Hide resolved

ajantha-bhat reviewed Jun 20, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

ajantha-bhat reviewed Jun 20, 2022

View reviewed changes

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java Outdated Show resolved Hide resolved

ajantha-bhat reviewed Jun 20, 2022

View reviewed changes

ajantha-bhat reviewed Jun 21, 2022

View reviewed changes

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java Outdated Show resolved Hide resolved

pvary reviewed Jun 21, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

pvary reviewed Jun 21, 2022

View reviewed changes

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java Outdated Show resolved Hide resolved

pvary reviewed Jun 21, 2022

View reviewed changes

implemented changes in BaseMetastoreCatalog

97c5a88

Mehul2500 force-pushed the master branch from d7bc501 to 97c5a88 Compare June 24, 2022 10:36

self review

2654ec4

Mehul2500 mentioned this pull request Jun 26, 2022

AWS: support registerTable in GlueCatalog #4099

Closed

Mehul2500 added 2 commits June 29, 2022 15:19

added testcases for register table in Ecs, Jdbc and Hadoop Catalog

3a18e8b

self review

a7d4b53

github-actions bot added the DELL label Jun 29, 2022

rdblue reviewed Jun 29, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 29, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

added testcases for Glue, DynamoDb Catalog

65a33be

Mehul2500 force-pushed the master branch from 89ce965 to 65a33be Compare July 4, 2022 10:01

github-actions bot added the AWS label Jul 4, 2022

self review

984115b

ajantha-bhat reviewed Jul 5, 2022

View reviewed changes

added testcases for registerTable() in Glue and DynamoDb Catalog

58e6385

Mehul2500 added 2 commits July 7, 2022 17:23

self review

1cc00f3

self review

8f9d9c9

ajantha-bhat reviewed Jul 8, 2022

View reviewed changes

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java Outdated Show resolved Hide resolved

ajantha-bhat approved these changes Jul 8, 2022

View reviewed changes

self review

d2ac693

Mehul2500 requested a review from rdblue July 10, 2022 10:59

nastra reviewed Jul 13, 2022

View reviewed changes

dimas-b reviewed Jul 13, 2022

View reviewed changes

Mehul2500 added 2 commits July 14, 2022 02:58

updated

ec18ade

updated in accordance to the review comments

3ef237a

Mehul2500 mentioned this pull request Jul 18, 2022

Core : Catalog Tables Migration API #5297

Closed

pvary approved these changes Jul 20, 2022

View reviewed changes

pvary merged commit 3c0fd8f into apache:master Jul 22, 2022

ajantha-bhat mentioned this pull request Jul 28, 2022

Support force option on RegisterTable procedure #5327

Closed

ajantha-bhat mentioned this pull request Aug 10, 2022

Core: Support bulk table migration from one catalog to another #5492

Closed

nastra mentioned this pull request Aug 19, 2022

Accessing Iceberg tables without catalog #5512

Closed

InvisibleProgrammer mentioned this pull request Dec 16, 2022

HIVE-26822: port changes before spotless apache/hive#3857

Closed

ajantha-bhat mentioned this pull request Feb 9, 2023

Spark: Update register table procedure test case for all the catalogs #6787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Implement BaseMetastoreCatalog.registerTable() #5037

Core: Implement BaseMetastoreCatalog.registerTable() #5037

Mehul2500 commented Jun 14, 2022 •

edited

Loading

ajantha-bhat commented Jun 21, 2022

pvary Jun 21, 2022

Mehul2500 Jun 24, 2022

pvary Jun 24, 2022

Mehul2500 Jun 24, 2022

pvary Jun 29, 2022

Mehul2500 Jul 4, 2022 •

edited

Loading

Mehul2500 Jul 6, 2022

kbendick Jul 13, 2022 •

edited

Loading

Mehul2500 Jul 14, 2022 •

edited

Loading

ajantha-bhat left a comment

Mehul2500 commented Jul 11, 2022

pvary left a comment

pvary commented Jul 22, 2022

Core: Implement BaseMetastoreCatalog.registerTable() #5037

Core: Implement BaseMetastoreCatalog.registerTable() #5037

Conversation

Mehul2500 commented Jun 14, 2022 • edited Loading

ajantha-bhat commented Jun 21, 2022

pvary Jun 21, 2022

Choose a reason for hiding this comment

Mehul2500 Jun 24, 2022

Choose a reason for hiding this comment

pvary Jun 24, 2022

Choose a reason for hiding this comment

Mehul2500 Jun 24, 2022

Choose a reason for hiding this comment

pvary Jun 29, 2022

Choose a reason for hiding this comment

Mehul2500 Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Mehul2500 Jul 6, 2022

Choose a reason for hiding this comment

kbendick Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Mehul2500 Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

ajantha-bhat left a comment

Choose a reason for hiding this comment

Mehul2500 commented Jul 11, 2022

pvary left a comment

Choose a reason for hiding this comment

pvary commented Jul 22, 2022

Mehul2500 commented Jun 14, 2022 •

edited

Loading

Mehul2500 Jul 4, 2022 •

edited

Loading

kbendick Jul 13, 2022 •

edited

Loading

Mehul2500 Jul 14, 2022 •

edited

Loading