Core: Move UpdateRequirement out of rest package #7750

nastra · 2023-06-01T10:19:17Z

This currently depends on #7741.

The goal of this PR is to make UpdateRequirement available outside of the rest package layer, so that we can use it for #7569.

The following things have been done in this PR:

create org.apache.iceberg.UpdateRequirement (copy of org.apache.iceberg.rest.requests.UpdateTableRequest.UpdateRequirement) and org.apache.iceberg.UpdateRequirementParser (copy of org.apache.iceberg.rest.requests.UpdateRequirementParser)
deprecate org.apache.iceberg.rest.requests.UpdateTableRequest.UpdateRequirement and org.apache.iceberg.rest.requests.UpdateTableRequest
replace org.apache.iceberg.rest.requests.UpdateTableRequest with org.apache.iceberg.rest.requests.CommitTableRequest which has the exact same structure, but takes org.apache.iceberg.UpdateRequirement
switch existing usage of org.apache.iceberg.rest.requests.UpdateTableRequest.UpdateRequirement to org.apache.iceberg.UpdateRequirement
switch existing usage of org.apache.iceberg.rest.requests.UpdateTableRequest to org.apache.iceberg.CommitTableRequest

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

rdblue · 2023-06-04T18:00:32Z

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

+      }
+    }
+
+    public List<UpdateRequirement> build() {


I don't think the builder makes much sense when these classes are public. The builder was originally creating the UpdateTableRequest, which needed a list of both changes and requirements. Now, this is only producing a list of requirements from a list of changes so the builder pattern is unnecessary.

I think the cleanest path forward is to try to match the MetadataUpdate API here. Let's keep the constructors for these classes public like MetadataUpdate classes and add logic for converting a collection of updates into a collection of requirements elsewhere.

I've made all the constructors public in UpdateRequirement and added UpdateRequirements with static methods that would then produce a List<UpdateRequirement from a List<MetadataUpdate>.
We don't expose a builder anymore in the API, but UpdateRequirements still uses one internally, since it needs to maintain some state when producing requirements from a given list of updates.

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

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

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

rdblue · 2023-06-04T18:26:24Z

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

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface CommitTableRequest extends RESTRequest {


I'm wondering if we need a new CommitTableRequest or if it would be fine to continue using UpdateTableRequest. The breaking change is going to be removing the List<UpdateTableRequest.UpdateRequirement> requirements() method, but the UpdateRequirement interface is also deprecated and will be removed.

Since source compatibility is already going to break, I don't see a problem with moving directly from List<UpdateTableRequest.UpdateRequirement> requirements() to List<UpdateRequirement> requirements(). The requirements that are returned will implement both interfaces until the older UpdateTableRequest.UpdateRequirement is removed.

if we 're ok breaking source compability, then we can update UpdateTableRequest directly and don't need CommitTableRequest

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

rdblue · 2023-06-12T16:05:14Z

.palantir/revapi.yml

+      old: "method java.util.List<org.apache.iceberg.rest.requests.UpdateTableRequest.UpdateRequirement>\
+        \ org.apache.iceberg.rest.requests.UpdateTableRequest::requirements()"
+      new: "method java.util.List<org.apache.iceberg.UpdateRequirement> org.apache.iceberg.rest.requests.UpdateTableRequest::requirements()"
+      justification: "Accepted src API break by moving UpdateTableRequest out of REST"


This is only an API breaking change if we return a different implementation. Since we are still returning UpdateTableRequest.UpdateRequirement, I think it is not going to be a binary-breaking.

correct, this is not binary-breaking: SOURCE: BREAKING, BINARY: NON_BREAKING

Right, I think it would just be better if the justification were a little more clear that we don't expect it to break anything.

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

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

rdblue · 2023-06-12T23:52:40Z

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

+                .addAll(createChanges)
+                .addAll(metadata.changes())
+                .build();
+        requirements = UpdateRequirements.forCreateTable(updates);


Looks good!

rdblue · 2023-06-12T23:56:37Z

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

-    baseChanges.forEach(requestBuilder::update);
-    metadata.changes().forEach(requestBuilder::update);
-    UpdateTableRequest request = requestBuilder.build();
+    UpdateTableRequest request = new UpdateTableRequest(requirements, updates);


The other request classes use a builder. I think that the intent here is to use the public constructor because the builder changed substantially and can't be updated to work for both. It would be nice to use a builder later, but I can see why this needs to be public and used at the moment.

yes exactly, the builder changed substantially. In the long run I think we can add a builder again

rdblue · 2023-06-13T00:05:07Z

core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java

+        .hasSize(2)
+        .hasOnlyElementsOfTypes(
+            UpdateRequirement.AssertTableUUID.class,
+            UpdateRequirement.AssertLastAssignedFieldId.class);


I don't think that these tests are sufficient. The assertions must be based on the base metadata's state. So the AssertTableUUID must contain the base metadata's UUID. And the last assigned field ID assertion should contain the base metadata's last column ID. I think the implementation is correct, but this would not catch some bugs introduced by updating the implementation.

makes sense and I've updated all tests to check values. I've also added more tests that check the failure conditions. Now we have full test coverage of all code branches in UpdateRequirement via TestUpdateRequirements

rdblue · 2023-06-13T00:08:44Z

@nastra, this is looking really close. I think the only issue is validating that the assertions are created correctly from the base table state, not just that the correct type of requirements are created. I guess this was being tested before by the REST catalog tests since these were embedded, but now that this is stand-alone logic I'd prefer to finish the tests. Also, we should make sure that there are tests for all of the paths in UpdateRequirements including when requirements are omitted because the table state has already changed (for example, two SetCurrentSchema updates).

core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java

rdblue · 2023-06-16T18:00:36Z

Thanks, @nastra!

github-actions bot added core OPENAPI labels Jun 1, 2023

nastra mentioned this pull request Jun 1, 2023

Core: Add REST API for committing changes against multiple tables #7569

Merged

nastra force-pushed the update-requirements-refactoring branch from dd0b516 to 4bcc34a Compare June 2, 2023 07:29