FrankChen021 commented on code in PR #20305:
URL: https://github.com/apache/druid/pull/20305#discussion_r3968277938
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -73,6 +74,21 @@ public void testAllPossible5ElementSequences()
{
List<List<Long>> possibleSequences = new ArrayList<>();
populateSequences(possibleSequences, new ArrayDeque<>(), 1, 6, 5);
+ for (int i1 = 0; i1 < possibleSequences.size(); i1++) {
+ for (int i2 = i1; i2 < possibleSequences.size(); i2++) {
+ for (int i3 = i2; i3 < possibleSequences.size(); i3++) {
+ testMergeOrder(possibleSequences.get(i1), possibleSequences.get(i2),
possibleSequences.get(i3));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void testMarkHandlingAcrossAllPossible4ElementSequences()
+ {
+ // Keep systematic coverage of mark handling across different heap layouts
while limiting its cross-product.
+ List<List<Long>> possibleSequences = new ArrayList<>();
+ populateSequences(possibleSequences, new ArrayDeque<>(), 1, 6, 4);
Review Comment:
[P2] Preserve mark coverage for five-row inputs
Before this split, the exhaustive five-element test invoked `testMerge` for
every sequence triple, so `markIteration` covered output positions that include
a fifth row from one source and the subsequent transitions or exhaustion. This
suite now caps every source at four elements, while the new five-element suite
never calls `mark()`. A regression in `MergingRowIterator` that appears only
after the fifth source row or its late removal from the heap would therefore
pass. Keep targeted five-element mark cases (including a single-source five-row
case) while retaining the reduced cross-product.
##########
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:
[P2] Retain the last-pointer exhaustion assertion
The extracted five-element order test stops after asserting that
`moveToNext()` returns false. The previous `testMerge` also asserted that
`getPointer()` still referenced the last valid row after that unsuccessful
call, which is an explicit `RowIterator` contract used by
`RowCombiningTimeAndDimsIterator`. The only remaining check is in the reduced
four-element mark suite, so a regression on late or five-row exhaustion is no
longer caught here. Track whether the input was non-empty and retain the old
last-pointer assertion after this `assertFalse`.
--
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]
