wombatu-kun commented on code in PR #17145:
URL: https://github.com/apache/iceberg/pull/17145#discussion_r3568987124
##########
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:
> Unrelated, but I frankly find these seemingly LLM-generated, extra
verbose, overly explaining comments distracting. This comment could have been
"We might loose test coverage by dropping the manifest source `Tracking`
because we now have no source that doesn't have a dv. Is that intentional, do
we loose anything here?"
sorry, it won't happen again
--
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]
