FrankChen021 commented on code in PR #20305:
URL: https://github.com/apache/druid/pull/20305#discussion_r3974767609
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -156,13 +177,47 @@ private static void testMerge(
}
i++;
}
- Assertions.assertFalse(mergingRowIterator.moveToNext(), message);
+ Assertions.assertFalse(mergingRowIterator.moveToNext(), failureMessage);
if (iterated) {
- Assertions.assertEquals(currentTimestamp,
mergingRowIterator.getPointer().timestampSelector.getLong(), message);
+ Assertions.assertEquals(
+ currentTimestamp,
+ mergingRowIterator.getPointer().timestampSelector.getLong(),
+ failureMessage
+ );
}
}
}
+ @SafeVarargs
+ private static void testMergeOrder(List<Long>... timestampSequences)
+ {
+ final Supplier<String> failureMessage
+ = () ->
Stream.of(timestampSequences).map(List::toString).collect(Collectors.joining("
"));
+ try (MergingRowIterator mergingRowIterator = new MergingRowIterator(
+
Stream.of(timestampSequences).map(TestRowIterator::new).collect(Collectors.toList())
+ )) {
+ final Iterator<Long> expectedTimestamps = Utils.mergeSorted(
+
Stream.of(timestampSequences).map(List::iterator).collect(Collectors.toList()),
+ Comparator.naturalOrder()
+ );
+ while (expectedTimestamps.hasNext()) {
+ Assertions.assertTrue(
+ mergingRowIterator.moveToNext(),
+ failureMessage
+ );
+ Assertions.assertEquals(
+ expectedTimestamps.next(),
+ mergingRowIterator.getPointer().timestampSelector.getLong(),
+ failureMessage
+ );
+ }
+ Assertions.assertFalse(
Review Comment:
Addressed in 243d7fc7cc. The order helper now tracks whether it produced a
row and verifies after the unsuccessful moveToNext() call that getPointer()
still references the final expected timestamp.
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -156,13 +177,47 @@ private static void testMerge(
}
i++;
}
- Assertions.assertFalse(mergingRowIterator.moveToNext(), message);
+ Assertions.assertFalse(mergingRowIterator.moveToNext(), failureMessage);
if (iterated) {
- Assertions.assertEquals(currentTimestamp,
mergingRowIterator.getPointer().timestampSelector.getLong(), message);
+ Assertions.assertEquals(
+ currentTimestamp,
+ mergingRowIterator.getPointer().timestampSelector.getLong(),
+ failureMessage
+ );
}
}
}
+ @SafeVarargs
+ private static void testMergeOrder(List<Long>... timestampSequences)
+ {
+ final Supplier<String> failureMessage
+ = () ->
Stream.of(timestampSequences).map(List::toString).collect(Collectors.joining("
"));
+ try (MergingRowIterator mergingRowIterator = new MergingRowIterator(
+
Stream.of(timestampSequences).map(TestRowIterator::new).collect(Collectors.toList())
+ )) {
+ final Iterator<Long> expectedTimestamps = Utils.mergeSorted(
+
Stream.of(timestampSequences).map(List::iterator).collect(Collectors.toList()),
+ Comparator.naturalOrder()
+ );
+ while (expectedTimestamps.hasNext()) {
+ Assertions.assertTrue(
+ mergingRowIterator.moveToNext(),
+ failureMessage
+ );
+ Assertions.assertEquals(
+ expectedTimestamps.next(),
Review Comment:
Addressed in 243d7fc7cc. The expected timestamp is now read into a local
variable before the assertions, so advancing the expected iterator is no longer
a side effect of an assertion argument.
--
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]
