gaborkaszab commented on code in PR #17041:
URL: https://github.com/apache/iceberg/pull/17041#discussion_r3544405665
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -212,274 +257,23 @@ void testInheritFromNullIsNoOp() {
}
private static Tracking createManifestTracking(long snapshotId, long
sequenceNumber) {
- TrackingStruct tracking = new TrackingStruct(Tracking.schema());
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, snapshotId);
- tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, sequenceNumber);
- tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, sequenceNumber);
- return tracking;
- }
-
- @Test
- void testAddedWithSameCommitDvStaysAdded() {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- assertThat(tracking.status()).isEqualTo(EntryStatus.ADDED);
- assertThat(tracking.snapshotId()).isEqualTo(42L);
- assertThat(tracking.dvSnapshotId()).isEqualTo(42L);
- assertThat(tracking.deletedPositions()).isNull();
- assertThat(tracking.replacedPositions()).isNull();
- // sequence numbers and firstRowId remain null; populated by inheritance
- assertThat(tracking.dataSequenceNumber()).isNull();
- assertThat(tracking.fileSequenceNumber()).isNull();
- assertThat(tracking.firstRowId()).isNull();
- }
-
- @Test
- void testExistingBuilderPreservesSourceFields() {
- Tracking source = sourceTracking();
-
- Tracking existing = TrackingBuilder.from(source, 1L).build();
-
- assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
- assertThat(existing.snapshotId()).isEqualTo(source.snapshotId());
-
assertThat(existing.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(existing.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(existing.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(existing.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testDeleteUpdatesSnapshotIdAndPreservesRest() {
- Tracking source = sourceTracking();
-
- Tracking deleted = TrackingBuilder.deleted(source, 999L);
-
- assertThat(deleted.status()).isEqualTo(EntryStatus.DELETED);
- assertThat(deleted.snapshotId()).isEqualTo(999L);
-
assertThat(deleted.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(deleted.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(deleted.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(deleted.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testReplaceUpdatesSnapshotIdAndPreservesRest() {
- Tracking source = sourceTracking();
-
- Tracking replaced = TrackingBuilder.replaced(source, 999L);
-
- assertThat(replaced.status()).isEqualTo(EntryStatus.REPLACED);
- assertThat(replaced.snapshotId()).isEqualTo(999L);
-
assertThat(replaced.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(replaced.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(replaced.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(replaced.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testSourceDvPositionsAreNotCarriedForward() {
- TrackingStruct source = sourceTracking();
- source.set(DELETED_POSITIONS_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2}));
Review Comment:
After the relocation, constructor is used instead of sourceTracking() +
setter functions
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -104,6 +105,31 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
+ @VisibleForTesting
+ TrackingStruct(
Review Comment:
I wasn't sure about the set+position approach, hence I introduced the
constructor for tests. I dropped it now and went with the approach you
suggested. Thanks!
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -212,274 +257,23 @@ void testInheritFromNullIsNoOp() {
}
private static Tracking createManifestTracking(long snapshotId, long
sequenceNumber) {
- TrackingStruct tracking = new TrackingStruct(Tracking.schema());
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, snapshotId);
- tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, sequenceNumber);
- tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, sequenceNumber);
- return tracking;
- }
-
- @Test
- void testAddedWithSameCommitDvStaysAdded() {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- assertThat(tracking.status()).isEqualTo(EntryStatus.ADDED);
- assertThat(tracking.snapshotId()).isEqualTo(42L);
- assertThat(tracking.dvSnapshotId()).isEqualTo(42L);
- assertThat(tracking.deletedPositions()).isNull();
- assertThat(tracking.replacedPositions()).isNull();
- // sequence numbers and firstRowId remain null; populated by inheritance
- assertThat(tracking.dataSequenceNumber()).isNull();
- assertThat(tracking.fileSequenceNumber()).isNull();
- assertThat(tracking.firstRowId()).isNull();
- }
-
- @Test
- void testExistingBuilderPreservesSourceFields() {
- Tracking source = sourceTracking();
-
- Tracking existing = TrackingBuilder.from(source, 1L).build();
-
- assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
- assertThat(existing.snapshotId()).isEqualTo(source.snapshotId());
-
assertThat(existing.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(existing.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(existing.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(existing.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testDeleteUpdatesSnapshotIdAndPreservesRest() {
- Tracking source = sourceTracking();
-
- Tracking deleted = TrackingBuilder.deleted(source, 999L);
-
- assertThat(deleted.status()).isEqualTo(EntryStatus.DELETED);
- assertThat(deleted.snapshotId()).isEqualTo(999L);
-
assertThat(deleted.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(deleted.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(deleted.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(deleted.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testReplaceUpdatesSnapshotIdAndPreservesRest() {
- Tracking source = sourceTracking();
-
- Tracking replaced = TrackingBuilder.replaced(source, 999L);
-
- assertThat(replaced.status()).isEqualTo(EntryStatus.REPLACED);
- assertThat(replaced.snapshotId()).isEqualTo(999L);
-
assertThat(replaced.dataSequenceNumber()).isEqualTo(source.dataSequenceNumber());
-
assertThat(replaced.fileSequenceNumber()).isEqualTo(source.fileSequenceNumber());
- assertThat(replaced.dvSnapshotId()).isEqualTo(source.dvSnapshotId());
- assertThat(replaced.firstRowId()).isEqualTo(source.firstRowId());
- }
-
- @Test
- void testSourceDvPositionsAreNotCarriedForward() {
- TrackingStruct source = sourceTracking();
- source.set(DELETED_POSITIONS_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2}));
- source.set(REPLACED_POSITIONS_ORDINAL, ByteBuffer.wrap(new byte[] {3, 4}));
-
- Tracking existing = TrackingBuilder.from(source, 1L).build();
- assertThat(existing.deletedPositions()).isNull();
- assertThat(existing.replacedPositions()).isNull();
-
- Tracking deleted = TrackingBuilder.deleted(source, 999L);
- assertThat(deleted.deletedPositions()).isNull();
- assertThat(deleted.replacedPositions()).isNull();
-
- Tracking replaced = TrackingBuilder.replaced(source, 999L);
- assertThat(replaced.deletedPositions()).isNull();
- assertThat(replaced.replacedPositions()).isNull();
- }
-
- @Test
- void testDvUpdatedProducesModifiedAndAdvancesDvSnapshotId() {
- Tracking source = sourceTracking();
- Tracking modified = TrackingBuilder.from(source, 999L).dvUpdated().build();
-
- assertThat(modified.status()).isEqualTo(EntryStatus.MODIFIED);
- // the entry snapshot id is preserved so we still know when the base file
was added
-
assertThat(modified.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
- // only the DV snapshot id advances to the commit snapshot
- assertThat(modified.dvSnapshotId()).isEqualTo(999L);
- }
-
- @Test
- void testManifestDVMutatorsRejectedOnAdded() {
- assertThatThrownBy(
- () ->
TrackingBuilder.added(42L).deletedPositions(ByteBuffer.wrap(new byte[] {1})))
- .isInstanceOf(IllegalStateException.class)
- .hasMessage("Cannot set deleted positions on ADDED entry");
-
- assertThatThrownBy(
- () ->
TrackingBuilder.added(42L).replacedPositions(ByteBuffer.wrap(new byte[] {1})))
- .isInstanceOf(IllegalStateException.class)
- .hasMessage("Cannot set replaced positions on ADDED entry");
- }
-
- @Test
- void testDvUpdatedRejectedWhenManifestPositionsSet() {
- assertThatThrownBy(
- () ->
- TrackingBuilder.from(manifestSourceTracking(), 999L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1}))
- .dvUpdated())
- .isInstanceOf(IllegalStateException.class)
- .hasMessage(
- "Cannot mark DV updated on a manifest entry (deleted/replaced
positions are set)");
-
- assertThatThrownBy(
- () ->
- TrackingBuilder.from(manifestSourceTracking(), 999L)
- .replacedPositions(ByteBuffer.wrap(new byte[] {1}))
- .dvUpdated())
- .isInstanceOf(IllegalStateException.class)
- .hasMessage(
- "Cannot mark DV updated on a manifest entry (deleted/replaced
positions are set)");
- }
-
- @Test
- void testBuilderRejectsNullSource() {
- assertThatThrownBy(() -> TrackingBuilder.from(null, 1L))
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid source tracking: null");
- }
-
- @Test
- void testSourceBuildersRejectSourceWithoutSequenceNumbers() {
- Tracking missingBoth = TrackingBuilder.added(42L).build();
-
- assertThatThrownBy(() -> TrackingBuilder.from(missingBoth, 1L))
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid tracking source: data sequence number is null");
-
- assertThatThrownBy(() -> TrackingBuilder.deleted(missingBoth, 1L))
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid tracking source: data sequence number is null");
-
- assertThatThrownBy(() -> TrackingBuilder.replaced(missingBoth, 1L))
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid tracking source: data sequence number is null");
-
- TrackingStruct missingFileSeq = new TrackingStruct(Tracking.schema());
- missingFileSeq.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
Review Comment:
After the relocation, constructor is used instead of setter fields
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -24,61 +24,122 @@
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.List;
-import java.util.function.Consumer;
-import java.util.stream.Stream;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
class TestTrackingStruct {
-
- // Ordinals looked up from Tracking.schema() so tests don't hard-code
positions.
- private static final List<Types.NestedField> TRACKING_FIELDS =
Tracking.schema().fields();
- private static final int STATUS_ORDINAL =
TRACKING_FIELDS.indexOf(Tracking.STATUS);
- private static final int SNAPSHOT_ID_ORDINAL =
TRACKING_FIELDS.indexOf(Tracking.SNAPSHOT_ID);
- private static final int DATA_SEQUENCE_NUMBER_ORDINAL =
- TRACKING_FIELDS.indexOf(Tracking.SEQUENCE_NUMBER);
- private static final int FILE_SEQUENCE_NUMBER_ORDINAL =
- TRACKING_FIELDS.indexOf(Tracking.FILE_SEQUENCE_NUMBER);
- private static final int DV_SNAPSHOT_ID_ORDINAL =
- TRACKING_FIELDS.indexOf(Tracking.DV_SNAPSHOT_ID);
- private static final int FIRST_ROW_ID_ORDINAL =
TRACKING_FIELDS.indexOf(Tracking.FIRST_ROW_ID);
- private static final int DELETED_POSITIONS_ORDINAL =
- TRACKING_FIELDS.indexOf(Tracking.DELETED_POSITIONS);
- private static final int REPLACED_POSITIONS_ORDINAL =
- TRACKING_FIELDS.indexOf(Tracking.REPLACED_POSITIONS);
+ private static final byte[] DELETED_POSITIONS = new byte[] {1, 2};
+ private static final byte[] REPLACED_POSITIONS = new byte[] {3, 4};
@Test
void testFieldAccess() {
- TrackingStruct tracking = new TrackingStruct(Tracking.schema());
+ TrackingStruct tracking =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ 42L,
+ 10L,
+ 11L,
+ 43L,
+ 1000L,
+ DELETED_POSITIONS,
+ REPLACED_POSITIONS,
+ "manifest-location",
+ 7L);
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, 42L);
- tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, 10L);
- tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, 11L);
- tracking.set(DV_SNAPSHOT_ID_ORDINAL, 43L);
- tracking.set(FIRST_ROW_ID_ORDINAL, 1000L);
+ assertThat(tracking.status()).isEqualTo(EntryStatus.ADDED);
+ assertThat(tracking.snapshotId()).isEqualTo(42L);
+ assertThat(tracking.dataSequenceNumber()).isEqualTo(10L);
+ assertThat(tracking.fileSequenceNumber()).isEqualTo(11L);
+ assertThat(tracking.dvSnapshotId()).isEqualTo(43L);
+ assertThat(tracking.firstRowId()).isEqualTo(1000L);
+
assertThat(tracking.deletedPositions()).isEqualTo(ByteBuffer.wrap(DELETED_POSITIONS));
+
assertThat(tracking.replacedPositions()).isEqualTo(ByteBuffer.wrap(REPLACED_POSITIONS));
+ assertThat(tracking.manifestLocation()).isEqualTo("manifest-location");
+ assertThat(tracking.manifestPos()).isEqualTo(7L);
+ }
+
+ @Test
+ void testSetByPosition() {
+ TrackingStruct tracking = new TrackingStruct();
+
+ tracking.set(ordinalByFieldName("status"), EntryStatus.ADDED.id());
Review Comment:
agree, renamed to pos
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -0,0 +1,286 @@
+/*
+ * 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 static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.ByteBuffer;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestTrackingBuilder {
+
+ @Test
+ void testAddedWithSameCommitDvStaysAdded() {
Review Comment:
In fact, there is a minor cleanup there: The usage of 'empty constructor +
setter functions by ordinals' is replaced by the usage of the constructor. I
left comments at each occurrences of these changes.
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -23,14 +23,15 @@
import java.util.Arrays;
import java.util.Objects;
import org.apache.iceberg.avro.SupportsIndexProjection;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.ByteBuffers;
/** Mutable {@link StructLike} implementation of {@link Tracking}. */
class TrackingStruct extends SupportsIndexProjection implements Tracking,
Serializable {
- private static final Types.StructType BASE_TYPE =
+ static final Types.StructType BASE_TYPE =
Review Comment:
The reason I used this one in the tests is that it contains the manifest
position while `Tracking.schema()` doesn't. I'll revert this to private, and
use a hard-coded ordinal for setting manifest position in tests.
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -514,84 +308,44 @@ void testProjectedStructLike() {
assertThat(tracking.get(1, Long.class)).isEqualTo(1000L);
}
- @Test
- void testJavaSerializationRoundTripForDataFile() throws IOException,
ClassNotFoundException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
- assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testJavaSerializationRoundTripForManifest() throws IOException,
ClassNotFoundException {
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void testSerializationRoundTrip(TestHelpers.RoundTripSerializer<Tracking>
roundTripSerializer)
+ throws IOException, ClassNotFoundException {
Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
+ new TrackingStruct(
+ EntryStatus.MODIFIED,
+ 42L,
+ 10L,
+ 11L,
+ 43L,
+ 1000L,
+ DELETED_POSITIONS,
+ REPLACED_POSITIONS,
+ "manifest-location",
+ 7L);
+
+ Tracking deserialized = roundTripSerializer.apply(tracking);
assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- @Test
- void testKryoSerializationRoundTripForDataFile() throws IOException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testKryoSerializationRoundTripForManifest() throws IOException {
- Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- private static TrackingStruct sourceTracking() {
- TrackingStruct tracking = new TrackingStruct(Tracking.schema());
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, 42L);
- tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, 10L);
- tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, 10L);
- tracking.set(DV_SNAPSHOT_ID_ORDINAL, 43L);
- tracking.set(FIRST_ROW_ID_ORDINAL, 1000L);
- return tracking;
- }
-
- private static TrackingStruct sourceTrackingWithStatus(EntryStatus status) {
- TrackingStruct tracking = sourceTracking();
- tracking.set(STATUS_ORDINAL, status.id());
- return tracking;
- }
-
- private static TrackingStruct manifestSourceTracking() {
- TrackingStruct tracking = sourceTracking();
- tracking.set(DV_SNAPSHOT_ID_ORDINAL, null);
- return tracking;
+ assertThat(deserialized.dataSequenceNumber()).isEqualTo(10L);
+ assertThat(deserialized.fileSequenceNumber()).isEqualTo(11L);
+ assertThat(deserialized.dvSnapshotId()).isEqualTo(43L);
+ assertThat(deserialized.firstRowId()).isEqualTo(1000L);
+
assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(DELETED_POSITIONS));
+
assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(REPLACED_POSITIONS));
+ assertThat(deserialized.manifestLocation()).isEqualTo("manifest-location");
+ assertThat(deserialized.manifestPos()).isEqualTo(7L);
+ }
+
+ private static int ordinalByFieldName(String fieldName) {
+ List<Types.NestedField> fields = TrackingStruct.BASE_TYPE.fields();
Review Comment:
I introduced a constant for the position of _pos. Changed this to use
`Tracking.schema()` and left a comment that this can't handle _pos
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -0,0 +1,286 @@
+/*
+ * 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 static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.ByteBuffer;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestTrackingBuilder {
+
+ @Test
+ void testAddedWithSameCommitDvStaysAdded() {
Review Comment:
Yes, these are moved, but not changed. Sorry, should've left a comment.
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -514,84 +308,44 @@ void testProjectedStructLike() {
assertThat(tracking.get(1, Long.class)).isEqualTo(1000L);
}
- @Test
- void testJavaSerializationRoundTripForDataFile() throws IOException,
ClassNotFoundException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
- assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testJavaSerializationRoundTripForManifest() throws IOException,
ClassNotFoundException {
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void testSerializationRoundTrip(TestHelpers.RoundTripSerializer<Tracking>
roundTripSerializer)
+ throws IOException, ClassNotFoundException {
Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
+ new TrackingStruct(
+ EntryStatus.MODIFIED,
+ 42L,
+ 10L,
+ 11L,
+ 43L,
+ 1000L,
+ DELETED_POSITIONS,
+ REPLACED_POSITIONS,
+ "manifest-location",
+ 7L);
+
+ Tracking deserialized = roundTripSerializer.apply(tracking);
assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- @Test
- void testKryoSerializationRoundTripForDataFile() throws IOException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testKryoSerializationRoundTripForManifest() throws IOException {
- Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- private static TrackingStruct sourceTracking() {
- TrackingStruct tracking = new TrackingStruct(Tracking.schema());
- tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
- tracking.set(SNAPSHOT_ID_ORDINAL, 42L);
- tracking.set(DATA_SEQUENCE_NUMBER_ORDINAL, 10L);
- tracking.set(FILE_SEQUENCE_NUMBER_ORDINAL, 10L);
- tracking.set(DV_SNAPSHOT_ID_ORDINAL, 43L);
- tracking.set(FIRST_ROW_ID_ORDINAL, 1000L);
- return tracking;
- }
-
- private static TrackingStruct sourceTrackingWithStatus(EntryStatus status) {
- TrackingStruct tracking = sourceTracking();
- tracking.set(STATUS_ORDINAL, status.id());
- return tracking;
- }
-
- private static TrackingStruct manifestSourceTracking() {
- TrackingStruct tracking = sourceTracking();
- tracking.set(DV_SNAPSHOT_ID_ORDINAL, null);
- return tracking;
+ assertThat(deserialized.dataSequenceNumber()).isEqualTo(10L);
+ assertThat(deserialized.fileSequenceNumber()).isEqualTo(11L);
+ assertThat(deserialized.dvSnapshotId()).isEqualTo(43L);
+ assertThat(deserialized.firstRowId()).isEqualTo(1000L);
+
assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(DELETED_POSITIONS));
+
assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(REPLACED_POSITIONS));
+ assertThat(deserialized.manifestLocation()).isEqualTo("manifest-location");
+ assertThat(deserialized.manifestPos()).isEqualTo(7L);
+ }
+
+ private static int ordinalByFieldName(String fieldName) {
+ List<Types.NestedField> fields = TrackingStruct.BASE_TYPE.fields();
+ for (int i = 0; i < fields.size(); i++) {
+ if (fields.get(i).name().equals(fieldName)) {
+ return i;
+ }
+ }
+ throw new IllegalArgumentException("Field not found in schema: " +
fieldName);
Review Comment:
Done
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -514,84 +308,44 @@ void testProjectedStructLike() {
assertThat(tracking.get(1, Long.class)).isEqualTo(1000L);
}
- @Test
- void testJavaSerializationRoundTripForDataFile() throws IOException,
ClassNotFoundException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
- assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testJavaSerializationRoundTripForManifest() throws IOException,
ClassNotFoundException {
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void testSerializationRoundTrip(TestHelpers.RoundTripSerializer<Tracking>
roundTripSerializer)
+ throws IOException, ClassNotFoundException {
Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized = TestHelpers.roundTripSerialize(tracking);
+ new TrackingStruct(
+ EntryStatus.MODIFIED,
+ 42L,
+ 10L,
+ 11L,
+ 43L,
+ 1000L,
+ DELETED_POSITIONS,
+ REPLACED_POSITIONS,
+ "manifest-location",
+ 7L);
+
+ Tracking deserialized = roundTripSerializer.apply(tracking);
assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- @Test
- void testKryoSerializationRoundTripForDataFile() throws IOException {
- Tracking tracking = TrackingBuilder.added(42L).dvUpdated().build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.snapshotId()).isEqualTo(42L);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletedPositions()).isNull();
- assertThat(deserialized.replacedPositions()).isNull();
- }
-
- @Test
- void testKryoSerializationRoundTripForManifest() throws IOException {
- Tracking tracking =
- TrackingBuilder.from(manifestSourceTracking(), 1L)
- .deletedPositions(ByteBuffer.wrap(new byte[] {1, 2}))
- .replacedPositions(ByteBuffer.wrap(new byte[] {3, 4}))
- .build();
-
- Tracking deserialized =
TestHelpers.KryoHelpers.roundTripSerialize(tracking);
-
- assertThat(deserialized.status()).isEqualTo(EntryStatus.MODIFIED);
- assertThat(deserialized.dvSnapshotId()).isEqualTo(1L);
- assertThat(deserialized.deletedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {1, 2}));
- assertThat(deserialized.replacedPositions()).isEqualTo(ByteBuffer.wrap(new
byte[] {3, 4}));
- }
-
- private static TrackingStruct sourceTracking() {
Review Comment:
After the relocation, these 3 helper functions use the constructor instead
of the setter functions
--
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]
