Core: Add in RenameTableRequest for the RESTCatalog #4448

kbendick · 2022-03-31T07:45:27Z

Adds RenameTableRequest as a request for the RESTCatalog and adds the call for renameTable to the RESTCatalog.

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

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

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

rdblue

Mostly looks good. I think we should allow empty namespaces and add the additional tests. Thanks, @kbendick!

kbendick · 2022-05-03T07:24:49Z

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

        err -> {
-          if (err instanceof SQLIntegrityConstraintViolationException) {
+          if (err instanceof SQLIntegrityConstraintViolationException ||
+              err.getMessage() != null && err.getMessage().contains("UNIQUE constraint failed")) {


I don't think this test is going to hold up for most JDBC databases.

There's a SQLCode on the error, but it was SQLIte specific in the test runs.

We looked into this and there isn't a good way to do it. In JdbcTableOperations, we do the same thing but check for just "constraint failed". I pasted some links for background in my other comment.

Updated to use the referenced code (with the same comment).

rdblue · 2022-05-03T15:09:55Z

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

+    Preconditions.checkArgument(source != null, "Invalid source table: null");
+    Preconditions.checkArgument(destination != null, "Invalid destination table: null");
+    Preconditions.checkArgument(!source.equals(destination),
+        "Invalid rename table request: source must not be the same as destination");


Should this be invalid or a no-op?

I suppose it can be a no-op given we've been somewhat permissive in terms of what is allowed to come through the requests (so that implementors can have discretion as to how they respond to these situations).

I looked at the Glue catalog, and it doesn't specifically check for this case at all. Though I imagine their server fails at the end.

In terms of making it a no-op, should we just let the client send the request and allow the server to deal with it? Maybe log a warning? Seems somewhat difficult to adjust the control flow from the validate function.

Allowing it to go through and the server can deal with it.

This is in line with some of the other catalogs.

rdblue · 2022-05-03T15:37:35Z

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

  }

+  public static void renameTable(Catalog catalog, RenameTableRequest request) {
+    request.validate();


I don't think other methods in CatalogHandlers run validate. I think that should be done before passing it in here.

Oh good point they don't. Since we're building them via the builder, it's not really necessary. Plus validate is called in the constructor anyway.

rdblue · 2022-05-03T15:38:13Z

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

+      catalog.createNamespace(NS);
+    }
+
+    Assert.assertFalse("Source table should not exist before create", catalog.tableExists(TABLE));


create -> rename.

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

rdblue · 2022-05-03T15:40:13Z

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

+
+    catalog.dropTable(TABLE);
+    catalog.dropTable(RENAMED_TABLE);
+    assertEmpty("Should not contain table after drop", catalog, NS);


These get cleaned up with the catalog, so no need to drop and assert.

rdblue

@kbendick, this looks ready to go to me, except for the unfinished changes from the JDBC catalog issue. If you remove the comments in tests and update the contains check, I'll merge this.

kbendick · 2022-05-03T17:08:16Z

@kbendick, this looks ready to go to me, except for the unfinished changes from the JDBC catalog issue. If you remove the comments in tests and update the contains check, I'll merge this.

Cool. I figured that the JDBC catalog issue was something there wasn't a clean way to resolve.

I will make the changes so we can merge this.

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

rdblue · 2022-05-04T21:57:12Z

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

+    String sourceTableUUID = ((HasTableOperations) catalog.loadTable(TABLE)).operations().current().uuid();
+    String destinationTableUUID = ((HasTableOperations) catalog.loadTable(RENAMED_TABLE)).operations().current().uuid();
+    Assert.assertNotEquals("Source and destination table should remain distinct after failed rename",
+        sourceTableUUID, destinationTableUUID);


Looks good.

rdblue

Looks great. I think there was a WIP comment accidentally left in, but overall looks good to merge.

kbendick · 2022-05-04T22:03:36Z

Looks great. I think there was a WIP comment accidentally left in, but overall looks good to merge.

Just noticed it. Removed!

kbendick · 2022-05-06T17:21:02Z

This should be good to go when you get a chance. Rebased and removed unnecessary commits.

github-actions bot added the core label Mar 31, 2022

kbendick commented Mar 31, 2022

View reviewed changes

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

kbendick commented Mar 31, 2022

View reviewed changes

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

kbendick commented Mar 31, 2022

View reviewed changes

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

kbendick force-pushed the rename-table-request branch from 8ba9b83 to f701064 Compare March 31, 2022 18:51

rdblue requested changes Mar 31, 2022

View reviewed changes

kbendick force-pushed the rename-table-request branch from f701064 to 5db9e19 Compare May 3, 2022 02:22

kbendick commented May 3, 2022

View reviewed changes

rdblue reviewed May 3, 2022

View reviewed changes

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

rdblue reviewed May 3, 2022

View reviewed changes

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

rdblue reviewed May 3, 2022

View reviewed changes

rdblue requested changes May 3, 2022

View reviewed changes

kbendick force-pushed the rename-table-request branch from 3e21a33 to bfe4884 Compare May 3, 2022 17:23

rdblue reviewed May 4, 2022

View reviewed changes

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

rdblue reviewed May 4, 2022

View reviewed changes

rdblue approved these changes May 4, 2022

View reviewed changes

Core: Add in RenameTableRequest for the RESTCatalog

aa7a5cc

kbendick force-pushed the rename-table-request branch from 13a0049 to aa7a5cc Compare May 6, 2022 17:20

rdblue approved these changes May 10, 2022

View reviewed changes

rdblue merged commit 62b01fa into apache:master May 10, 2022

Core: Add in RenameTableRequest for the RESTCatalog #4448

Core: Add in RenameTableRequest for the RESTCatalog #4448

Uh oh!

Conversation

kbendick commented Mar 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbendick May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

kbendick commented May 3, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

kbendick commented May 4, 2022

Uh oh!

kbendick commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kbendick May 4, 2022 •

edited

Loading