Skip to content

Commit b04a87b

Browse files
lmcreanGoogle Java Core Libraries
authored andcommitted
Fix Iterators.mergeSorted() to preserve stability for equal elements.
Fixes #7989 Fixes #5773 RELNOTES=`collect`: Improved `Iterators.mergeSorted()` to preserve stability for equal elements. PiperOrigin-RevId: 806971295
1 parent 84aca22 commit b04a87b

File tree

4 files changed

+66
-24
lines changed

4 files changed

+66
-24
lines changed

android/guava-tests/test/com/google/common/collect/IteratorsTest.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,7 +1568,8 @@ public void testMergeSorted_stable_issue5773Example() {
15681568
new TestDatum("A", 2),
15691569
new TestDatum("B", 1),
15701570
new TestDatum("C", 1),
1571-
new TestDatum("C", 2));
1571+
new TestDatum("C", 2))
1572+
.inOrder();
15721573
}
15731574

15741575
public void testMergeSorted_stable_allEqual() {
@@ -1587,7 +1588,8 @@ public void testMergeSorted_stable_allEqual() {
15871588
new TestDatum("A", 1),
15881589
new TestDatum("A", 2),
15891590
new TestDatum("A", 3),
1590-
new TestDatum("A", 4));
1591+
new TestDatum("A", 4))
1592+
.inOrder();
15911593
}
15921594

