gaborkaszab commented on code in PR #16964:
URL: https://github.com/apache/iceberg/pull/16964#discussion_r3481380185
##########
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:
fixed
##########
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:
Additional context:
[This](https://github.com/apache/iceberg/pull/16769/changes#r3463659478) was
the original comment.
The intention was to prevent adding the same DV through the builder, because
that bumps the `dv_snapshot_id` while not changing the DV itself. I first
introduced an override for `equals` in `DeletionVectorStruct` but we decided to
avoid such overrides.
As a workaround @rdblue suggested that we can "mimic" equality checks here
by requiring 2 things:
1. if DV cardinality doesn't increase than we probably add the same DV again
2. if cardinality increased, that's great, but expectation is that we don't
write the new DV in-place.
I haven't thought about DV compaction, seems something we might want to
discuss in the future. If we decide to support it, we can still
remove/restructure the conditions here. WDYT?
##########
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:
thx, reads much better! How about this?
`cardinality changed but location and offset are the same`
Update: probably we shouldn't include the cardinality part into this
message. Currently we don't allow adding DV with same location and offset that
we already know of, regardless of cardinality.
Maybe this check can come first before the cardinality check? WDYT?
##########
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:
I agree. Simplified the message here, and don't mention 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:
thx, was going to, but then I forgot :)
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -23,19 +23,19 @@
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
class TrackedFileBuilder {
- private final long snapshotId;
private final FileContent contentType;
+ private final TrackingBuilder trackingBuilder;
Review Comment:
technically, nothing changes from the user's perspective, it's just how we
create Tracking internally.
##########
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:
That's what I thought too, but apparently these` Preconditions` parameters
aren't lazily evaluated. Without the ternary every test that wanted to set a DV
gave NPE on this.
--
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]
