Skip to content

fix: add and remove partition transform on same column failed when use v1 metadata #2691

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

Merged
pvary merged 6 commits into from
Jun 21, 2021
Merged

fix: add and remove partition transform on same column failed when use v1 metadata #2691

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
563fca3
fix: add and remove partition transform on same column failed when us…
Jun 10, 2021
a9035f2
fix ut failed in TestUpdatePartitionSpec and TestTableUpdatePartition…
Jun 10, 2021
4877bdc
fix ut failed in TestAlterTablePartitionFields
Jun 10, 2021
adf19d7
rename void transfrom partition name when conflict is detected
Jun 16, 2021
c9a429b
move conflict detect and handle code to addField and renameField
Jun 17, 2021
00a48a3
avoid change adds type by regenerate PartitionField if name is null
Jun 18, 2021
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
9 changes: 9 additions & 0 deletions api/src/main/java/org/apache/iceberg/transforms/Transform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@ default boolean isIdentity() {
return false;
}

/**
* Return whether this transform is the void transform.
*
* @return true if this is an void transform, false otherwise
*/
default boolean isVoid() {
return false;
}

/**
* Returns a human-readable String representation of a transformed value.
* <p>
Expand Down
5 changes: 5 additions & 0 deletions api/src/main/java/org/apache/iceberg/transforms/VoidTransform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ public UnboundPredicate<Void> project(String name, BoundPredicate<S> predicate)
return null;
}

@Override
public boolean isVoid() {
return true;
}

@Override
public String toHumanString(Void value) {
return "null";
Expand Down
8 changes: 6 additions & 2 deletions core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ public PartitionSpec apply() {
// field IDs were not required for v1 and were assigned sequentially in each partition spec starting at 1,000.
// to maintain consistent field ids across partition specs in v1 tables, any partition field that is removed
// must be replaced with a null transform. null values are always allowed in partition data.
builder.add(field.sourceId(), field.fieldId(), field.name(), Transforms.alwaysNull());
// To avoid name conflict when add and remove same partition transform multiple times, field name will be
// replaced by field name append with field id.
builder.add(field.sourceId(), field.fieldId(), field.name() + "_" + field.fieldId(), Transforms.alwaysNull());
}
}

Expand Down Expand Up @@ -290,7 +292,9 @@ private static Map<Pair<Integer, String>, PartitionField> indexSpecByTransform(P
ImmutableMap.Builder<Pair<Integer, String>, PartitionField> builder = ImmutableMap.builder();
List<PartitionField> fields = spec.fields();
for (PartitionField field : fields) {
builder.put(Pair.of(field.sourceId(), field.transform().toString()), field);
if (!field.transform().isVoid()) {
builder.put(Pair.of(field.sourceId(), field.transform().toString()), field);
}
}

return builder.build();
Expand Down
12 changes: 6 additions & 6 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 @@ -73,8 +73,8 @@ public void testCommitUpdatedSpec() {

V1Assert.assertEquals("Should soft delete id and data buckets", PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("data", "data_bucket")
.alwaysNull("id", "id_bucket_8")
.alwaysNull("data", "data_bucket_1000")
.alwaysNull("id", "id_bucket_8_1001")
.truncate("data", 8, "data_trunc_8")
.build(), table.spec());

Expand Down Expand Up @@ -166,7 +166,7 @@ public void testRemoveAndAddField() {

V1Assert.assertEquals("Should soft delete data bucket", PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("data", "data_bucket")
.alwaysNull("data", "data_bucket_1000")
.bucket("id", 8, "id_bucket_8")
.build(), table.spec());

Expand All @@ -187,7 +187,7 @@ public void testAddAndRemoveField() {

V1Assert.assertEquals("Should remove and then add a bucket field", PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("data", "data_bucket")
.alwaysNull("data", "data_bucket_1000")
.bucket("data", 6, "data_bucket_6")
.build(), table.spec());
V2Assert.assertEquals("Should remove and then add a bucket field", PartitionSpec.builderFor(table.schema())
Expand All @@ -205,7 +205,7 @@ public void testAddAfterLastFieldRemoved() {

V1Assert.assertEquals("Should add a new id bucket", PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("data", "data_bucket")
.alwaysNull("data", "data_bucket_1000")
.build(), table.spec());
V1Assert.assertEquals("Should match the last assigned field id",
1000, table.spec().lastAssignedFieldId());
Expand All @@ -222,7 +222,7 @@ public void testAddAfterLastFieldRemoved() {

V1Assert.assertEquals("Should add a new id bucket", PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("data", "data_bucket")
.alwaysNull("data", "data_bucket_1000")
.bucket("id", 8, "id_bucket_8")
.build(), table.spec());
V2Assert.assertEquals("Should add a new id bucket", PartitionSpec.builderFor(table.schema())
Expand Down
47 changes: 40 additions & 7 deletions core/src/test/java/org/apache/iceberg/TestUpdatePartitionSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ public void testRemoveIdentityByName() {
.apply();

PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.alwaysNull("category", "category")
.alwaysNull("category", "category_1000")
.day("ts")
.bucket("id", 16, "shard")
.build();
Expand All @@ -282,7 +282,7 @@ public void testRemoveBucketByName() {
PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.identity("category")
.day("ts")
.alwaysNull("id", "shard")
.alwaysNull("id", "shard_1002")
.build();

V1Assert.assertEquals("Should match expected spec", v1Expected, updated);
Expand All @@ -302,7 +302,7 @@ public void testRemoveIdentityByEquivalent() {
.apply();

PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.alwaysNull("category", "category")
.alwaysNull("category", "category_1000")
.day("ts")
.bucket("id", 16, "shard")
.build();
Expand All @@ -325,7 +325,7 @@ public void testRemoveDayByEquivalent() {

PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.identity("category")
.alwaysNull("ts", "ts_day")
.alwaysNull("ts", "ts_day_1001")
.bucket("id", 16, "shard")
.build();

Expand All @@ -348,7 +348,7 @@ public void testRemoveBucketByEquivalent() {
PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.identity("category")
.day("ts")
.alwaysNull("id", "shard")
.alwaysNull("id", "shard_1002")
.build();

V1Assert.assertEquals("Should match expected spec", v1Expected, updated);
Expand Down Expand Up @@ -386,7 +386,7 @@ public void testMultipleChanges() {

PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.identity("category")
.alwaysNull("ts", "ts_day")
.alwaysNull("ts", "ts_day_1001")
.bucket("id", 16)
.truncate("data", 4, "prefix")
.build();
Expand All @@ -411,7 +411,7 @@ public void testAddDeletedName() {
PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.identity("category")
.day("ts")
.alwaysNull("id", "shard")
.alwaysNull("id", "shard_1002")
.build();

V1Assert.assertEquals("Should match expected spec", v1Expected, updated);
Expand Down Expand Up @@ -569,6 +569,39 @@ public void testRenameAndDelete() {
.renameField("shard", "id_bucket"));
}

@Test
public void testRemoveAndAddMultiTimes() {
PartitionSpec addFirstTime = new BaseUpdatePartitionSpec(formatVersion, UNPARTITIONED)
.addField(day("ts"))
.apply();
PartitionSpec removeFirstTime = new BaseUpdatePartitionSpec(formatVersion, addFirstTime)
.removeField(day("ts"))
.apply();
PartitionSpec addSecondTime = new BaseUpdatePartitionSpec(formatVersion, removeFirstTime)
.addField(day("ts"))
.apply();
PartitionSpec removeSecondTime = new BaseUpdatePartitionSpec(formatVersion, addSecondTime)
.removeField(day("ts"))
.apply();
PartitionSpec updated = new BaseUpdatePartitionSpec(formatVersion, removeSecondTime)
.addField(day("ts"))
.apply();

PartitionSpec v1Expected = PartitionSpec.builderFor(SCHEMA)
.alwaysNull("ts", "ts_day_1000")
.alwaysNull("ts", "ts_day_1001")
.day("ts")
.build();

V1Assert.assertEquals("Should match expected spec", v1Expected, updated);

PartitionSpec v2Expected = PartitionSpec.builderFor(SCHEMA)
.day("ts")
.build();

V2Assert.assertEquals("Should match expected spec", v2Expected, updated);
}

private static int id(String name) {
return SCHEMA.findField(name).fieldId();
}
Expand Down
16 changes: 8 additions & 8 deletions ...ions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public void testDropIdentityPartition() {

PartitionSpec expected = PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("category", "category")
.alwaysNull("category", "category_1000")
.build();

Assert.assertEquals("Should have new spec field", expected, table.spec());
Expand All @@ -226,7 +226,7 @@ public void testDropDaysPartition() {

PartitionSpec expected = PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("ts", "ts_day")
.alwaysNull("ts", "ts_day_1000")
.build();

Assert.assertEquals("Should have new spec field", expected, table.spec());
Expand All @@ -246,7 +246,7 @@ public void testDropBucketPartition() {

PartitionSpec expected = PartitionSpec.builderFor(table.schema())
.withSpecId(1)
.alwaysNull("id", "id_bucket")
.alwaysNull("id", "id_bucket_1000")
.build();

Assert.assertEquals("Should have new spec field", expected, table.spec());
Expand All @@ -271,7 +271,7 @@ public void testDropPartitionByName() {

PartitionSpec expected = PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("id", "shard")
.alwaysNull("id", "shard_1000")
.build();

Assert.assertEquals("Should have new spec field", expected, table.spec());
Expand All @@ -295,7 +295,7 @@ public void testReplacePartition() {
table.refresh();
expected = PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("ts", "ts_day")
.alwaysNull("ts", "ts_day_1000")
.hour("ts")
.build();
Assert.assertEquals("Should changed from daily to hourly partitioned field", expected, table.spec());
Expand All @@ -319,7 +319,7 @@ public void testReplacePartitionAndRename() {
table.refresh();
expected = PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("ts", "ts_day")
.alwaysNull("ts", "ts_day_1000")
.hour("ts", "hour_col")
.build();
Assert.assertEquals("Should changed from daily to hourly partitioned field", expected, table.spec());
Expand All @@ -343,7 +343,7 @@ public void testReplaceNamedPartition() {
table.refresh();
expected = PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("ts", "day_col")
.alwaysNull("ts", "day_col_1000")
.hour("ts")
.build();
Assert.assertEquals("Should changed from daily to hourly partitioned field", expected, table.spec());
Expand All @@ -367,7 +367,7 @@ public void testReplaceNamedPartitionAndRenameDifferently() {
table.refresh();
expected = PartitionSpec.builderFor(table.schema())
.withSpecId(2)
.alwaysNull("ts", "day_col")
.alwaysNull("ts", "day_col_1000")
.hour("ts", "hour_col")
.build();
Assert.assertEquals("Should changed from daily to hourly partitioned field", expected, table.spec());
Expand Down