gaborkaszab commented on code in PR #16964:
URL: https://github.com/apache/iceberg/pull/16964#discussion_r3496938587
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -238,21 +231,29 @@ TrackedFileBuilder deletionVector(DeletionVector
newDeletionVector) {
Preconditions.checkArgument(newDeletionVector != null, "Invalid deletion
vector: null");
Preconditions.checkArgument(
contentType == FileContent.DATA,
- "Deletion vector can only be added to DATA entries, but entry type is:
%s",
+ "Cannot add deletion vector for file with content: %s",
contentType);
- Preconditions.checkArgument(
- this.deletionVector == null ||
!this.deletionVector.equals(newDeletionVector),
- "The same deletion vector already added");
+ if (deletionVector != null) {
+ Preconditions.checkArgument(
+ deletionVector.cardinality() < newDeletionVector.cardinality(),
+ "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+ deletionVector == null ? null : deletionVector.cardinality(),
Review Comment:
you're right. Now that I moved the Precondition into the "is not null" if,
the ternary is not needed anymore.
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -315,38 +314,20 @@ TrackedFile build() {
Preconditions.checkArgument(recordCount != null, "Missing required field:
record count");
Preconditions.checkArgument(
fileSizeInBytes != null, "Missing required field: file size in bytes");
- Preconditions.checkArgument(partitionData != null, "Missing required
field: partition data");
Review Comment:
I think it's valid not to set partition if the data file is unpartitioned.
See [my other PR](https://github.com/apache/iceberg/pull/17000) that sets
partition optional in the schema for exactly this.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileBuilder.java:
##########
@@ -351,6 +362,33 @@ public void invalidNullInputs() {
.hasMessage("Invalid source: null");
}
+ @Test
+ public void unpartitionedSpecWithNonNullPartitionFails() {
+ assertThatThrownBy(
+ () ->
+ TrackedFileBuilder.data(30L)
+ .partition(PartitionSpec.unpartitioned(), PARTITION_DATA))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid partition: must be null for unpartitioned spec");
+ }
+
+ @Test
+ public void unpartitionedSpecWithNullPartitionSucceeds() {
+ PartitionSpec unpartitioned = PartitionSpec.unpartitioned();
+ TrackedFile trackedFile =
+ TrackedFileBuilder.data(50L)
+ .formatVersion(FORMAT_VERSION_V4)
+ .location("s3://bucket/data/file.parquet")
+ .fileFormat(FileFormat.PARQUET)
+ .recordCount(2000L)
+ .fileSizeInBytes(12345L)
+ .partition(unpartitioned, null)
Review Comment:
it will, I verify here that specId could be null if the PartitionSpec is
unpartitioned. Alternatively, we could omit calling `partition` on the builder,
but this test is not for that purpose.
--
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]
