stevenzwu commented on code in PR #16408:
URL: https://github.com/apache/iceberg/pull/16408#discussion_r3350943053
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -166,28 +192,271 @@ void testInheritFromRejectsUnequalSequenceNumbers() {
@Test
void testNoDefaultingWithoutInheritance() {
- TrackingStruct tracking =
TrackingStruct.builder().status(EntryStatus.ADDED).build();
+ TrackingStruct tracking = new TrackingStruct(Tracking.schema());
+ tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
// no inheritance, nulls stay null
assertThat(tracking.snapshotId()).isNull();
assertThat(tracking.dataSequenceNumber()).isNull();
assertThat(tracking.fileSequenceNumber()).isNull();
}
+ @Test
+ void testInheritFromNullIsNoOp() {
+ TrackingStruct tracking = new TrackingStruct(Tracking.schema());
+ tracking.set(STATUS_ORDINAL, EntryStatus.ADDED.id());
+
+ tracking.inheritFrom(null);
+
+ // null source is a no-op; all unset fields stay null
+ assertThat(tracking.snapshotId()).isNull();
+ assertThat(tracking.dataSequenceNumber()).isNull();
+ assertThat(tracking.fileSequenceNumber()).isNull();
+ }
+
private static Tracking createManifestTracking(long snapshotId, long
sequenceNumber) {
- return TrackingStruct.builder()
- .status(EntryStatus.ADDED)
- .snapshotId(snapshotId)
- .dataSequenceNumber(sequenceNumber)
- .fileSequenceNumber(sequenceNumber)
- .build();
+ 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 testBuilderValidation() {
- assertThatThrownBy(() -> TrackingStruct.builder().build())
+ void testAddedBuilder() {
+ 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 testExistingBuilderAllowsDvMutation() {
+ Tracking existing = TrackingBuilder.from(sourceTracking(),
999L).dvUpdated().build();
+ assertThat(existing.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 testDvSnapshotIdAndManifestDVPositionsAreMutuallyExclusive() {
+ // sourceTracking has dvSnapshotId=43, inherited by existing(source)
+ assertThatThrownBy(
+ () ->
+ TrackingBuilder.from(sourceTracking(), 1L)
+ .deletedPositions(ByteBuffer.wrap(new byte[] {1})))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage("Cannot set deleted positions on a data file entry (DV
snapshot ID is set)");
+
+ assertThatThrownBy(
+ () ->
+ TrackingBuilder.from(sourceTracking(), 1L)
+ .replacedPositions(ByteBuffer.wrap(new byte[] {1})))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage("Cannot set replaced positions on a data file entry (DV
snapshot ID is set)");
+
+ // Setting MDV positions first then dvUpdated is also rejected
+ assertThatThrownBy(
+ () ->
+ TrackingBuilder.from(manifestSourceTracking(), 1L)
+ .deletedPositions(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 status: null");
+ .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());
+ missingFileSeq.set(SNAPSHOT_ID_ORDINAL, 42L);
+ missingFileSeq.set(DATA_SEQUENCE_NUMBER_ORDINAL, 10L);
+
+ assertThatThrownBy(() -> TrackingBuilder.from(missingFileSeq, 1L))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid tracking source: file sequence number is null");
+
+ assertThatThrownBy(() -> TrackingBuilder.deleted(missingFileSeq, 1L))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid tracking source: file sequence number is null");
+
+ assertThatThrownBy(() -> TrackingBuilder.replaced(missingFileSeq, 1L))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid tracking source: file sequence number is null");
+ }
+
+ private static Stream<Arguments> terminalTransitionCases() {
+ Consumer<Tracking> builderCall = source -> TrackingBuilder.from(source,
1L);
+ Consumer<Tracking> deletedCall = source -> TrackingBuilder.deleted(source,
1L);
+ Consumer<Tracking> replacedCall = source ->
TrackingBuilder.replaced(source, 1L);
+ return Stream.of(
+ Arguments.of(EntryStatus.DELETED, builderCall),
+ Arguments.of(EntryStatus.DELETED, deletedCall),
+ Arguments.of(EntryStatus.DELETED, replacedCall),
+ Arguments.of(EntryStatus.REPLACED, builderCall),
+ Arguments.of(EntryStatus.REPLACED, deletedCall),
+ Arguments.of(EntryStatus.REPLACED, replacedCall));
+ }
+
+ @ParameterizedTest
+ @MethodSource("terminalTransitionCases")
+ void testRejectsTransitionsFromTerminalStatus(
+ EntryStatus sourceStatus, Consumer<Tracking> factoryCall) {
+ Tracking source = sourceTrackingWithStatus(sourceStatus);
+ assertThatThrownBy(() -> factoryCall.accept(source))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage("Cannot revive non-live entry with status " +
sourceStatus);
+ }
+
+ @Test
+ void testExistingToExistingIsAllowed() {
+ Tracking existingSource = sourceTrackingWithStatus(EntryStatus.EXISTING);
+
+ Tracking existing = TrackingBuilder.from(existingSource, 1L).build();
+
+ assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
+ assertThat(existing.snapshotId()).isEqualTo(existingSource.snapshotId());
+ }
+
+ @Test
+ void testExistingToTerminalTransitions() {
+ Tracking existingSource = sourceTrackingWithStatus(EntryStatus.EXISTING);
+
+ Tracking deleted = TrackingBuilder.deleted(existingSource, 999L);
+ assertThat(deleted.status()).isEqualTo(EntryStatus.DELETED);
+ assertThat(deleted.snapshotId()).isEqualTo(999L);
+
+ Tracking replaced = TrackingBuilder.replaced(existingSource, 999L);
+ assertThat(replaced.status()).isEqualTo(EntryStatus.REPLACED);
+ assertThat(replaced.snapshotId()).isEqualTo(999L);
+ }
+
+ @Test
+ void testInternalSetIgnoresUnknownOrdinal() {
+ TrackingStruct tracking = new TrackingStruct(Tracking.schema());
Review Comment:
I feel current code is fine since it tests internalSet anyway. I am going to
merge this PR. we can follow up on this later if needed
--
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]
