gaborkaszab commented on code in PR #17145:
URL: https://github.com/apache/iceberg/pull/17145#discussion_r3586875029
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -48,44 +55,38 @@ void testAddedWithSameCommitDvStaysAdded() {
@Test
void testExistingBuilderPreservesSourceFields() {
- Tracking source = sourceTracking();
-
- Tracking existing = TrackingBuilder.from(source, 1L).build();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_ADDED,
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());
+
assertThat(existing.snapshotId()).isEqualTo(SOURCE_TRACKING_ADDED.snapshotId());
Review Comment:
Isn't that redundant check? We have an equality check with
`SOURCE_TRACKING_ADDED.snapshotId()` that is 42L, even if not spelled
explicitly in the tests, I don't find it useful to check that it's not 1L.
##########
core/src/test/java/org/apache/iceberg/TestTrackingBuilder.java:
##########
@@ -209,78 +211,70 @@ private static Stream<Arguments>
terminalTransitionCases() {
@MethodSource("terminalTransitionCases")
void testRejectsTransitionsFromTerminalStatus(
EntryStatus sourceStatus, Consumer<Tracking> factoryCall) {
- Tracking source = sourceTrackingWithStatus(sourceStatus);
+ Tracking source = new TrackingStruct(sourceStatus, 42L, 10L, 10L, 43L,
1000L, null, null);
+
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();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_EXISTING,
1L).build();
assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
- assertThat(existing.snapshotId()).isEqualTo(existingSource.snapshotId());
+
assertThat(existing.snapshotId()).isEqualTo(SOURCE_TRACKING_EXISTING.snapshotId());
}
@Test
void testExistingToTerminalTransitions() {
- Tracking existingSource = sourceTrackingWithStatus(EntryStatus.EXISTING);
-
- Tracking deleted = TrackingBuilder.deleted(existingSource, 999L);
+ Tracking deleted = TrackingBuilder.deleted(SOURCE_TRACKING_EXISTING, 999L);
assertThat(deleted.status()).isEqualTo(EntryStatus.DELETED);
assertThat(deleted.snapshotId()).isEqualTo(999L);
- Tracking replaced = TrackingBuilder.replaced(existingSource, 999L);
+ Tracking replaced = TrackingBuilder.replaced(SOURCE_TRACKING_EXISTING,
999L);
assertThat(replaced.status()).isEqualTo(EntryStatus.REPLACED);
assertThat(replaced.snapshotId()).isEqualTo(999L);
}
@Test
void testExistingPreservesSourceSnapshotId() {
- Tracking source = sourceTracking();
- Tracking existing = TrackingBuilder.from(source, 999L).build();
+ Tracking existing = TrackingBuilder.from(SOURCE_TRACKING_ADDED,
999L).build();
assertThat(existing.status()).isEqualTo(EntryStatus.EXISTING);
-
assertThat(existing.snapshotId()).isEqualTo(source.snapshotId()).isNotEqualTo(999L);
+ assertThat(existing.snapshotId())
+ .isEqualTo(SOURCE_TRACKING_ADDED.snapshotId())
+ .isNotEqualTo(999L);
}
@Test
void testCarryForwardFromModifiedSourceChangesToExisting() {
- Tracking modifiedSource = sourceTrackingWithStatus(EntryStatus.MODIFIED);
- Tracking carried = TrackingBuilder.from(modifiedSource, 999L).build();
+ Tracking carried = TrackingBuilder.from(SOURCE_TRACKING_MODIFIED,
999L).build();
assertThat(carried.status()).isEqualTo(EntryStatus.EXISTING);
-
assertThat(carried.snapshotId()).isEqualTo(modifiedSource.snapshotId()).isNotEqualTo(999L);
-
assertThat(carried.dvSnapshotId()).isEqualTo(modifiedSource.dvSnapshotId()).isNotEqualTo(999L);
-
assertThat(carried.dataSequenceNumber()).isEqualTo(modifiedSource.dataSequenceNumber());
-
assertThat(carried.fileSequenceNumber()).isEqualTo(modifiedSource.fileSequenceNumber());
- assertThat(carried.firstRowId()).isEqualTo(modifiedSource.firstRowId());
+ assertThat(carried.snapshotId())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.snapshotId())
+ .isNotEqualTo(999L);
+ assertThat(carried.dvSnapshotId())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.dvSnapshotId())
+ .isNotEqualTo(999L);
+ assertThat(carried.dataSequenceNumber())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.dataSequenceNumber());
+ assertThat(carried.fileSequenceNumber())
+ .isEqualTo(SOURCE_TRACKING_MODIFIED.fileSequenceNumber());
+
assertThat(carried.firstRowId()).isEqualTo(SOURCE_TRACKING_MODIFIED.firstRowId());
}
@Test
void testManifestDVPositionsProduceModified() {
ByteBuffer deletedBytes = ByteBuffer.wrap(new byte[] {1, 2});
- Tracking addedSource = manifestSourceTracking();
Tracking modified =
- TrackingBuilder.from(addedSource,
999L).deletedPositions(deletedBytes).build();
+ TrackingBuilder.from(SOURCE_TRACKING_ADDED,
999L).deletedPositions(deletedBytes).build();
Review Comment:
Yes, the "manifest tracking" we use for source is slightly different because
we set `dvSnapshotId` now, but it doesn't make any difference for these tests.
I went for cleaner test setup 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]
