You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/06/01 10:19:19 UTC
[GitHub] [iceberg] nastra opened a new pull request, #7750: Core: Move UpdateRequirement out of rest package
nastra opened a new pull request, #7750:
URL: https://github.com/apache/iceberg/pull/7750
This currently depends on https://github.com/apache/iceberg/pull/7741.
The goal of this PR is to make `UpdateRequirement` available outside of the `rest` package layer, so that we can use it for https://github.com/apache/iceberg/pull/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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1228316200
##########
.palantir/revapi.yml:
##########
@@ -737,6 +737,11 @@ acceptedBreaks:
old: "method void org.apache.iceberg.rest.auth.OAuth2Util.AuthSession::<init>(java.util.Map<java.lang.String,\
\ java.lang.String>, java.lang.String, java.lang.String)"
justification: "Removing deprecations for 1.3.0"
+ - code: "java.method.returnTypeTypeParametersChanged"
+ 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"
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216955586
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
Review Comment:
Why is the constructors public? They weren't public in the original version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227648966
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -287,15 +287,15 @@ private static boolean isCreate(UpdateTableRequest request) {
boolean isCreate =
request.requirements().stream()
.anyMatch(
- UpdateTableRequest.UpdateRequirement.AssertTableDoesNotExist.class::isInstance);
+ org.apache.iceberg.UpdateRequirement.AssertTableDoesNotExist.class::isInstance);
Review Comment:
I manually added this, thinking it would conflict with the old `UpdateRequirement` but imported this now properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227349837
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -101,30 +103,38 @@ public TableMetadata refresh() {
@Override
public void commit(TableMetadata base, TableMetadata metadata) {
- UpdateTableRequest.Builder requestBuilder;
- List<MetadataUpdate> baseChanges;
Consumer<ErrorResponse> errorHandler;
+ List<UpdateRequirement> requirements;
+ List<MetadataUpdate> updates;
switch (updateType) {
case CREATE:
Preconditions.checkState(
base == null, "Invalid base metadata for create transaction, expected null: %s", base);
- requestBuilder = UpdateTableRequest.builderForCreate();
- baseChanges = createChanges;
+ updates =
+ ImmutableList.<MetadataUpdate>builder()
+ .addAll(createChanges)
+ .addAll(metadata.changes())
+ .build();
+ requirements = UpdateRequirements.forCreateTable(updates);
Review Comment:
Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1225220451
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
Review Comment:
tests in the rest package are using some of the constructors, hence I made those public. I'll remove the builder and rework the API to match `MetadataUpdate`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#issuecomment-1595054757
Thanks, @nastra!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1226832669
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
Review Comment:
I've made all constructors public and removed the Builder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#issuecomment-1588274596
@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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #7750:
URL: https://github.com/apache/iceberg/pull/7750
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227655313
##########
core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java:
##########
@@ -110,6 +118,28 @@ public void serialize(
}
}
+ public static class UpdateReqDeserializer
Review Comment:
good call, yes I think it makes sense to make these non-public.
There are some Ser/De classes that are used directly, such as `ErrorResponseSerializer` / `ErrorResponseDeserializer`. Those are being used in `S3ObjectMapper`. However, most others can be non-public and I've also updated `UpdateReqDeserializer` / `UpdateReqSerializer` to non-public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216971175
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
Review Comment:
See my other comment. I think we should remove the builder and keep the public constructors to match `MetadataUpdate`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1217808493
##########
core/src/main/java/org/apache/iceberg/rest/requests/CommitTableRequest.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest.requests;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.iceberg.MetadataUpdate;
+import org.apache.iceberg.UpdateRequirement;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTRequest;
+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface CommitTableRequest extends RESTRequest {
Review Comment:
if we 're ok breaking source compability, then we can update `UpdateTableRequest` directly and don't need `CommitTableRequest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227348462
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -287,15 +287,15 @@ private static boolean isCreate(UpdateTableRequest request) {
boolean isCreate =
request.requirements().stream()
.anyMatch(
- UpdateTableRequest.UpdateRequirement.AssertTableDoesNotExist.class::isInstance);
+ org.apache.iceberg.UpdateRequirement.AssertTableDoesNotExist.class::isInstance);
Review Comment:
Can this not be imported?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227644801
##########
.palantir/revapi.yml:
##########
@@ -737,6 +737,11 @@ acceptedBreaks:
old: "method void org.apache.iceberg.rest.auth.OAuth2Util.AuthSession::<init>(java.util.Map<java.lang.String,\
\ java.lang.String>, java.lang.String, java.lang.String)"
justification: "Removing deprecations for 1.3.0"
+ - code: "java.method.returnTypeTypeParametersChanged"
+ 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"
Review Comment:
correct, this is not binary-breaking: `SOURCE: BREAKING, BINARY: NON_BREAKING`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216976503
##########
core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java:
##########
@@ -95,6 +117,12 @@ public Builder(TableMetadata base, boolean isReplace) {
this.isReplace = isReplace;
}
+ public Builder forTable(TableIdentifier ident) {
Review Comment:
I don't think we need this if we are replacing the `UpdateTableRequest` right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216970383
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null) {
+ throw new CommitFailedException("Requirement failed: table already exists");
+ }
+ }
+ }
+
+ class AssertTableUUID implements UpdateRequirement {
+ private final String uuid;
+
+ public AssertTableUUID(String uuid) {
+ this.uuid = uuid;
+ }
+
+ public String uuid() {
+ return uuid;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (!uuid.equalsIgnoreCase(base.uuid())) {
+ throw new CommitFailedException(
+ "Requirement failed: UUID does not match: expected %s != %s", base.uuid(), uuid);
+ }
+ }
+ }
+
+ class AssertRefSnapshotID implements UpdateRequirement {
+ private final String name;
+ private final Long snapshotId;
+
+ public AssertRefSnapshotID(String name, Long snapshotId) {
+ this.name = name;
+ this.snapshotId = snapshotId;
+ }
+
+ public String refName() {
+ return name;
+ }
+
+ public Long snapshotId() {
+ return snapshotId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ SnapshotRef ref = base.ref(name);
+ if (ref != null) {
+ String type = ref.isBranch() ? "branch" : "tag";
+ if (snapshotId == null) {
+ // a null snapshot ID means the ref should not exist already
+ throw new CommitFailedException(
+ "Requirement failed: %s %s was created concurrently", type, name);
+ } else if (snapshotId != ref.snapshotId()) {
+ throw new CommitFailedException(
+ "Requirement failed: %s %s has changed: expected id %s != %s",
+ type, name, snapshotId, ref.snapshotId());
+ }
+ } else if (snapshotId != null) {
+ throw new CommitFailedException(
+ "Requirement failed: branch or tag %s is missing, expected %s", name, snapshotId);
+ }
+ }
+ }
+
+ class AssertLastAssignedFieldId implements UpdateRequirement {
+ private final int lastAssignedFieldId;
+
+ public AssertLastAssignedFieldId(int lastAssignedFieldId) {
+ this.lastAssignedFieldId = lastAssignedFieldId;
+ }
+
+ public int lastAssignedFieldId() {
+ return lastAssignedFieldId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null && base.lastColumnId() != lastAssignedFieldId) {
+ throw new CommitFailedException(
+ "Requirement failed: last assigned field id changed: expected id %s != %s",
+ lastAssignedFieldId, base.lastColumnId());
+ }
+ }
+ }
+
+ class AssertCurrentSchemaID implements UpdateRequirement {
+ private final int schemaId;
+
+ public AssertCurrentSchemaID(int schemaId) {
+ this.schemaId = schemaId;
+ }
+
+ public int schemaId() {
+ return schemaId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (schemaId != base.currentSchemaId()) {
+ throw new CommitFailedException(
+ "Requirement failed: current schema changed: expected id %s != %s",
+ schemaId, base.currentSchemaId());
+ }
+ }
+ }
+
+ class AssertLastAssignedPartitionId implements UpdateRequirement {
+ private final int lastAssignedPartitionId;
+
+ public AssertLastAssignedPartitionId(int lastAssignedPartitionId) {
+ this.lastAssignedPartitionId = lastAssignedPartitionId;
+ }
+
+ public int lastAssignedPartitionId() {
+ return lastAssignedPartitionId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null && base.lastAssignedPartitionId() != lastAssignedPartitionId) {
+ throw new CommitFailedException(
+ "Requirement failed: last assigned partition id changed: expected id %s != %s",
+ lastAssignedPartitionId, base.lastAssignedPartitionId());
+ }
+ }
+ }
+
+ class AssertDefaultSpecID implements UpdateRequirement {
+ private final int specId;
+
+ public AssertDefaultSpecID(int specId) {
+ this.specId = specId;
+ }
+
+ public int specId() {
+ return specId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (specId != base.defaultSpecId()) {
+ throw new CommitFailedException(
+ "Requirement failed: default partition spec changed: expected id %s != %s",
+ specId, base.defaultSpecId());
+ }
+ }
+ }
+
+ class AssertDefaultSortOrderID implements UpdateRequirement {
+ private final int sortOrderId;
+
+ public AssertDefaultSortOrderID(int sortOrderId) {
+ this.sortOrderId = sortOrderId;
+ }
+
+ public int sortOrderId() {
+ return sortOrderId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (sortOrderId != base.defaultSortOrderId()) {
+ throw new CommitFailedException(
+ "Requirement failed: default sort order changed: expected id %s != %s",
+ sortOrderId, base.defaultSortOrderId());
+ }
+ }
+ }
+
+ class Builder {
+ private final TableMetadata base;
+ private final ImmutableList.Builder<UpdateRequirement> requirements = ImmutableList.builder();
+ private final List<MetadataUpdate> updates = Lists.newArrayList();
+ private final Set<String> changedRefs = Sets.newHashSet();
+ private final boolean isReplace;
+ private TableIdentifier identifier = null;
+ private boolean addedSchema = false;
+ private boolean setSchemaId = false;
+ private boolean addedSpec = false;
+ private boolean setSpecId = false;
+ private boolean setOrderId = false;
+
+ public Builder(TableMetadata base, boolean isReplace) {
+ this.base = base;
+ this.isReplace = isReplace;
+ }
+
+ public Builder forTable(TableIdentifier ident) {
+ Preconditions.checkArgument(null != ident, "Invalid table identifier: null");
+ this.identifier = ident;
+ return this;
+ }
+
+ private Builder require(UpdateRequirement requirement) {
+ Preconditions.checkArgument(requirement != null, "Invalid requirement: null");
+ requirements.add(requirement);
+ return this;
+ }
+
+ private Builder requireCreate() {
+ return require(new UpdateRequirement.AssertTableDoesNotExist());
+ }
+
+ private Builder requireTableUUID(String uuid) {
+ Preconditions.checkArgument(uuid != null, "Invalid required UUID: null");
+ return require(new UpdateRequirement.AssertTableUUID(uuid));
+ }
+
+ private Builder requireRefSnapshotId(String ref, Long snapshotId) {
+ return require(new UpdateRequirement.AssertRefSnapshotID(ref, snapshotId));
+ }
+
+ private Builder requireLastAssignedFieldId(int fieldId) {
+ return require(new UpdateRequirement.AssertLastAssignedFieldId(fieldId));
+ }
+
+ private Builder requireCurrentSchemaId(int schemaId) {
+ return require(new UpdateRequirement.AssertCurrentSchemaID(schemaId));
+ }
+
+ private Builder requireLastAssignedPartitionId(int partitionId) {
+ return require(new UpdateRequirement.AssertLastAssignedPartitionId(partitionId));
+ }
+
+ private Builder requireDefaultSpecId(int specId) {
+ return require(new UpdateRequirement.AssertDefaultSpecID(specId));
+ }
+
+ private Builder requireDefaultSortOrderId(int orderId) {
+ return require(new UpdateRequirement.AssertDefaultSortOrderID(orderId));
+ }
+
+ public Builder update(MetadataUpdate update) {
+ Preconditions.checkArgument(update != null, "Invalid update: null");
+ updates.add(update);
+
+ // add requirements based on the change
+ if (update instanceof MetadataUpdate.SetSnapshotRef) {
+ update((MetadataUpdate.SetSnapshotRef) update);
+ } else if (update instanceof MetadataUpdate.AddSchema) {
+ update((MetadataUpdate.AddSchema) update);
+ } else if (update instanceof MetadataUpdate.SetCurrentSchema) {
+ update((MetadataUpdate.SetCurrentSchema) update);
+ } else if (update instanceof MetadataUpdate.AddPartitionSpec) {
+ update((MetadataUpdate.AddPartitionSpec) update);
+ } else if (update instanceof MetadataUpdate.SetDefaultPartitionSpec) {
+ update((MetadataUpdate.SetDefaultPartitionSpec) update);
+ } else if (update instanceof MetadataUpdate.SetDefaultSortOrder) {
+ update((MetadataUpdate.SetDefaultSortOrder) update);
+ }
+
+ return this;
+ }
+
+ private void update(MetadataUpdate.SetSnapshotRef setRef) {
+ // require that the ref is unchanged from the base
+ String name = setRef.name();
+ // add returns true the first time the ref name is added
+ boolean added = changedRefs.add(name);
+ if (added && base != null && !isReplace) {
+ SnapshotRef baseRef = base.ref(name);
+ // require that the ref does not exist (null) or is the same as the base snapshot
+ requireRefSnapshotId(name, baseRef != null ? baseRef.snapshotId() : null);
+ }
+ }
+
+ private void update(MetadataUpdate.AddSchema update) {
+ if (!addedSchema) {
+ if (base != null) {
+ requireLastAssignedFieldId(base.lastColumnId());
+ }
+ this.addedSchema = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetCurrentSchema update) {
+ if (!setSchemaId) {
+ if (base != null && !isReplace) {
+ // require that the current schema has not changed
+ requireCurrentSchemaId(base.currentSchemaId());
+ }
+ this.setSchemaId = true;
+ }
+ }
+
+ private void update(MetadataUpdate.AddPartitionSpec update) {
+ if (!addedSpec) {
+ if (base != null) {
+ requireLastAssignedPartitionId(base.lastAssignedPartitionId());
+ }
+ this.addedSpec = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetDefaultPartitionSpec update) {
+ if (!setSpecId) {
+ if (base != null && !isReplace) {
+ // require that the default spec has not changed
+ requireDefaultSpecId(base.defaultSpecId());
+ }
+ this.setSpecId = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetDefaultSortOrder update) {
+ if (!setOrderId) {
+ if (base != null && !isReplace) {
+ // require that the default write order has not changed
+ requireDefaultSortOrderId(base.defaultSortOrderId());
+ }
+ this.setOrderId = true;
+ }
+ }
+
+ public List<UpdateRequirement> build() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1229106626
##########
core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java:
##########
@@ -0,0 +1,536 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import java.util.UUID;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.assertj.core.api.InstanceOfAssertFactories;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestUpdateRequirements {
+ private final TableMetadata metadata = mock(TableMetadata.class);
+ private final TableMetadata updated = mock(TableMetadata.class);
+
+ @BeforeEach
+ public void before() {
+ String uuid = UUID.randomUUID().toString();
+ when(metadata.uuid()).thenReturn(uuid);
+ when(updated.uuid()).thenReturn(uuid);
+ }
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> UpdateRequirements.forCreateTable(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+ }
+
+ @Test
+ public void emptyUpdatesForCreateTable() {
+ assertThat(UpdateRequirements.forCreateTable(ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableDoesNotExist.class);
+ }
+
+ @Test
+ public void emptyUpdatesForUpdateAndReplaceTable() {
+ assertThat(UpdateRequirements.forReplaceTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertThat(UpdateRequirements.forUpdateTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void tableAlreadyExists() {
+ List<UpdateRequirement> requirements = UpdateRequirements.forCreateTable(ImmutableList.of());
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(metadata)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: table already exists");
+ }
+
+ @Test
+ public void assignUUID() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid())));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertTableUUID(requirements);
+ }
+
+ @Test
+ public void assignUUIDFailure() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid())));
+
+ when(updated.uuid()).thenReturn(UUID.randomUUID().toString());
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage(
+ String.format(
+ "Requirement failed: UUID does not match: expected %s != %s",
+ updated.uuid(), metadata.uuid()));
+ }
+
+ @Test
+ public void upgradeFormatVersion() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.UpgradeFormatVersion(2)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertTableUUID(requirements);
+ }
+
+ @Test
+ public void addSchema() {
+ int lastColumnId = 1;
+ when(metadata.lastColumnId()).thenReturn(lastColumnId);
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AddSchema(new Schema(), lastColumnId)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class,
+ UpdateRequirement.AssertLastAssignedFieldId.class);
+
+ assertTableUUID(requirements);
+
+ assertThat(requirements)
+ .element(1)
+ .asInstanceOf(
+ InstanceOfAssertFactories.type(UpdateRequirement.AssertLastAssignedFieldId.class))
+ .extracting(UpdateRequirement.AssertLastAssignedFieldId::lastAssignedFieldId)
+ .isEqualTo(lastColumnId);
+ }
+
+ @Test
+ public void addSchemaFailure() {
+ when(metadata.lastColumnId()).thenReturn(2);
+ when(updated.lastColumnId()).thenReturn(3);
+
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata,
+ ImmutableList.of(
+ new MetadataUpdate.AddSchema(new Schema(), 1),
+ new MetadataUpdate.AddSchema(new Schema(), 2),
+ new MetadataUpdate.AddSchema(new Schema(), 3)));
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: last assigned field id changed: expected id 2 != 3");
+ }
+
+ @Test
+ public void setCurrentSchema() {
+ int schemaId = 3;
+ when(metadata.currentSchemaId()).thenReturn(schemaId);
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.SetCurrentSchema(schemaId)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class, UpdateRequirement.AssertCurrentSchemaID.class);
+
+ assertTableUUID(requirements);
+
+ assertThat(requirements)
+ .element(1)
+ .asInstanceOf(InstanceOfAssertFactories.type(UpdateRequirement.AssertCurrentSchemaID.class))
+ .extracting(UpdateRequirement.AssertCurrentSchemaID::schemaId)
+ .isEqualTo(schemaId);
+ }
+
+ @Test
+ public void setCurrentSchemaFailure() {
+ int schemaId = 3;
+ when(metadata.currentSchemaId()).thenReturn(schemaId);
+ when(updated.currentSchemaId()).thenReturn(schemaId + 1);
+
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata,
+ ImmutableList.of(
+ new MetadataUpdate.SetCurrentSchema(schemaId),
+ new MetadataUpdate.SetCurrentSchema(schemaId + 1)));
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: current schema changed: expected id 3 != 4");
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1226897415
##########
.palantir/revapi.yml:
##########
@@ -737,6 +737,11 @@ acceptedBreaks:
old: "method void org.apache.iceberg.rest.auth.OAuth2Util.AuthSession::<init>(java.util.Map<java.lang.String,\
\ java.lang.String>, java.lang.String, java.lang.String)"
justification: "Removing deprecations for 1.3.0"
+ - code: "java.method.returnTypeTypeParametersChanged"
+ 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"
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216976258
##########
core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java:
##########
@@ -78,12 +94,18 @@ public static Builder builderFor(TableMetadata base) {
return new Builder(base, false).requireTableUUID(base.uuid());
}
+ /**
+ * @deprecated will be removed in 1.4.0, use {@link org.apache.iceberg.UpdateRequirement.Builder}
+ * instead.
+ */
+ @Deprecated
Review Comment:
I think the `builderFor` methods should also be deprecated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227355854
##########
core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.UUID;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestUpdateRequirements {
+ private final TableMetadata metadata = mock(TableMetadata.class);
+
+ @BeforeEach
+ public void before() {
+ String uuid = UUID.randomUUID().toString();
+ when(metadata.uuid()).thenReturn(uuid);
+ }
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> UpdateRequirements.forCreateTable(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+ }
+
+ @Test
+ public void emptyUpdatesForCreateTable() {
+ assertThat(UpdateRequirements.forCreateTable(ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableDoesNotExist.class);
+ }
+
+ @Test
+ public void emptyUpdatesForUpdateAndReplaceTable() {
+ assertThat(UpdateRequirements.forReplaceTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertThat(UpdateRequirements.forUpdateTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void assignUUID() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid()))))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void upgradeFormatVersion() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.UpgradeFormatVersion(2))))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void addSchema() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AddSchema(new Schema(), 0))))
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class,
+ UpdateRequirement.AssertLastAssignedFieldId.class);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1226424376
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null) {
+ throw new CommitFailedException("Requirement failed: table already exists");
+ }
+ }
+ }
+
+ class AssertTableUUID implements UpdateRequirement {
+ private final String uuid;
+
+ public AssertTableUUID(String uuid) {
+ this.uuid = uuid;
+ }
+
+ public String uuid() {
+ return uuid;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (!uuid.equalsIgnoreCase(base.uuid())) {
+ throw new CommitFailedException(
+ "Requirement failed: UUID does not match: expected %s != %s", base.uuid(), uuid);
+ }
+ }
+ }
+
+ class AssertRefSnapshotID implements UpdateRequirement {
+ private final String name;
+ private final Long snapshotId;
+
+ public AssertRefSnapshotID(String name, Long snapshotId) {
+ this.name = name;
+ this.snapshotId = snapshotId;
+ }
+
+ public String refName() {
+ return name;
+ }
+
+ public Long snapshotId() {
+ return snapshotId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ SnapshotRef ref = base.ref(name);
+ if (ref != null) {
+ String type = ref.isBranch() ? "branch" : "tag";
+ if (snapshotId == null) {
+ // a null snapshot ID means the ref should not exist already
+ throw new CommitFailedException(
+ "Requirement failed: %s %s was created concurrently", type, name);
+ } else if (snapshotId != ref.snapshotId()) {
+ throw new CommitFailedException(
+ "Requirement failed: %s %s has changed: expected id %s != %s",
+ type, name, snapshotId, ref.snapshotId());
+ }
+ } else if (snapshotId != null) {
+ throw new CommitFailedException(
+ "Requirement failed: branch or tag %s is missing, expected %s", name, snapshotId);
+ }
+ }
+ }
+
+ class AssertLastAssignedFieldId implements UpdateRequirement {
+ private final int lastAssignedFieldId;
+
+ public AssertLastAssignedFieldId(int lastAssignedFieldId) {
+ this.lastAssignedFieldId = lastAssignedFieldId;
+ }
+
+ public int lastAssignedFieldId() {
+ return lastAssignedFieldId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null && base.lastColumnId() != lastAssignedFieldId) {
+ throw new CommitFailedException(
+ "Requirement failed: last assigned field id changed: expected id %s != %s",
+ lastAssignedFieldId, base.lastColumnId());
+ }
+ }
+ }
+
+ class AssertCurrentSchemaID implements UpdateRequirement {
+ private final int schemaId;
+
+ public AssertCurrentSchemaID(int schemaId) {
+ this.schemaId = schemaId;
+ }
+
+ public int schemaId() {
+ return schemaId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (schemaId != base.currentSchemaId()) {
+ throw new CommitFailedException(
+ "Requirement failed: current schema changed: expected id %s != %s",
+ schemaId, base.currentSchemaId());
+ }
+ }
+ }
+
+ class AssertLastAssignedPartitionId implements UpdateRequirement {
+ private final int lastAssignedPartitionId;
+
+ public AssertLastAssignedPartitionId(int lastAssignedPartitionId) {
+ this.lastAssignedPartitionId = lastAssignedPartitionId;
+ }
+
+ public int lastAssignedPartitionId() {
+ return lastAssignedPartitionId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (base != null && base.lastAssignedPartitionId() != lastAssignedPartitionId) {
+ throw new CommitFailedException(
+ "Requirement failed: last assigned partition id changed: expected id %s != %s",
+ lastAssignedPartitionId, base.lastAssignedPartitionId());
+ }
+ }
+ }
+
+ class AssertDefaultSpecID implements UpdateRequirement {
+ private final int specId;
+
+ public AssertDefaultSpecID(int specId) {
+ this.specId = specId;
+ }
+
+ public int specId() {
+ return specId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (specId != base.defaultSpecId()) {
+ throw new CommitFailedException(
+ "Requirement failed: default partition spec changed: expected id %s != %s",
+ specId, base.defaultSpecId());
+ }
+ }
+ }
+
+ class AssertDefaultSortOrderID implements UpdateRequirement {
+ private final int sortOrderId;
+
+ public AssertDefaultSortOrderID(int sortOrderId) {
+ this.sortOrderId = sortOrderId;
+ }
+
+ public int sortOrderId() {
+ return sortOrderId;
+ }
+
+ @Override
+ public void validate(TableMetadata base) {
+ if (sortOrderId != base.defaultSortOrderId()) {
+ throw new CommitFailedException(
+ "Requirement failed: default sort order changed: expected id %s != %s",
+ sortOrderId, base.defaultSortOrderId());
+ }
+ }
+ }
+
+ class Builder {
+ private final TableMetadata base;
+ private final ImmutableList.Builder<UpdateRequirement> requirements = ImmutableList.builder();
+ private final List<MetadataUpdate> updates = Lists.newArrayList();
+ private final Set<String> changedRefs = Sets.newHashSet();
+ private final boolean isReplace;
+ private TableIdentifier identifier = null;
+ private boolean addedSchema = false;
+ private boolean setSchemaId = false;
+ private boolean addedSpec = false;
+ private boolean setSpecId = false;
+ private boolean setOrderId = false;
+
+ public Builder(TableMetadata base, boolean isReplace) {
+ this.base = base;
+ this.isReplace = isReplace;
+ }
+
+ public Builder forTable(TableIdentifier ident) {
+ Preconditions.checkArgument(null != ident, "Invalid table identifier: null");
+ this.identifier = ident;
+ return this;
+ }
+
+ private Builder require(UpdateRequirement requirement) {
+ Preconditions.checkArgument(requirement != null, "Invalid requirement: null");
+ requirements.add(requirement);
+ return this;
+ }
+
+ private Builder requireCreate() {
+ return require(new UpdateRequirement.AssertTableDoesNotExist());
+ }
+
+ private Builder requireTableUUID(String uuid) {
+ Preconditions.checkArgument(uuid != null, "Invalid required UUID: null");
+ return require(new UpdateRequirement.AssertTableUUID(uuid));
+ }
+
+ private Builder requireRefSnapshotId(String ref, Long snapshotId) {
+ return require(new UpdateRequirement.AssertRefSnapshotID(ref, snapshotId));
+ }
+
+ private Builder requireLastAssignedFieldId(int fieldId) {
+ return require(new UpdateRequirement.AssertLastAssignedFieldId(fieldId));
+ }
+
+ private Builder requireCurrentSchemaId(int schemaId) {
+ return require(new UpdateRequirement.AssertCurrentSchemaID(schemaId));
+ }
+
+ private Builder requireLastAssignedPartitionId(int partitionId) {
+ return require(new UpdateRequirement.AssertLastAssignedPartitionId(partitionId));
+ }
+
+ private Builder requireDefaultSpecId(int specId) {
+ return require(new UpdateRequirement.AssertDefaultSpecID(specId));
+ }
+
+ private Builder requireDefaultSortOrderId(int orderId) {
+ return require(new UpdateRequirement.AssertDefaultSortOrderID(orderId));
+ }
+
+ public Builder update(MetadataUpdate update) {
+ Preconditions.checkArgument(update != null, "Invalid update: null");
+ updates.add(update);
+
+ // add requirements based on the change
+ if (update instanceof MetadataUpdate.SetSnapshotRef) {
+ update((MetadataUpdate.SetSnapshotRef) update);
+ } else if (update instanceof MetadataUpdate.AddSchema) {
+ update((MetadataUpdate.AddSchema) update);
+ } else if (update instanceof MetadataUpdate.SetCurrentSchema) {
+ update((MetadataUpdate.SetCurrentSchema) update);
+ } else if (update instanceof MetadataUpdate.AddPartitionSpec) {
+ update((MetadataUpdate.AddPartitionSpec) update);
+ } else if (update instanceof MetadataUpdate.SetDefaultPartitionSpec) {
+ update((MetadataUpdate.SetDefaultPartitionSpec) update);
+ } else if (update instanceof MetadataUpdate.SetDefaultSortOrder) {
+ update((MetadataUpdate.SetDefaultSortOrder) update);
+ }
+
+ return this;
+ }
+
+ private void update(MetadataUpdate.SetSnapshotRef setRef) {
+ // require that the ref is unchanged from the base
+ String name = setRef.name();
+ // add returns true the first time the ref name is added
+ boolean added = changedRefs.add(name);
+ if (added && base != null && !isReplace) {
+ SnapshotRef baseRef = base.ref(name);
+ // require that the ref does not exist (null) or is the same as the base snapshot
+ requireRefSnapshotId(name, baseRef != null ? baseRef.snapshotId() : null);
+ }
+ }
+
+ private void update(MetadataUpdate.AddSchema update) {
+ if (!addedSchema) {
+ if (base != null) {
+ requireLastAssignedFieldId(base.lastColumnId());
+ }
+ this.addedSchema = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetCurrentSchema update) {
+ if (!setSchemaId) {
+ if (base != null && !isReplace) {
+ // require that the current schema has not changed
+ requireCurrentSchemaId(base.currentSchemaId());
+ }
+ this.setSchemaId = true;
+ }
+ }
+
+ private void update(MetadataUpdate.AddPartitionSpec update) {
+ if (!addedSpec) {
+ if (base != null) {
+ requireLastAssignedPartitionId(base.lastAssignedPartitionId());
+ }
+ this.addedSpec = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetDefaultPartitionSpec update) {
+ if (!setSpecId) {
+ if (base != null && !isReplace) {
+ // require that the default spec has not changed
+ requireDefaultSpecId(base.defaultSpecId());
+ }
+ this.setSpecId = true;
+ }
+ }
+
+ private void update(MetadataUpdate.SetDefaultSortOrder update) {
+ if (!setOrderId) {
+ if (base != null && !isReplace) {
+ // require that the default write order has not changed
+ requireDefaultSortOrderId(base.defaultSortOrderId());
+ }
+ this.setOrderId = true;
+ }
+ }
+
+ public List<UpdateRequirement> build() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227655313
##########
core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java:
##########
@@ -110,6 +118,28 @@ public void serialize(
}
}
+ public static class UpdateReqDeserializer
Review Comment:
good call, yes I think it makes sense to make these non-public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216955586
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
+ void validate(TableMetadata base);
+
+ static Builder builderForCreate() {
+ return new Builder(null, false).requireCreate();
+ }
+
+ static Builder builderForReplace(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, true).requireTableUUID(base.uuid());
+ }
+
+ static Builder builderFor(TableMetadata base) {
+ Preconditions.checkNotNull(base, "Cannot create a builder from table metadata: null");
+ return new Builder(base, false).requireTableUUID(base.uuid());
+ }
+
+ class AssertTableDoesNotExist implements UpdateRequirement {
+ public AssertTableDoesNotExist() {}
Review Comment:
Why is this public? It wasn't public in the original version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216972789
##########
core/src/main/java/org/apache/iceberg/UpdateRequirementParser.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.JsonUtil;
+
+public class UpdateRequirementParser {
Review Comment:
The only change to this was copying it to the new package, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1217661147
##########
core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java:
##########
@@ -45,6 +49,14 @@ public UpdateTableRequest(List<UpdateRequirement> requirements, List<MetadataUpd
this.updates = updates;
}
+ public UpdateTableRequest(
Review Comment:
good point, yep this can be non-public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227349300
##########
core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java:
##########
@@ -110,6 +118,28 @@ public void serialize(
}
}
+ public static class UpdateReqDeserializer
Review Comment:
Maybe these classes should not be `public` so that we can change them? I think the only way `RESTSerializers` is currently used is through the `registerAll` method and that shouldn't require these to be public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1217711077
##########
core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java:
##########
@@ -95,6 +117,12 @@ public Builder(TableMetadata base, boolean isReplace) {
this.isReplace = isReplace;
}
+ public Builder forTable(TableIdentifier ident) {
Review Comment:
yep, I removed this already in #7741
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227351962
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -133,9 +143,7 @@ public void commit(TableMetadata base, TableMetadata metadata) {
String.format("Update type %s is not supported", updateType));
}
- baseChanges.forEach(requestBuilder::update);
- metadata.changes().forEach(requestBuilder::update);
- UpdateTableRequest request = requestBuilder.build();
+ UpdateTableRequest request = new UpdateTableRequest(requirements, updates);
Review Comment:
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 the builder later, but I can see why this needs to be public and used at the moment.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -133,9 +143,7 @@ public void commit(TableMetadata base, TableMetadata metadata) {
String.format("Update type %s is not supported", updateType));
}
- baseChanges.forEach(requestBuilder::update);
- metadata.changes().forEach(requestBuilder::update);
- UpdateTableRequest request = requestBuilder.build();
+ UpdateTableRequest request = new UpdateTableRequest(requirements, updates);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1227657498
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -133,9 +143,7 @@ public void commit(TableMetadata base, TableMetadata metadata) {
String.format("Update type %s is not supported", updateType));
}
- baseChanges.forEach(requestBuilder::update);
- metadata.changes().forEach(requestBuilder::update);
- UpdateTableRequest request = requestBuilder.build();
+ UpdateTableRequest request = new UpdateTableRequest(requirements, updates);
Review Comment:
yes exactly, the builder changed substantially. In the long run I think we can add a builder again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1228118701
##########
core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.UUID;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestUpdateRequirements {
+ private final TableMetadata metadata = mock(TableMetadata.class);
+
+ @BeforeEach
+ public void before() {
+ String uuid = UUID.randomUUID().toString();
+ when(metadata.uuid()).thenReturn(uuid);
+ }
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> UpdateRequirements.forCreateTable(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+ }
+
+ @Test
+ public void emptyUpdatesForCreateTable() {
+ assertThat(UpdateRequirements.forCreateTable(ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableDoesNotExist.class);
+ }
+
+ @Test
+ public void emptyUpdatesForUpdateAndReplaceTable() {
+ assertThat(UpdateRequirements.forReplaceTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertThat(UpdateRequirements.forUpdateTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void assignUUID() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid()))))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void upgradeFormatVersion() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.UpgradeFormatVersion(2))))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void addSchema() {
+ assertThat(
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AddSchema(new Schema(), 0))))
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class,
+ UpdateRequirement.AssertLastAssignedFieldId.class);
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1228322186
##########
core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java:
##########
@@ -0,0 +1,536 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import java.util.UUID;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.assertj.core.api.InstanceOfAssertFactories;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestUpdateRequirements {
+ private final TableMetadata metadata = mock(TableMetadata.class);
+ private final TableMetadata updated = mock(TableMetadata.class);
+
+ @BeforeEach
+ public void before() {
+ String uuid = UUID.randomUUID().toString();
+ when(metadata.uuid()).thenReturn(uuid);
+ when(updated.uuid()).thenReturn(uuid);
+ }
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> UpdateRequirements.forCreateTable(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forUpdateTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(null, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table metadata: null");
+
+ assertThatThrownBy(() -> UpdateRequirements.forReplaceTable(metadata, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid metadata updates: null");
+ }
+
+ @Test
+ public void emptyUpdatesForCreateTable() {
+ assertThat(UpdateRequirements.forCreateTable(ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableDoesNotExist.class);
+ }
+
+ @Test
+ public void emptyUpdatesForUpdateAndReplaceTable() {
+ assertThat(UpdateRequirements.forReplaceTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertThat(UpdateRequirements.forUpdateTable(metadata, ImmutableList.of()))
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+ }
+
+ @Test
+ public void tableAlreadyExists() {
+ List<UpdateRequirement> requirements = UpdateRequirements.forCreateTable(ImmutableList.of());
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(metadata)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: table already exists");
+ }
+
+ @Test
+ public void assignUUID() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid())));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertTableUUID(requirements);
+ }
+
+ @Test
+ public void assignUUIDFailure() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AssignUUID(metadata.uuid())));
+
+ when(updated.uuid()).thenReturn(UUID.randomUUID().toString());
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage(
+ String.format(
+ "Requirement failed: UUID does not match: expected %s != %s",
+ updated.uuid(), metadata.uuid()));
+ }
+
+ @Test
+ public void upgradeFormatVersion() {
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.UpgradeFormatVersion(2)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(1)
+ .hasOnlyElementsOfType(UpdateRequirement.AssertTableUUID.class);
+
+ assertTableUUID(requirements);
+ }
+
+ @Test
+ public void addSchema() {
+ int lastColumnId = 1;
+ when(metadata.lastColumnId()).thenReturn(lastColumnId);
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.AddSchema(new Schema(), lastColumnId)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class,
+ UpdateRequirement.AssertLastAssignedFieldId.class);
+
+ assertTableUUID(requirements);
+
+ assertThat(requirements)
+ .element(1)
+ .asInstanceOf(
+ InstanceOfAssertFactories.type(UpdateRequirement.AssertLastAssignedFieldId.class))
+ .extracting(UpdateRequirement.AssertLastAssignedFieldId::lastAssignedFieldId)
+ .isEqualTo(lastColumnId);
+ }
+
+ @Test
+ public void addSchemaFailure() {
+ when(metadata.lastColumnId()).thenReturn(2);
+ when(updated.lastColumnId()).thenReturn(3);
+
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata,
+ ImmutableList.of(
+ new MetadataUpdate.AddSchema(new Schema(), 1),
+ new MetadataUpdate.AddSchema(new Schema(), 2),
+ new MetadataUpdate.AddSchema(new Schema(), 3)));
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: last assigned field id changed: expected id 2 != 3");
+ }
+
+ @Test
+ public void setCurrentSchema() {
+ int schemaId = 3;
+ when(metadata.currentSchemaId()).thenReturn(schemaId);
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata, ImmutableList.of(new MetadataUpdate.SetCurrentSchema(schemaId)));
+ requirements.forEach(req -> req.validate(metadata));
+
+ assertThat(requirements)
+ .hasSize(2)
+ .hasOnlyElementsOfTypes(
+ UpdateRequirement.AssertTableUUID.class, UpdateRequirement.AssertCurrentSchemaID.class);
+
+ assertTableUUID(requirements);
+
+ assertThat(requirements)
+ .element(1)
+ .asInstanceOf(InstanceOfAssertFactories.type(UpdateRequirement.AssertCurrentSchemaID.class))
+ .extracting(UpdateRequirement.AssertCurrentSchemaID::schemaId)
+ .isEqualTo(schemaId);
+ }
+
+ @Test
+ public void setCurrentSchemaFailure() {
+ int schemaId = 3;
+ when(metadata.currentSchemaId()).thenReturn(schemaId);
+ when(updated.currentSchemaId()).thenReturn(schemaId + 1);
+
+ List<UpdateRequirement> requirements =
+ UpdateRequirements.forUpdateTable(
+ metadata,
+ ImmutableList.of(
+ new MetadataUpdate.SetCurrentSchema(schemaId),
+ new MetadataUpdate.SetCurrentSchema(schemaId + 1)));
+
+ assertThatThrownBy(() -> requirements.forEach(req -> req.validate(updated)))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Requirement failed: current schema changed: expected id 3 != 4");
Review Comment:
This test gets coverage for multiple `SetCurrentSchema` changes, but it doesn't validate that the requirements are what we expect. I think this should add another `SetCurrentSchema` to the `setCurrentSchema` test above because that shouldn't change the behavior. Other tests that only generate a single requirement should also be tested similarly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216975128
##########
core/src/main/java/org/apache/iceberg/UpdateRequirement.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+/** Represents a requirement for a {@link MetadataUpdate} */
+public interface UpdateRequirement {
Review Comment:
These changes can be in a separate PR, right? I don't see a need to include them in changes to the commit request, since the original `UpdateTableRequest` is going to remain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216993219
##########
core/src/main/java/org/apache/iceberg/rest/requests/CommitTableRequest.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest.requests;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.iceberg.MetadataUpdate;
+import org.apache.iceberg.UpdateRequirement;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTRequest;
+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface CommitTableRequest extends RESTRequest {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1216975587
##########
core/src/main/java/org/apache/iceberg/rest/requests/UpdateTableRequest.java:
##########
@@ -45,6 +49,14 @@ public UpdateTableRequest(List<UpdateRequirement> requirements, List<MetadataUpd
this.updates = updates;
}
+ public UpdateTableRequest(
Review Comment:
This doesn't need to be public right? I think it was a mistake that the other constructor was public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7750: Core: Move UpdateRequirement out of rest package
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7750:
URL: https://github.com/apache/iceberg/pull/7750#discussion_r1217660286
##########
core/src/main/java/org/apache/iceberg/UpdateRequirementParser.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.JsonUtil;
+
+public class UpdateRequirementParser {
Review Comment:
yes exactly, it was a pure copy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org