Skip to content

Returns isUnpartitioned=true for VoidTransform on all fields #3059

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
RussellSpitzer merged 1 commit into from
Dec 9, 2022
Merged

Returns isUnpartitioned=true for VoidTransform on all fields #3059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion 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 @@ -87,7 +87,7 @@ public List<PartitionField> fields() {
}

public boolean isPartitioned() {
return fields.length > 0;
return fields.length > 0 && fields().stream().anyMatch(f -> !f.transform().isVoid());
}

public boolean isUnpartitioned() {
Expand Down
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 @@ -136,6 +136,15 @@ default boolean isIdentity() {
return false;
}

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

/**
* Returns a human-readable String representation of a transformed value.
*
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 @@ -79,6 +79,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/DataFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ static PartitionData newPartitionData(PartitionSpec spec) {

static PartitionData copyPartitionData(
PartitionSpec spec, StructLike partitionData, PartitionData reuse) {
Preconditions.checkArgument(
spec.isPartitioned(), "Can't copy partition data to a unpartitioned table");
PartitionData data = reuse;
if (data == null) {
data = newPartitionData(spec);
Expand Down Expand Up @@ -136,7 +138,7 @@ public static class Builder {
public Builder(PartitionSpec spec) {
this.spec = spec;
this.specId = spec.specId();
this.isPartitioned = spec.fields().size() > 0;
this.isPartitioned = spec.isPartitioned();
this.partitionData = isPartitioned ? newPartitionData(spec) : null;
}

Expand Down Expand Up @@ -219,7 +221,9 @@ public Builder withFormat(FileFormat newFormat) {
}

public Builder withPartition(StructLike newPartition) {
this.partitionData = copyPartitionData(spec, newPartition, partitionData);
if (isPartitioned) {
this.partitionData = copyPartitionData(spec, newPartition, partitionData);
}
Comment on lines +224 to +226
Copy link
Contributor Author

@xinbinhuang xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

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

Only copy if the spec is partitionable.

Copy link
Member

@RussellSpitzer RussellSpitzer Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would rather we fix this in one of the other ways we discussed. This ends up feeling a little strange to me since you can call "withPartition" and the arg is ignored. I don't like when functions can take arguments but then not doing anything with them.

Copy link
Contributor Author

@xinbinhuang xinbinhuang Nov 19, 2021

Choose a reason for hiding this comment

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

@aokolnychyi Do you mind taking a look at this change?

I have a chat with @RussellSpitzer offline, and would love to have your thoughts on this.

The current implementation

Alternatively, we are thinking:

GenericRecord emptyRecord = GenericRecord.create(spec.schema());
delegate = writerFactory.newDataWriter(outputFile, spec, emptyRecord);

at around here:

iceberg/spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java

Line 594 in 1e5abce

delegate = writerFactory.newDataWriter(outputFile, spec, null);

return this;
}

Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/org/apache/iceberg/FileMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public static class Builder {
Builder(PartitionSpec spec) {
this.spec = spec;
this.specId = spec.specId();
this.isPartitioned = spec.fields().size() > 0;
this.isPartitioned = spec.isPartitioned();
this.partitionData = isPartitioned ? DataFiles.newPartitionData(spec) : null;
}

Expand Down Expand Up @@ -154,7 +154,9 @@ public Builder withFormat(FileFormat newFormat) {
}

public Builder withPartition(StructLike newPartition) {
this.partitionData = DataFiles.copyPartitionData(spec, newPartition, partitionData);
if (isPartitioned) {
this.partitionData = DataFiles.copyPartitionData(spec, newPartition, partitionData);
}
Comment on lines +157 to +159
Copy link
Contributor Author

@xinbinhuang xinbinhuang Nov 13, 2021

Choose a reason for hiding this comment

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

Do similar thing to FileMetadata.java

return this;
}

Expand Down
9 changes: 9 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestPartitionSpecInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,20 @@ public void cleanupTables() {
TestTables.clearTables();
}

@Test
public void testSpecIsUnpartitionedForVoidTranforms() {
PartitionSpec spec =
PartitionSpec.builderFor(schema).alwaysNull("id").alwaysNull("data").build();

Assert.assertTrue(spec.isUnpartitioned());
}

@Test
public void testSpecInfoUnpartitionedTable() {
PartitionSpec spec = PartitionSpec.unpartitioned();
TestTables.TestTable table = TestTables.create(tableDir, "test", schema, spec, formatVersion);

Assert.assertTrue(spec.isUnpartitioned());
Assert.assertEquals(spec, table.spec());
Assert.assertEquals(spec.lastAssignedFieldId(), table.spec().lastAssignedFieldId());
Assert.assertEquals(ImmutableMap.of(spec.specId(), spec), table.specs());
Expand Down
5 changes: 5 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestPartitioning.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ public void testPartitionTypeWithSpecEvolutionInV1Tables() {
NestedField.optional(1001, "category_bucket_8", Types.IntegerType.get()));
StructType actualType = Partitioning.partitionType(table);
Assert.assertEquals("Types must match", expectedType, actualType);

table.updateSpec().removeField("data").removeField("category_bucket_8").commit();

Assert.assertEquals("Should have 3 specs", 3, table.specs().size());
Assert.assertTrue("PartitionSpec should be unpartitioned", table.spec().isUnpartitioned());
}

@Test
Expand Down