15931595
private static final class TestDatum {

android/guava/src/com/google/common/collect/Iterators.java

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,8 +1294,9 @@ public E peek() {
12941294
* <p>Callers must ensure that the source {@code iterators} are in non-descending order as this
12951295
* method does not sort its input.
12961296
*
1297-
* <p>For any equivalent elements across all {@code iterators}, it is undefined which element is
1298-
* returned first.
1297+
* <p>For any equivalent elements across all {@code iterators}, elements are returned in the order
1298+
* of their source iterators. That is, if element A from iterator 1 and element B from iterator 2
1299+
* compare as equal, A will be returned before B if iterator 1 was passed before iterator 2.
12991300
*
13001301
* @since 11.0
13011302
*/
@@ -1318,21 +1319,38 @@ public E peek() {
13181319
*/
13191320
private static final class MergingIterator<T extends @Nullable Object>
13201321
extends UnmodifiableIterator<T> {
1321-
final Queue<PeekingIterator<T>> queue;
1322+
1323+
// Wrapper class to track insertion order for stable sorting
1324+
private static class IndexedIterator<E extends @Nullable Object> {
1325+
final PeekingIterator<E> iterator;
1326+
final int index;
1327+
1328+
IndexedIterator(PeekingIterator<E> iterator, int index) {
1329+
this.iterator = iterator;
1330+
this.index = index;
1331+
}
1332+
}
1333+
1334+
final Queue<IndexedIterator<T>> queue;
13221335

13231336
MergingIterator(
13241337
Iterable<? extends Iterator<? extends T>> iterators, Comparator<? super T> itemComparator) {
13251338
// A comparator that's used by the heap, allowing the heap
1326-
// to be sorted based on the top of each iterator.
1327-
Comparator<PeekingIterator<T>> heapComparator =
1328-
(PeekingIterator<T> o1, PeekingIterator<T> o2) ->
1329-
itemComparator.compare(o1.peek(), o2.peek());
1339+
// to be sorted based on the top of each iterator, with insertion order as tiebreaker
1340+
Comparator<IndexedIterator<T>> heapComparator =
1341+
(o1, o2) ->
1342+
ComparisonChain.start()
1343+
.compare(o1.iterator.peek(), o2.iterator.peek(), itemComparator)
1344+
// When elements are equal, use insertion order to maintain stability
1345+
.compare(o1.index, o2.index)
1346+
.result();
13301347

13311348
queue = new PriorityQueue<>(2, heapComparator);
13321349

1350+
int index = 0;
13331351
for (Iterator<? extends T> iterator : iterators) {
13341352
if (iterator.hasNext()) {
1335-
queue.add(Iterators.peekingIterator(iterator));
1353+
queue.add(new IndexedIterator<>(peekingIterator(iterator), index++));
13361354
}
13371355
}
13381356
}
@@ -1345,10 +1363,11 @@ public boolean hasNext() {
13451363
@Override
13461364
@ParametricNullness
13471365
public T next() {
1348-
PeekingIterator<T> nextIter = queue.remove();
1366+
IndexedIterator<T> nextIndexed = queue.remove();
1367+
PeekingIterator<T> nextIter = nextIndexed.iterator;
13491368
T next = nextIter.next();
13501369
if (nextIter.hasNext()) {
1351-
queue.add(nextIter);
1370+
queue.add(nextIndexed);
13521371
}
13531372
return next;
13541373
}

guava-tests/test/com/google/common/collect/IteratorsTest.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,7 +1568,8 @@ public void testMergeSorted_stable_issue5773Example() {
15681568
new TestDatum("A", 2),
15691569
new TestDatum("B", 1),
15701570
new TestDatum("C", 1),
1571-
new TestDatum("C", 2));
1571+
new TestDatum("C", 2))
1572+
.inOrder();
15721573
}
15731574

15741575
public void testMergeSorted_stable_allEqual() {
@@ -1587,7 +1588,8 @@ public void testMergeSorted_stable_allEqual() {
15871588
new TestDatum("A", 1),
15881589
new TestDatum("A", 2),
15891590
new TestDatum("A", 3),
1590-
new TestDatum("A", 4));
1591+
new TestDatum("A", 4))
1592+
.inOrder();
15911593
}
15921594

15931595
private static final class TestDatum {

guava/src/com/google/common/collect/Iterators.java

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,8 +1294,9 @@ public E peek() {
12941294
* <p>Callers must ensure that the source {@code iterators} are in non-descending order as this
12951295
* method does not sort its input.
12961296
*
1297-
* <p>For any equivalent elements across all {@code iterators}, it is undefined which element is
1298-
* returned first.
1297+
* <p>For any equivalent elements across all {@code iterators}, elements are returned in the order
1298+
* of their source iterators. That is, if element A from iterator 1 and element B from iterator 2
1299+
* compare as equal, A will be returned before B if iterator 1 was passed before iterator 2.
12991300
*
13001301
* @since 11.0
13011302
*/
@@ -1318,21 +1319,38 @@ public E peek() {
13181319
*/
13191320
private static final class MergingIterator<T extends @Nullable Object>
13201321
extends UnmodifiableIterator<T> {
1321-
final Queue<PeekingIterator<T>> queue;
1322+
1323+
// Wrapper class to track insertion order for stable sorting
1324+
private static class IndexedIterator<E extends @Nullable Object> {
1325+
final PeekingIterator<E> iterator;
1326+
final int index;
1327+
1328+
IndexedIterator(PeekingIterator<E> iterator, int index) {
1329+
this.iterator = iterator;
1330+
this.index = index;
1331+
}
1332+
}
1333+
1334+
final Queue<IndexedIterator<T>> queue;
13221335

13231336
MergingIterator(
13241337
Iterable<? extends Iterator<? extends T>> iterators, Comparator<? super T> itemComparator) {
13251338
// A comparator that's used by the heap, allowing the heap
1326-
// to be sorted based on the top of each iterator.
1327-
Comparator<PeekingIterator<T>> heapComparator =
1328-
(PeekingIterator<T> o1, PeekingIterator<T> o2) ->
1329-
itemComparator.compare(o1.peek(), o2.peek());
1339+
// to be sorted based on the top of each iterator, with insertion order as tiebreaker
1340+
Comparator<IndexedIterator<T>> heapComparator =
1341+
(o1, o2) ->
1342+
ComparisonChain.start()
1343+
.compare(o1.iterator.peek(), o2.iterator.peek(), itemComparator)
1344+
// When elements are equal, use insertion order to maintain stability
1345+
.compare(o1.index, o2.index)
1346+
.result();
13301347

13311348
queue = new PriorityQueue<>(2, heapComparator);
13321349

1350+
int index = 0;
13331351
for (Iterator<? extends T> iterator : iterators) {
13341352
if (iterator.hasNext()) {
1335-
queue.add(Iterators.peekingIterator(iterator));
1353+
queue.add(new IndexedIterator<>(peekingIterator(iterator), index++));
13361354
}
13371355
}
13381356
}
@@ -1345,10 +1363,11 @@ public boolean hasNext() {
13451363
@Override
13461364
@ParametricNullness
13471365
public T next() {
1348-
PeekingIterator<T> nextIter = queue.remove();
1366+
IndexedIterator<T> nextIndexed = queue.remove();
1367+
PeekingIterator<T> nextIter = nextIndexed.iterator;
13491368
T next = nextIter.next();
13501369
if (nextIter.hasNext()) {
1351-
queue.add(nextIter);
1370+
queue.add(nextIndexed);
13521371
}
13531372
return next;
13541373
}

0 commit comments

Comments
 (0)