xinbinhuang commented on a change in pull request #3059:
URL: https://github.com/apache/iceberg/pull/3059#discussion_r748750150
##########
File path: core/src/main/java/org/apache/iceberg/DataFiles.java
##########
@@ -43,6 +43,7 @@ static PartitionData newPartitionData(PartitionSpec spec) {
}
static PartitionData copyPartitionData(PartitionSpec spec, StructLike
partitionData, PartitionData reuse) {
+ Preconditions.checkArgument(!spec.isUnpartitioned(), "Can't copy partition
data to a unpartitioned table");
Review comment:
The underlying method explicitly check for `isUnpartitioned()` in case
this method is called outside the builder. (`FileMetadata.java`)
##########
File path: core/src/main/java/org/apache/iceberg/FileMetadata.java
##########
@@ -63,7 +63,7 @@ public static Builder deleteFileBuilder(PartitionSpec spec) {
Builder(PartitionSpec spec) {
this.spec = spec;
this.specId = spec.specId();
- this.isPartitioned = spec.fields().size() > 0;
+ this.isPartitioned = !spec.isUnpartitioned();
Review comment:
Do similar thing to `FileMetadata.java`
##########
File path: core/src/main/java/org/apache/iceberg/DataFiles.java
##########
@@ -218,7 +219,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);
+ }
Review comment:
Only copy if the spec is partitionable.
##########
File path: core/src/main/java/org/apache/iceberg/FileMetadata.java
##########
@@ -156,7 +156,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);
+ }
Review comment:
Do similar thing to `FileMetadata.java`
##########
File path: core/src/main/java/org/apache/iceberg/DataFiles.java
##########
@@ -136,7 +137,7 @@ public static Builder builder(PartitionSpec spec) {
public Builder(PartitionSpec spec) {
this.spec = spec;
this.specId = spec.specId();
- this.isPartitioned = spec.fields().size() > 0;
+ this.isPartitioned = !spec.isUnpartitioned();
Review comment:
More inclusive check for `isPartitioned` to includes V1 table
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
