Skip to content

API: Fix inconsistent TimeTransform Type #6482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zhangbutao wants to merge 2 commits into from
Closed

API: Fix inconsistent TimeTransform Type #6482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5fa184
API: Fix inconsistent TimeTransform Type
zhangbutao Dec 22, 2022
891c820
fix tests & address review comments
zhangbutao Jan 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions api/src/main/java/org/apache/iceberg/PartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ public Builder year(String sourceName, String targetName) {
sourceColumn.fieldId(),
nextFieldId(),
targetName,
Transforms.year(sourceColumn.type()));

@Fokko Fokko Feb 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to remove those because then there is a situation where we don't know the sourceColumn.

Transforms.year());
checkForRedundantPartitions(field);
fields.add(field);
return this;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down
8 changes: 1 addition & 7 deletions api/src/main/java/org/apache/iceberg/UnboundPartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
42 changes: 42 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@

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.apache.iceberg.types.Types;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -187,6 +189,46 @@ public void testRemoveAndAddField() {
Assert.assertEquals(1001, table.spec().lastAssignedFieldId());
}

@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())
.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());
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();

@hililiwei hililiwei Dec 23, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

@zhangbutao zhangbutao Jan 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this case. Other tests have include the case, eg. testAddAfterLastFieldRemoved


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(

@hililiwei hililiwei Dec 23, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, these two tests are essentially the same for me, right? Please correct me if I missed something.

@zhangbutao zhangbutao Jan 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. test1: adding a year field to the spec(table.updateSpec().addField(year("year_field")).commit();)
    I want to verify if the actual table transform type is same as PartitionSpec.builderFor and the transform type class should be org.apache.iceberg.transforms.Years. If not same, the test2 will also faill.

  2. test2: table.updateSpec().removeField("year_field_year").addField(year("year_field")).commit();
    Because i have encountered exception when removed and then add a same field, i want to check if this could work after the fix. To be exact, test2 is a e2e test.

@zhangbutao zhangbutao Jan 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, You mean V1Assert.assertEquals and V2Assert.assertEquals? I just follow other tests pattern and i think there maybe have some difference bettewn v2 and v1 formation, but i have not make more research.

"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();
Expand Down