anoopj commented on code in PR #16408:
URL: https://github.com/apache/iceberg/pull/16408#discussion_r3345522607
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -252,121 +252,140 @@ public String toString() {
.add("replaced_rows_count", replacedRowsCount)
.add("min_sequence_number", minSequenceNumber)
.add("dv", dv == null ? "null" : "(binary)")
- .add("dv_cardinality", dvCardinality == null ? "null" : dvCardinality)
+ .add("dv_cardinality", dvCardinality)
.toString();
}
static class Builder {
- private int addedFilesCount = -1;
- private int existingFilesCount = -1;
- private int deletedFilesCount = -1;
- private int replacedFilesCount = -1;
- private long addedRowsCount = -1L;
- private long existingRowsCount = -1L;
- private long deletedRowsCount = -1L;
- private long replacedRowsCount = -1L;
- private long minSequenceNumber = -1L;
+ private Integer addedFilesCount = null;
+ private Integer existingFilesCount = null;
+ private Integer deletedFilesCount = null;
+ private Integer replacedFilesCount = null;
+ private Long addedRowsCount = null;
+ private Long existingRowsCount = null;
+ private Long deletedRowsCount = null;
+ private Long replacedRowsCount = null;
+ private Long minSequenceNumber = null;
private byte[] dv = null;
private Long dvCardinality = null;
Builder addedFilesCount(int count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid added files count: %s (must be >= 0)", count);
this.addedFilesCount = count;
return this;
}
Builder existingFilesCount(int count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid existing files count: %s (must be >= 0)",
count);
this.existingFilesCount = count;
return this;
}
Builder deletedFilesCount(int count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid deleted files count: %s (must be >= 0)", count);
this.deletedFilesCount = count;
return this;
}
Builder replacedFilesCount(int count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid replaced files count: %s (must be >= 0)",
count);
this.replacedFilesCount = count;
return this;
}
Builder addedRowsCount(long count) {
+ Preconditions.checkArgument(count >= 0, "Invalid added rows count: %s
(must be >= 0)", count);
this.addedRowsCount = count;
return this;
}
Builder existingRowsCount(long count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid existing rows count: %s (must be >= 0)", count);
this.existingRowsCount = count;
return this;
}
Builder deletedRowsCount(long count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid deleted rows count: %s (must be >= 0)", count);
this.deletedRowsCount = count;
return this;
}
Builder replacedRowsCount(long count) {
+ Preconditions.checkArgument(
+ count >= 0, "Invalid replaced rows count: %s (must be >= 0)", count);
this.replacedRowsCount = count;
return this;
}
Builder minSequenceNumber(long sequenceNumber) {
+ Preconditions.checkArgument(
+ sequenceNumber >= 0, "Invalid min sequence number: %s (must be >=
0)", sequenceNumber);
this.minSequenceNumber = sequenceNumber;
return this;
}
Builder dv(ByteBuffer buffer) {
- this.dv = buffer != null ? ByteBuffers.toByteArray(buffer) : null;
- return this;
- }
-
- Builder dv(byte[] buffer) {
- this.dv = buffer;
+ Preconditions.checkArgument(buffer != null, "Invalid DV: null");
+ this.dv = ByteBuffers.toByteArray(buffer);
return this;
}
- Builder dvCardinality(Long cardinality) {
+ Builder dvCardinality(long cardinality) {
+ Preconditions.checkArgument(
+ cardinality >= 0, "Invalid DV cardinality: %s (must be >= 0)",
cardinality);
this.dvCardinality = cardinality;
return this;
}
ManifestInfoStruct build() {
Preconditions.checkArgument(
- addedFilesCount >= 0, "Invalid added files count: %s (must be >=
0)", addedFilesCount);
+ addedFilesCount != null, "Missing required value: added files
count");
Preconditions.checkArgument(
- existingFilesCount >= 0,
- "Invalid existing files count: %s (must be >= 0)",
- existingFilesCount);
+ existingFilesCount != null, "Missing required value: existing files
count");
Preconditions.checkArgument(
- deletedFilesCount >= 0,
- "Invalid deleted files count: %s (must be >= 0)",
- deletedFilesCount);
+ deletedFilesCount != null, "Missing required value: deleted files
count");
Preconditions.checkArgument(
- replacedFilesCount >= 0,
- "Invalid replaced files count: %s (must be >= 0)",
- replacedFilesCount);
+ replacedFilesCount != null, "Missing required value: replaced files
count");
+ Preconditions.checkArgument(
+ addedRowsCount != null, "Missing required value: added rows count");
+ Preconditions.checkArgument(
+ existingRowsCount != null, "Missing required value: existing rows
count");
+ Preconditions.checkArgument(
+ deletedRowsCount != null, "Missing required value: deleted rows
count");
Preconditions.checkArgument(
- addedRowsCount >= 0, "Invalid added rows count: %s (must be >= 0)",
addedRowsCount);
+ replacedRowsCount != null, "Missing required value: replaced rows
count");
Preconditions.checkArgument(
- existingRowsCount >= 0,
- "Invalid existing rows count: %s (must be >= 0)",
- existingRowsCount);
+ minSequenceNumber != null, "Missing required value: min sequence
number");
Preconditions.checkArgument(
- deletedRowsCount >= 0, "Invalid deleted rows count: %s (must be >=
0)", deletedRowsCount);
+ addedRowsCount == 0 || addedFilesCount > 0,
Review Comment:
The negative values are rejected at the setters (addedRowsCount(...),
addedFilesCount(...) etc. all check >= 0 since the last commit). The idea was
to reject invalid values as early as possible, instead of failing `build()`. So
the composite check only needs to validate combinations of non-negative values.
> individual validation can also miss some conditions like addedRowsCount >
0 && addedFilesCount == 0
The check `addedRowsCount == 0 || addedFilesCount > 0` will prevent this.
It would
Another case is files > 0 and rows = 0. That is a valid scenario (empty data
files were written).
--
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]
