You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/12/23 13:15:42 UTC
[GitHub] [iceberg] hililiwei commented on a diff in pull request #6482: API: Fix inconsistent TimeTransform Type
hililiwei commented on code in PR #6482:
URL: https://github.com/apache/iceberg/pull/6482#discussion_r1056334660
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -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());
Review Comment:
This seems superfluous; the previous one is enough? If needed, we can add
`Assert.assertEquals(1001, table.spec().lastAssignedFieldId());`
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -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(
Review Comment:
Again, these two tests are essentially the same for me, right? Please correct me if I missed something.
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -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();
Review Comment:
Do we need to add a case that remove `year_field_year` first and then add `year_field_year` after the deletion is successful?
```
table.updateSpec().removeField("year_field_year").commit();
assert……
table.updateSpec().addField(year("year_field")).commit();
```
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -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());
Review Comment:
nit: `Should append a year partition field to the spec`?
--
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