Fix duplicate elements left by CollectionUtils.sortAndDedup

[winklemad]

Description

CollectionUtils.sortAndDedup(List, Comparator) can leave duplicate elements in the list. After it retains a new unique value it updates its "last retained value" tracker from the wrong source, so once the input contains two or more consecutive runs of equal values, every run after the first survives.

T cmp = deduped.next(); // meant to hold the last retained value
...
do {
    T old = oldArray.next();
    // cmp is reassigned from the write cursor (deduped.next()), not from the retained value `old`
    if (comparator.compare(cmp, old) != 0 && (cmp = deduped.next()) != old) {
        deduped.set(old);
    }
} while (oldArray.hasNext());

When a new unique old is found, cmp is assigned deduped.next() — the value currently under the write cursor — instead of old, the value actually being kept. cmp then goes stale and the next comparison is made against the wrong value.

Example:

• sortAndDedup([0, 0, 1, 1], naturalOrder()) returns [0, 1, 1] (expected [0, 1])
• sortAndDedup([2, 2, 2, 3, 3], naturalOrder()) returns [2, 3, 3] (expected [2, 3])

It triggers whenever a sorted run of ≥2 equal values is followed by another run of ≥2 equal values.

Impact

BinaryFieldMapper calls this to dedup a document's multi-valued binary doc-values before serialization:

// server/src/main/java/org/opensearch/index/mapper/BinaryFieldMapper.java:294
CollectionUtils.sortAndDedup(bytesList, Arrays::compareUnsigned);

A document whose binary field holds values like {aa, aa, bb, bb} is stored as [aa, bb, bb] — duplicated, oversized stored doc-values.

Root cause / fix

The sibling BytesRefUtils.sortAndDedup uses the standard adjacent previous/current compare and is correct. Track the retained value directly instead of the write-cursor value:

if (comparator.compare(cmp, old) != 0) {
    // a new unique value: advance the write cursor and only copy when it isn't already in place
    if (deduped.next() != old) {
        deduped.set(old);
    }
    cmp = old; // remember the value we just retained
}

This keeps the original in-place write and the reference-equality skip (avoids a redundant set when the value is already in position); it only corrects which value cmp tracks. This is the only ListIterator-based dedup in the class, and BinaryFieldMapper is its only production caller.

Test

Extended CollectionUtilsTests.testSortAndDedup with multiple-consecutive-run cases ([0,0,1,1], [2,2,2,3,3], [0,0,1,1,2,2], runs followed by a trailing unique, unsorted multi-group input, and a reverse comparator). The existing assertDeduped helper already exercises both ArrayList and LinkedList and asserts that adjacent elements differ. The new cases fail before the fix (spurious duplicates survive) and pass after. :libs:opensearch-core precommit and the CollectionUtilsTests suite are green locally.

Related Issues

No linked issue — found by reading the code.

Check List

• Functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b803375)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b803375
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid reference equality for dedup placement check Using reference equality (!=) to decide whether to overwrite may fail for equal-but-non-identical objects that the comparator treats as distinct (e.g., different String instances with the same content but different case under a case-insensitive comparator), causing unnecessary but harmless writes — however it can also skip a required set when references happen to alias across positions. Use a positional check (compare oldArray's index to deduped's index) rather than reference identity to reliably determine whether the write cursor is already at the source position. libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java [126-133] if (comparator.compare(cmp, old) != 0) { - // a new unique value: advance the write cursor and only copy when it isn't already in place - if (deduped.next() != old) { + // a new unique value: advance the write cursor and copy only when it isn't already in place + int writeIdx = deduped.nextIndex(); + deduped.next(); + if (writeIdx != oldArray's current index - 1) { deduped.set(old); } - // remember the value we just retained, not the (possibly stale) value under the write cursor cmp = old; } Suggestion importance[1-10]: 2 __ Why: The suggestion raises a theoretical concern about reference equality, but the improved_code is pseudocode (oldArray's current index - 1) and not valid Java. Also, an unnecessary set due to non-identical equal objects is harmless, so the concern's practical impact is minimal.	Low

---

Previous suggestions

Suggestions up to commit ca88f64

Category	Suggestion	Impact
Possible issue	Use comparator instead of reference equality Comparing retained/unique values via reference equality (deduped.next() != old) is incorrect for objects whose equality is defined by the comparator (e.g., Integer autoboxing outside the cache range, or String instances). Use the comparator to decide whether to overwrite, ensuring correctness for all object types. libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java [126-133] do { T old = oldArray.next(); // get the next item and advance iter if (comparator.compare(cmp, old) != 0) { // a new unique value: advance the write cursor and only copy when it isn't already in place - if (deduped.next() != old) { + if (comparator.compare(deduped.next(), old) != 0) { deduped.set(old); } // remember the value we just retained, not the (possibly stale) value under the write cursor cmp = old; } } while (oldArray.hasNext()); Suggestion importance[1-10]: 6 __ Why: Using reference equality (!=) to check if the value is already in place is a micro-optimization that avoids a redundant set call; correctness is preserved either way since deduped.set(old) would just overwrite with the same logical value. However, using the comparator is more semantically consistent and avoids any edge cases with object identity, though the original code was also correct.	Low

[github-actions]

❌ Gradle check result for ca88f64: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[winklemad]

On the automated suggestion to replace deduped.next() != old with a comparator check — I kept the reference-equality intentionally. Value-equality (what collapses duplicates) is already decided by the comparator in the outer if (comparator.compare(cmp, old) != 0). The inner reference check only decides whether a physical copy is needed at the write cursor, so at worst it performs a redundant, harmless set with an equal element — it never leaves a duplicate. Using the comparator there would only add a comparison without changing behavior.

To make that explicit I added a regression case with distinct Integer references outside the autobox cache ([1000, 1000, 2000, 2000]), which dedups to [1000, 2000] — confirming correctness for non-identical but equal objects.

[github-actions]

Persistent review updated to latest commit b803375

[github-actions]

✅ Gradle check result for b803375: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.46%. Comparing base (d603ae1) to head (b803375).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22445      +/-   ##
============================================
- Coverage     73.46%   73.46%   -0.01%     
+ Complexity    76477    76459      -18     
============================================
  Files          6104     6104              
  Lines        346568   346570       +2     
  Branches      49883    49884       +1     
============================================
- Hits         254598   254594       -4     
- Misses        71693    71727      +34     
+ Partials      20277    20249      -28     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.