jun-he commented on a change in pull request #922:
URL: https://github.com/apache/iceberg/pull/922#discussion_r453247380
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitionSpecParser.java
##########
@@ -19,16 +19,20 @@
package org.apache.iceberg;
+import org.apache.iceberg.types.Types;
import org.junit.Assert;
import org.junit.Test;
-public class TestPartitionSpecParser extends TableTestBase {
- public TestPartitionSpecParser() {
- super(1);
- }
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class TestPartitionSpecParser {
+ private static final Schema SCHEMA = new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get())
+ );
@Test
- public void testToJsonForV1Table() {
+ public void testToJson() {
Review comment:
Thanks for the comment. This test was updated because `TableTestBase`
was removed. There is no new test cases added into it..
I updated the test name because, with the current implementation, this test
is not specific for `V1 Table` and there is no special `toJson` handling needed
for V2 table.
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecUpdate.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.apache.iceberg.exceptions.ValidationException;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+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.Maps;
+import org.apache.iceberg.transforms.Transforms;
+
+/**
+ * PartitionSpec evolution API implementation.
+ */
+class PartitionSpecUpdate implements UpdatePartitionSpec {
+
+ private static final String SOFT_DELETE_POSTFIX = "__[removed]";
+
+ private final TableMetadata base;
+ private final TableOperations ops;
+ private final List<PartitionSpec> specs;
+ private final Schema schema;
+ private final Map<String, PartitionField> curSpecFields;
+ private final List<Consumer<PartitionSpec.Builder>> newSpecFields =
Lists.newArrayList();
+ private final Map<String, PartitionField> newRemovedFields =
Maps.newHashMap();
Review comment:
@rdblue Thanks for the comments. I think actually we can remove
`newRemovedFields `, `checkIfRemoved `, and `getLastPartitionField ` as there
is no need to check if a newly added field is just removed.
As we discussed in
https://github.com/apache/iceberg/pull/922#discussion_r441153291, the main
concern for removing a field and then adding it back is to pollute the metadata.
In the current implementation, it won't add duplications and then equivalent
to do nothing or rename the field because the partition field ID reuse.
I added a few unit tests for those cases.
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -429,11 +429,16 @@ public TableMetadata updatePartitionSpec(PartitionSpec
newPartitionSpec) {
}
}
- Preconditions.checkArgument(defaultSpecId != newDefaultSpecId,
- "Cannot set default partition spec to the current default");
+ // Always setting default partition spec to the new partition spec
+ ImmutableList.Builder<PartitionSpec> builder = ImmutableList.builder();
+ for (PartitionSpec spec : specs) {
+ if (spec.specId() == newDefaultSpecId) {
+ builder.add(freshSpec(newDefaultSpecId, schema, newPartitionSpec));
+ } else {
+ builder.add(spec);
+ }
+ }
Review comment:
@rdblue Thanks for the comment. It is a good idea to support noop commit.
The main reason to remove the precondition check here is to support rename a
partition field. In renaming case, the partition spec is compatible but it is
different and need to be committed.
Updated the code to support no-op commit and added unit tests.
##########
File path: api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java
##########
@@ -241,11 +263,45 @@ public void
testAddPartitionFieldsWithAndWithoutFieldIds() {
.add(1, "id_partition2", "bucket[5]")
.add(1, 1005, "id_partition1", "bucket[4]")
.truncate("s", 1, "custom_truncate")
+ .add(1, 1002, "id_partition3", "bucket[3]")
.build();
Assert.assertEquals(1000, spec.fields().get(0).fieldId());
Assert.assertEquals(1005, spec.fields().get(1).fieldId());
Assert.assertEquals(1006, spec.fields().get(2).fieldId());
+ Assert.assertEquals(1002, spec.fields().get(3).fieldId());
Assert.assertEquals(1006, spec.lastAssignedFieldId());
}
+
+ @Test
+ public void testAddPartitionFieldsWithInvalidFieldId() {
+ AssertHelpers.assertThrows("Should detect invalid duplicate field id",
+ IllegalArgumentException.class,
+ "Cannot add a partition that duplicates another within",
+ () -> PartitionSpec.builderFor(SCHEMA)
+ .add(1, "id_partition2", "bucket[5]")
+ .add(1, 1005, "id_partition1", "bucket[4]")
+ .add(1, 1005, "id_partition3", "bucket[3]")
+ .build());
+ }
+
+ @Test
+ public void testMultipleBucketPartitions() {
+ AssertHelpers.assertThrows("Should not allow bucket[8](id) and
bucket[16](id)",
+ IllegalArgumentException.class, "Cannot use partition name more than
once",
+ () -> PartitionSpec.builderFor(SCHEMA)
+ .bucket("id", 8, "bucket")
+ .bucket("s", 16, "bucket").build());
+
+ AssertHelpers.assertThrows("Should not allow bucket[8](id) and
bucket[16](id)",
+ IllegalArgumentException.class, "Cannot add redundant partition",
+ () -> PartitionSpec.builderFor(SCHEMA).bucket("id", 8).bucket("id",
16).build());
+
+ AssertHelpers.assertThrows("Should not allow bucket[8](id) and
bucket[16](id)",
+ IllegalArgumentException.class, "Cannot add redundant partition",
+ () -> PartitionSpec.builderFor(SCHEMA)
+ .bucket("id", 8, "id_bucket1")
+ .bucket("id", 16, "id_bucket2").build());
+
Review comment:
Done
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecUpdate.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.apache.iceberg.exceptions.ValidationException;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+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.Maps;
+import org.apache.iceberg.transforms.Transforms;
+
+/**
+ * PartitionSpec evolution API implementation.
+ */
+class PartitionSpecUpdate implements UpdatePartitionSpec {
+
+ private static final String SOFT_DELETE_POSTFIX = "__[removed]";
+
+ private final TableMetadata base;
+ private final TableOperations ops;
+ private final List<PartitionSpec> specs;
+ private final Schema schema;
+ private final Map<String, PartitionField> curSpecFields;
+ private final List<Consumer<PartitionSpec.Builder>> newSpecFields =
Lists.newArrayList();
+ private final Map<String, PartitionField> newRemovedFields =
Maps.newHashMap();
Review comment:
I also updated the code to use the correct ID from the start to avoid
`freshSpecFieldIds` at the end.
##########
File path: api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java
##########
@@ -38,6 +38,9 @@
public void testMultipleTimestampPartitions() {
AssertHelpers.assertThrows("Should not allow year(ts) and year(ts)",
IllegalArgumentException.class, "Cannot use partition name more than
once",
+ () -> PartitionSpec.builderFor(SCHEMA).year("ts",
"year").year("another_ts", "year").build());
Review comment:
Thanks for the explanation! Updated accordingly.
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -346,11 +346,34 @@ private void checkAndAddPartitionName(String name,
Integer identitySourceColumnI
partitionNames.add(name);
}
+ private String getDedupKey(PartitionField field) {
+ String transform = field.transform().toString();
+ String type;
+ if ("hour".equalsIgnoreCase(transform) ||
"day".equalsIgnoreCase(transform) ||
+ "month".equalsIgnoreCase(transform) ||
"year".equalsIgnoreCase(transform)) {
+ type = "time";
+ } else if (transform.startsWith("bucket[")) {
+ type = "bucket";
+ } else {
+ return null;
Review comment:
Yep, that is better. I add a `getName()` method to Transform interface.
Similar to `bucket`, we may only allow one `truncate` transform for a given
field.
Do you know if there is a valid use case with multiple `truncate` for a
field?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
