stevenzwu commented on code in PR #16408: URL: https://github.com/apache/iceberg/pull/16408#discussion_r3338857155
########## core/src/main/java/org/apache/iceberg/TrackingBuilder.java: ########## @@ -0,0 +1,179 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import java.nio.ByteBuffer; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.util.ByteBuffers; + +class TrackingBuilder { + private final EntryStatus status; + private final Long snapshotId; + private final Long dataSequenceNumber; + private final Long fileSequenceNumber; + private final Long firstRowId; + // ID of the snapshot in which the new Tracking instance will be committed. + private final long newSnapshotId; + private Long dvSnapshotId; + private byte[] deletedPositions; + private byte[] replacedPositions; + + /** + * Creates a builder for a newly added file. + * + * @param snapshotId the snapshot ID in which the new tracking instance will be committed + */ + static TrackingBuilder added(long snapshotId) { Review Comment: nit: parameter is `snapshotId` here and on the private `TrackingBuilder(long)` ctor, while the other factories (`builder`, `delete`, `replace`) all take `newSnapshotId`. The javadoc here even describes the param as "the snapshot ID in which the new tracking instance will be committed" — same semantic. Renaming to `newSnapshotId` everywhere would make the meaning uniform. ########## core/src/main/java/org/apache/iceberg/TrackingBuilder.java: ########## @@ -0,0 +1,179 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import java.nio.ByteBuffer; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.util.ByteBuffers; + +class TrackingBuilder { Review Comment: what is the reason refactor the builder class out of the `TrackingStruct` to a top level class? ########## 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 positive)", cardinality); Review Comment: `DeletionVectorStruct.Builder.cardinality` was relaxed to `>= 0` in the latest commit, but the parallel `dvCardinality` setter here still requires `> 0`. These represent the same value, so the validations should match for the same reason raised on `TestDeletionVectorStruct.java`. `testBuilderRejectsNonPositiveDvCardinality` would need to be updated alongside. -- 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]
