Core: Support registerTable with REST session catalog #6512

krvikash · 2023-01-02T09:57:41Z

Core: Support registerTable with REST session catalog

krvikash · 2023-01-02T14:02:39Z

CI failure will be fixed with #6511

core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java

nastra · 2023-01-09T15:17:31Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

+        .create();
+  }
+
+  private boolean isValidIdentifier(TableIdentifier tableIdentifier) {


do we need this, given that it always returns true? I know we do the same in BaseMetastoreCatalog where catalog implementations can override this behavior but I don't think we need this here in particular.
You might rather want to call checkIdentifierIsValid(..) in this case.

Yes, I have copied isValidIdentifier from BaseMetastoreCatalog implementation. Thanks for the input. I will use checkIdentifierIsValid over here.

ajantha-bhat · 2023-01-09T15:44:05Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

  @Override
  public void invalidateTable(SessionContext context, TableIdentifier ident) {}

  @Override


Just wondering why can't we remove this method and let it use the parent implementation.
Other catalogs do the same.

oh.. This is SessionCatalog, not the Catalog. So, there is no parent implementation.
I think this logic can be moved to BaseSessionCatalog. So, it can help if there are some more implementations in the future.

Thanks, @ajantha-bhat for the input. Yes, moving implementation logic makes sense to BaseSessionCatalog. I will update the code.

I see that all the method's implementation is in RESTSessionCatalog.java. IMO we should keep registerTable implementation in RESTSessionCatalog.java as well. WDYT?

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

rdblue · 2023-01-10T16:35:28Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

+        .withProperties(tableProperties)
+        .withPartitionSpec(metadata.spec())
+        .withSortOrder(metadata.sortOrder())
+        .create();


This discards all table data and older schemas, specs, and sort orders. I think this needs to preserve the table metadata completely.

rdblue

This is a great start, but the main issue is that it discards the table data and all metadata that isn't current. That's a pretty big problem.

If we want to support register, then I think the protocol itself should support register and accept a complete serialized TableMetadata object, rather than using the change-based API.

krvikash · 2023-01-14T20:08:10Z

If we want to support register, then I think the protocol itself should support register

Hi @rdblue, Thanks for the review. If I understood correctly, you wanted to introduce a new RESTRequest for registerTable similar to what we have CreateTableRequest. I have introduced a new RegisterTableRequest and used it for regsiterTable. I have updated the test case as well where I am verifying verify metadata like specs, sortOrders, history, snapshots ...

Please let me know if this is not what you intended to convey.

krvikash · 2023-01-14T23:01:15Z

CI Failure will be fixed by #6589

TestJdbcCatalog > testRegisterTable() FAILED
    org.junit.ComparisonFailure: Name must match expected:<[test_jdbc_catalog.]newdb.table> but was:<[]newdb.table>
        at org.junit.Assert.assertEquals(Assert.java:117)

TestNessieCatalog > testRegisterTable() FAILED
    org.junit.ComparisonFailure: Name must match expected:<ne[ssie.ne]wdb.table> but was:<ne[]wdb.table>
        at org.junit.Assert.assertEquals(Assert.java:117)

krvikash · 2023-06-20T21:51:31Z

Thanks @rdblue for the review, I have addressed the comments. However, there are a few open conversations.

core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java

krvikash · 2023-06-27T18:43:28Z

(Modified RegisterTableRequest methods)

krvikash · 2023-06-27T18:46:22Z

(resolved conflicts)

core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java

krvikash · 2023-06-27T19:41:13Z

Thanks for reviewing @bryanck, Addressed comment.

nastra

this LGTM, just a few small things to make CI happy

open-api/rest-catalog-open-api.yaml

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java

krvikash · 2023-06-28T18:18:57Z

Thanks @nastra for the review. Addressed comments.

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

rdblue

Almost there! Looks like the only blocker is validating that the original metadata file is used. Just drop that assertion and this should be ready.

krvikash · 2023-07-04T04:45:31Z

@rdblue, thank you for reviewing. I have addressed all comments.

rdblue · 2023-07-04T21:06:31Z

Thanks, @krvikash! Great to have this supported in the REST catalog.

krvikash · 2023-07-05T03:59:58Z

Thanks @rdblue for merging. Thanks @nastra, @ajantha-bhat, @bryanck, @rdblue for the review.

github-actions bot added the core label Jan 2, 2023

krvikash force-pushed the rest-catalog-resiter-table-support branch from e2b1ce6 to 48e9e48 Compare January 2, 2023 11:26

krvikash mentioned this pull request Jan 3, 2023

Add register_table and unregister_table procedures in Iceberg REST catalog trinodb/trino#15512

Closed

krvikash force-pushed the rest-catalog-resiter-table-support branch from 48e9e48 to e12ef3a Compare January 5, 2023 08:31

nastra requested changes Jan 9, 2023

View reviewed changes

ajantha-bhat reviewed Jan 9, 2023

View reviewed changes

krvikash force-pushed the rest-catalog-resiter-table-support branch 2 times, most recently from b35f367 to 9b38a8c Compare January 10, 2023 16:21

rdblue reviewed Jan 10, 2023

View reviewed changes

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

rdblue reviewed Jan 10, 2023

View reviewed changes

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

rdblue reviewed Jan 10, 2023

View reviewed changes

rdblue requested changes Jan 10, 2023

View reviewed changes

krvikash force-pushed the rest-catalog-resiter-table-support branch 3 times, most recently from 8784571 to 1477f27 Compare January 14, 2023 19:38

krvikash mentioned this pull request Jan 14, 2023

Core: Avoid creating new metadata file when registerTable API is used #6591

Merged

krvikash requested review from ajantha-bhat, nastra and rdblue and removed request for ajantha-bhat, nastra and rdblue January 17, 2023 10:29

bryanck reviewed Jun 26, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java Outdated Show resolved Hide resolved

krvikash force-pushed the rest-catalog-resiter-table-support branch from 1e49855 to cd35e0a Compare June 27, 2023 18:46

krvikash force-pushed the rest-catalog-resiter-table-support branch from cd35e0a to 1ee587f Compare June 27, 2023 18:53

bryanck reviewed Jun 27, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java Outdated Show resolved Hide resolved

krvikash force-pushed the rest-catalog-resiter-table-support branch from 1ee587f to 6c86950 Compare June 27, 2023 19:40

nastra reviewed Jun 28, 2023

View reviewed changes

open-api/rest-catalog-open-api.yaml Outdated Show resolved Hide resolved

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java Show resolved Hide resolved

core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java Show resolved Hide resolved

krvikash force-pushed the rest-catalog-resiter-table-support branch from 6c86950 to fa2b2a9 Compare June 28, 2023 18:17

nastra approved these changes Jun 29, 2023

View reviewed changes

nastra requested review from bryanck and rdblue June 29, 2023 06:47

rdblue reviewed Jul 3, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 3, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 3, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 3, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated Show resolved Hide resolved

rdblue requested changes Jul 3, 2023

View reviewed changes

Core: Support registerTable with REST session catalog

4527e8e

krvikash force-pushed the rest-catalog-resiter-table-support branch from fa2b2a9 to 4527e8e Compare July 4, 2023 04:23

krvikash requested a review from rdblue July 4, 2023 18:41

rdblue approved these changes Jul 4, 2023

View reviewed changes

rdblue merged commit f3826bd into apache:master Jul 4, 2023

krvikash deleted the rest-catalog-resiter-table-support branch July 5, 2023 04:00

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

Core: Support registerTable with REST session catalog (apache#6512)

3726842

Core: Support registerTable with REST session catalog #6512

Core: Support registerTable with REST session catalog #6512

Uh oh!

Conversation

krvikash commented Jan 2, 2023

Uh oh!

krvikash commented Jan 2, 2023

Uh oh!

Uh oh!

nastra Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

krvikash Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

krvikash Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

krvikash Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rdblue Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

krvikash commented Jan 14, 2023

Uh oh!

krvikash commented Jan 14, 2023

Uh oh!

krvikash commented Jun 20, 2023

Uh oh!

Uh oh!

krvikash commented Jun 27, 2023

Uh oh!

krvikash commented Jun 27, 2023

Uh oh!

Uh oh!

krvikash commented Jun 27, 2023

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krvikash commented Jun 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

krvikash commented Jul 4, 2023

Uh oh!

rdblue commented Jul 4, 2023

Uh oh!

krvikash commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants