stevenzwu commented on code in PR #16964:
URL: https://github.com/apache/iceberg/pull/16964#discussion_r3477812540
##########
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:
might be a little cleaner to move these two checks inside a if block, like
```
if (deletionVector != null) {
Preconditions.checkArgument("deletionVector.cardinality() <
newDeletionVector.cardinality(), ...);
Preconditions.checkArgument("!deletionVector.location().equals(newDeletionVector.location())
|| deletionVector.offset() != newDeletionVector.offset()", ...);
}
##########
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:
Minor: this message ("same location and offset has changed cardinality")
reads awkwardly. Since the cardinality-must-increase rule above is intentional,
reaching this check means the cardinality already changed, so this is really
guarding against an unchanged location and offset. Something like "cardinality
changed but location and offset are unchanged" would read more clearly.
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -147,18 +139,17 @@ private static TrackedFile terminal(TrackedFile source,
Tracking tracking) {
private TrackedFileBuilder(FileContent contentType, long snapshotId) {
this.contentType = contentType;
- this.snapshotId = snapshotId;
+ this.trackingBuilder = TrackingBuilder.added(snapshotId);
}
private TrackedFileBuilder(TrackedFile source, long snapshotId) {
this.contentType = source.contentType();
- this.snapshotId = snapshotId;
this.formatVersion = source.formatVersion();
this.location = source.location();
this.fileFormat = source.fileFormat();
this.recordCount = source.recordCount();
this.fileSizeInBytes = source.fileSizeInBytes();
- this.partitionData = (PartitionData) source.partition();
+ this.partition = (PartitionData) source.partition();
Review Comment:
Casting `partition()` to `PartitionData` where the concrete type is actually
needed is already the established pattern in core. For example,
`ManifestFilterManager`:
```java
PartitionData partition = (PartitionData) file.partition();
```
We can't narrow `TrackedFile.partition()` to return `PartitionData` to avoid
the cast either. Every file and scan type in the codebase exposes `partition()`
as `StructLike` — `ContentFile`, `BaseFile`, `V1Metadata`/`V2Metadata`,
`PartitionScanTask`, `ContentScanTask`, etc.
--
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]
