rdblue commented on code in PR #16092:
URL: https://github.com/apache/iceberg/pull/16092#discussion_r3244791119
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -224,4 +255,130 @@ public String toString() {
.add("dv_cardinality", dvCardinality == null ? "null" : 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 byte[] dv = null;
+ private Long dvCardinality = null;
+
+ Builder addedFilesCount(int count) {
+ this.addedFilesCount = count;
+ return this;
+ }
+
+ Builder existingFilesCount(int count) {
+ this.existingFilesCount = count;
+ return this;
+ }
+
+ Builder deletedFilesCount(int count) {
+ this.deletedFilesCount = count;
+ return this;
+ }
+
+ Builder replacedFilesCount(int count) {
+ this.replacedFilesCount = count;
+ return this;
+ }
+
+ Builder addedRowsCount(long count) {
+ this.addedRowsCount = count;
+ return this;
+ }
+
+ Builder existingRowsCount(long count) {
+ this.existingRowsCount = count;
+ return this;
+ }
+
+ Builder deletedRowsCount(long count) {
+ this.deletedRowsCount = count;
+ return this;
+ }
+
+ Builder replacedRowsCount(long count) {
+ this.replacedRowsCount = count;
+ return this;
+ }
+
+ Builder minSequenceNumber(long 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;
+ return this;
+ }
+
+ Builder dvCardinality(Long cardinality) {
+ this.dvCardinality = cardinality;
+ return this;
+ }
+
+ ManifestInfoStruct build() {
+ Preconditions.checkArgument(
Review Comment:
For builders, we usually fail as early as possible. For this, it means
moving these checks into `addedFilesCount(int)` and similar methods. This keeps
the code more focused: problems that are specific to a value are checked when
setting that value, and checks in `build` are for consistency. For instance, we
could have a check here that `addedRowsCount == 0 || addedFilesCount > 0`
Another good side effect is the caller gets a stack trace with
`addedFilesCount` rather than `build`. This is minor because we typically see
builders configured and completed in one place, but there are some cases like
parsing that pass builders around the call stack and it is best to fail
immediately to point the caller to the right place in their own code.
I think the rationale for adding the checks here was that we use `-1` as a
sentinel value that means "not set", which has to be checked in `build`. But
that leads to bad error messages:
```java
ManifestInfoStruct.builder().build()
```
This fails with `Invalid added files count: -1 (must be >= 0)` instead of
`Missing required value: added files count`. I appreciate the adherence to
doing what `BaseFile` was doing, but in this case I think the right solution is
to use `Integer` and `null` for the default, then check the nulls here.
--
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]
