anoopj commented on code in PR #16964:
URL: https://github.com/apache/iceberg/pull/16964#discussion_r3478017641
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -238,21 +229,28 @@ 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");
+ deletionVector == null || deletionVector.cardinality() <
newDeletionVector.cardinality(),
Review Comment:
I don't believe the spec requiring increasing cardinality. A writer could
reserialize the DVs into a new puffin blob. Should we allow equal cardinality?
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -238,21 +229,28 @@ 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");
+ deletionVector == null || deletionVector.cardinality() <
newDeletionVector.cardinality(),
+ "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+ deletionVector == null ? null : deletionVector.cardinality(),
Review Comment:
ternary is not needed because your precondition check already validates
`null`
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -238,21 +229,28 @@ 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");
+ deletionVector == null || deletionVector.cardinality() <
newDeletionVector.cardinality(),
+ "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+ deletionVector == null ? null : deletionVector.cardinality(),
+ newDeletionVector.cardinality());
+ Preconditions.checkArgument(
+ deletionVector == null
+ || !deletionVector.location().equals(newDeletionVector.location())
+ || deletionVector.offset() != newDeletionVector.offset(),
+ "Invalid DV update: same location and offset has changed cardinality");
Review Comment:
This is moot if we allow equal cardinality, but the error message part about
cardinality is dependent on prior precondition check. It can be a bit
confusing.
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -238,21 +229,28 @@ 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");
+ deletionVector == null || deletionVector.cardinality() <
newDeletionVector.cardinality(),
+ "Invalid DV update, cardinality must increase: existing=%s, new=%s",
+ deletionVector == null ? null : deletionVector.cardinality(),
Review Comment:
```suggestion
deletionVector.cardinality(),
```
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -205,15 +196,17 @@ TrackedFileBuilder fileSizeInBytes(long
newFileSizeInBytes) {
return this;
}
- TrackedFileBuilder specId(int newSpecId) {
- Preconditions.checkArgument(newSpecId >= 0, "Invalid spec ID: %s (must be
>= 0)", newSpecId);
- this.specId = newSpecId;
- return this;
- }
-
- TrackedFileBuilder partition(PartitionData newPartitionData) {
- Preconditions.checkArgument(newPartitionData != null, "Invalid partition:
null");
- this.partitionData = newPartitionData;
+ TrackedFileBuilder partition(PartitionSpec spec, StructLike newPartition) {
+ Preconditions.checkArgument(spec != null, "Invalid spec: null");
+ if (spec.isUnpartitioned()) {
+ Preconditions.checkArgument(
+ newPartition == null, "Invalid partition: must be null for
unpartitioned spec");
+ this.specId = spec.specId();
Review Comment:
Minor: this is in both branches of the `if`. Hoist it out?
--
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]
