From a5fa184365314b27123612ff5375a30763da0316 Mon Sep 17 00:00:00 2001 From: zhangbutao Date: Thu, 22 Dec 2022 15:08:20 +0800 Subject: [PATCH 1/2] API: Fix inconsistent TimeTransform Type --- .../org/apache/iceberg/PartitionSpec.java | 8 ++-- .../apache/iceberg/UnboundPartitionSpec.java | 8 +--- .../org/apache/iceberg/TableTestBase.java | 3 +- .../iceberg/TestTableUpdatePartitionSpec.java | 48 +++++++++++++++++++ 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/PartitionSpec.java b/api/src/main/java/org/apache/iceberg/PartitionSpec.java index a31cfd76583b..0c76b0c32158 100644 --- a/api/src/main/java/org/apache/iceberg/PartitionSpec.java +++ b/api/src/main/java/org/apache/iceberg/PartitionSpec.java @@ -440,7 +440,7 @@ public Builder year(String sourceName, String targetName) { sourceColumn.fieldId(), nextFieldId(), targetName, - Transforms.year(sourceColumn.type())); + Transforms.year()); checkForRedundantPartitions(field); fields.add(field); return this; @@ -458,7 +458,7 @@ public Builder month(String sourceName, String targetName) { sourceColumn.fieldId(), nextFieldId(), targetName, - Transforms.month(sourceColumn.type())); + Transforms.month()); checkForRedundantPartitions(field); fields.add(field); return this; @@ -476,7 +476,7 @@ public Builder day(String sourceName, String targetName) { sourceColumn.fieldId(), nextFieldId(), targetName, - Transforms.day(sourceColumn.type())); + Transforms.day()); checkForRedundantPartitions(field); fields.add(field); return this; @@ -494,7 +494,7 @@ public Builder hour(String sourceName, String targetName) { sourceColumn.fieldId(), nextFieldId(), targetName, - Transforms.hour(sourceColumn.type())); + Transforms.hour()); checkForRedundantPartitions(field); fields.add(field); return this; diff --git a/api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java b/api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java index cc8526f9072c..b66ae4f8de5c 100644 --- a/api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java +++ b/api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java @@ -54,13 +54,7 @@ private PartitionSpec.Builder copyToBuilder(Schema schema) { PartitionSpec.Builder builder = PartitionSpec.builderFor(schema).withSpecId(specId); for (UnboundPartitionField field : fields) { - Type fieldType = schema.findType(field.sourceId); - Transform transform; - if (fieldType != null) { - transform = Transforms.fromString(fieldType, field.transform.toString()); - } else { - transform = Transforms.fromString(field.transform.toString()); - } + Transform transform = Transforms.fromString(field.transform.toString()); if (field.partitionId != null) { builder.add(field.sourceId, field.partitionId, field.name, transform); } else { diff --git a/core/src/test/java/org/apache/iceberg/TableTestBase.java b/core/src/test/java/org/apache/iceberg/TableTestBase.java index 0914f1d77370..e522f366a630 100644 --- a/core/src/test/java/org/apache/iceberg/TableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/TableTestBase.java @@ -52,7 +52,8 @@ public class TableTestBase { // Schema passed to create tables public static final Schema SCHEMA = new Schema( - required(3, "id", Types.IntegerType.get()), required(4, "data", Types.StringType.get())); + required(3, "id", Types.IntegerType.get()), required(4, "data", Types.StringType.get()), + required(5, "year_field", Types.DateType.get())); protected static final int BUCKETS_NUMBER = 16; diff --git a/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java b/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java index f770cd279287..079aa3c37145 100644 --- a/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java +++ b/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java @@ -20,6 +20,7 @@ import static org.apache.iceberg.expressions.Expressions.bucket; import static org.apache.iceberg.expressions.Expressions.truncate; +import static org.apache.iceberg.expressions.Expressions.year; import org.apache.iceberg.transforms.Transforms; import org.junit.Assert; @@ -187,6 +188,53 @@ public void testRemoveAndAddField() { Assert.assertEquals(1001, table.spec().lastAssignedFieldId()); } + @Test + public void testAddAfterRemoveTimeField() { + table.updateSpec().addField(year("year_field")).commit(); + + PartitionSpec newSpec = PartitionSpec.builderFor(table.schema()) + .withSpecId(1) + .bucket("data", 16) + .year("year_field") + .build(); + + Assert.assertEquals( + "Should have same transform class: org.apache.iceberg.transforms.Years", + newSpec.fields().get(1).transform().getClass().getName(), + table.spec().fields().get(1).transform().getClass().getName()); + + V1Assert.assertEquals("Should add a new id year", newSpec, table.spec()); + V2Assert.assertEquals( + "Should add a new id year", + PartitionSpec.builderFor(table.schema()) + .withSpecId(1) + .add(2, 1000, "data_bucket", Transforms.bucket(16)) + .add(3, 1001, "year_field_year", Transforms.year()) + .build(), + table.spec()); + + // remove and add a field with TimeTransform(Years, Months, Days, Hours) + table.updateSpec().removeField("year_field_year").addField(year("year_field")).commit(); + + V1Assert.assertEquals( + "Should remove and then add a year field", + PartitionSpec.builderFor(table.schema()) + .withSpecId(1) + .bucket("data", 16) + .year("year_field") + .build(), + table.spec()); + V2Assert.assertEquals( + "Should remove and then add a year field", + PartitionSpec.builderFor(table.schema()) + .withSpecId(1) + .add(2, 1000, "data_bucket", Transforms.bucket(16)) + .add(3, 1001, "year_field_year", Transforms.year()) + .build(), + table.spec()); + Assert.assertEquals(1001, table.spec().lastAssignedFieldId()); + } + @Test public void testAddAndRemoveField() { table.updateSpec().addField(bucket("data", 6)).removeField("data_bucket").commit(); From 891c820a519dd3107298188bdf48739c61735689 Mon Sep 17 00:00:00 2001 From: zhangbutao Date: Sat, 28 Jan 2023 17:13:36 +0800 Subject: [PATCH 2/2] fix tests & address review comments --- .../java/org/apache/iceberg/TableTestBase.java | 3 +-- .../iceberg/TestTableUpdatePartitionSpec.java | 14 ++++---------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TableTestBase.java b/core/src/test/java/org/apache/iceberg/TableTestBase.java index e522f366a630..0914f1d77370 100644 --- a/core/src/test/java/org/apache/iceberg/TableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/TableTestBase.java @@ -52,8 +52,7 @@ public class TableTestBase { // Schema passed to create tables public static final Schema SCHEMA = new Schema( - required(3, "id", Types.IntegerType.get()), required(4, "data", Types.StringType.get()), - required(5, "year_field", Types.DateType.get())); + required(3, "id", Types.IntegerType.get()), required(4, "data", Types.StringType.get())); protected static final int BUCKETS_NUMBER = 16; diff --git a/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java b/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java index 079aa3c37145..20191e4f045d 100644 --- a/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java +++ b/core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java @@ -23,6 +23,7 @@ import static org.apache.iceberg.expressions.Expressions.year; import org.apache.iceberg.transforms.Transforms; +import org.apache.iceberg.types.Types; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -190,6 +191,7 @@ public void testRemoveAndAddField() { @Test public void testAddAfterRemoveTimeField() { + table.updateSchema().addColumn("year_field", Types.DateType.get()).commit(); table.updateSpec().addField(year("year_field")).commit(); PartitionSpec newSpec = PartitionSpec.builderFor(table.schema()) @@ -202,16 +204,8 @@ public void testAddAfterRemoveTimeField() { "Should have same transform class: org.apache.iceberg.transforms.Years", newSpec.fields().get(1).transform().getClass().getName(), table.spec().fields().get(1).transform().getClass().getName()); - - V1Assert.assertEquals("Should add a new id year", newSpec, table.spec()); - V2Assert.assertEquals( - "Should add a new id year", - PartitionSpec.builderFor(table.schema()) - .withSpecId(1) - .add(2, 1000, "data_bucket", Transforms.bucket(16)) - .add(3, 1001, "year_field_year", Transforms.year()) - .build(), - table.spec()); + Assert.assertEquals("Should append a year partition field to the spec", newSpec, table.spec()); + Assert.assertEquals(1001, table.spec().lastAssignedFieldId()); // remove and add a field with TimeTransform(Years, Months, Days, Hours) table.updateSpec().removeField("year_field_year").addField(year("year_field")).commit();