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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
