Fix parent-bucket scoping in StreamStringTermsAggregator

[harshavamsi]

When StreamStringTermsAggregator ran as a sub-aggregator under another terms aggregator, every parent bucket received the same inner bucket list. The collector wrote into a flat docCounts array keyed on segment ordinal, ignoring the owningBucketOrd passed into collect(), and selectTopBuckets walked that same global array once per parent ordinal.

Route collection through LongKeyedBucketOrds keyed on (owningBucketOrd, segmentOrdinal) so each parent bucket maintains its own doc-count state, and teach selectTopBuckets to enumerate buckets per owner via ordsEnum(owningBucketOrd). Thread CardinalityUpperBound into createStreamStringTermsAggregator so the factory can pick between FromSingle and FromMany implementations.

Add a deterministic reproducer in StreamStringTermsAggregatorTests that fails on the pre-fix code by asserting two parent buckets produce distinct inner results.

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4f9f35b)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Uninitialized candidate array slots candidateBucketOrds is allocated via newLongArray(size, false) (uninitialized), then only positions 0..cnt-1 are populated. When cnt < bucketsForOwner (some buckets filtered by minDocCount), the trailing slots contain garbage. Although the sort/selector operates within [0, cnt) / [0, effectiveSegmentSize), if the buffer is sized larger than cnt there is no functional bug — but if a future refactor iterates up to bucketsForOwner, it will read garbage. More immediately, allocating bucketsForOwner slots when many may be filtered inflates memory. Consider using true (zero-fill) or sizing the array to cnt after a first pass. try (LongArray candidateBucketOrds = context.bigArrays().newLongArray(Math.toIntExact(bucketsForOwner), false)) { int cnt = 0; long totalDocCount = 0; LongKeyedBucketOrds.BucketOrdsEnum enumerator = bucketOrds.ordsEnum(owningBucketOrd); while (enumerator.next()) { long bucketOrd = enumerator.ord(); long docCount = bucketDocCount(bucketOrd); totalDocCount += docCount; if (docCount >= thresholds.getMinDocCount()) { candidateBucketOrds.set(cnt++, bucketOrd); } } Possible Issue In the multi-valued collection path, the loop uses sortedDocValuesPerBatch.nextOrd() != SortedSetDocValues.NO_MORE_DOCS as the termination check. NO_MORE_DOCS is Integer.MAX_VALUE, but per newer Lucene APIs nextOrd() on SortedSetDocValues should be bounded by docValueCount() iterations rather than by a sentinel; the current check happens to work only because the count is also decremented. If nextOrd() ever returns a valid ord equal to that sentinel it would be treated as end-of-iteration. Prefer relying solely on docValueCount(). int count = sortedDocValuesPerBatch.docValueCount(); long ordinal; while ((count-- > 0) && (ordinal = sortedDocValuesPerBatch.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) { collectInto(sub, doc, owningBucketOrd, ordinal); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 4f9f35b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid unnecessary long-to-int narrowing for array size LongArray.newLongArray accepts a long size, but here bucketsForOwner (already a long) is narrowed via Math.toIntExact. This will throw ArithmeticException for owners with more than Integer.MAX_VALUE candidate ords, even though LongArray natively supports long sizes. Pass the long directly. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [268] -try (LongArray candidateBucketOrds = context.bigArrays().newLongArray(Math.toIntExact(bucketsForOwner), false)) { +try (LongArray candidateBucketOrds = context.bigArrays().newLongArray(bucketsForOwner, false)) { Suggestion importance[1-10]: 6 __ Why: Valid observation: BigArrays.newLongArray accepts a long, so Math.toIntExact unnecessarily narrows and could throw for very large bucket counts. Impact is low in practice but improves correctness.	Low
	Document empty close for Releasable contract ResultStrategy implements Releasable and used to release reusableIndices here. Although the field is now removed, subclasses may still hold resources; consider keeping close() as an extension point (fine as-is) but ensure any future strategy state is released. Confirm no other transient state (e.g., BigArrays-backed) escapes without release. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [368] @Override -public void close() {} +public void close() { + // no resources to release; kept for Releasable contract and future subclasses +} Suggestion importance[1-10]: 1 __ Why: Purely a documentation/comment suggestion with negligible impact on correctness or functionality.	Low
Possible issue	Fix fallback ordering direction for top-N selection When partiallyBuiltBucketComparator is null and order is not a key order, ensureOrdinalComparator() leaves ordinalComparator null, and the fallback compares raw doc counts. However, IntroSelector.select picks the smallest segmentSize elements, so this returns the lowest doc counts instead of the highest for count-descending order. Verify the sign or reuse partiallyBuiltBucketComparator for count ordering to avoid selecting the wrong top-N. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [313-329] @Override protected int comparePivot(int j) { long candidateBucketOrd = candidateBucketOrds.get(j); long left; long right; if (isKeyOrder(order)) { left = bucketOrds.get(candidateBucketOrd); right = pivotSegmentOrd; } else { left = candidateBucketOrd; right = pivotBucketOrd; } if (ordinalComparator != null) { return -ordinalComparator.compare(left, right); } - return Long.compare(bucketDocCount(candidateBucketOrd), bucketDocCount(pivotBucketOrd)); + // select() keeps the smallest N; invert so that highest doc counts are kept + return Long.compare(bucketDocCount(pivotBucketOrd), bucketDocCount(candidateBucketOrd)); } Suggestion importance[1-10]: 3 __ Why: The concern is speculative; ensureOrdinalComparator sets ordinalComparator in the non-key path as well (via partiallyBuiltBucketComparator), and the fallback is only reached if neither branch applies. The suggested inversion may introduce a bug rather than fix one.	Low

---

Previous suggestions

Suggestions up to commit 30d2ace

Category	Suggestion	Impact
Possible issue	Avoid unnecessary long-to-int narrowing LongArray.newLongArray accepts a long size, so calling Math.toIntExact(bucketsForOwner) needlessly narrows to int and will throw ArithmeticException when a single parent has more than Integer.MAX_VALUE candidate ords. Pass bucketsForOwner directly to preserve the full range supported by LongArray. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [268] -try (LongArray candidateBucketOrds = context.bigArrays().newLongArray(Math.toIntExact(bucketsForOwner), false)) { +try (LongArray candidateBucketOrds = context.bigArrays().newLongArray(bucketsForOwner, false)) { Suggestion importance[1-10]: 5 __ Why: Valid observation that LongArray accepts a long size, so the Math.toIntExact narrowing is unnecessary and could throw for extremely large parent bucket cardinalities. Impact is minor in practice.	Low
	Fix inverted sub-aggregation ordering sign When ordering by sub-aggregation (non-key, non-count), partiallyBuiltBucketComparator is used through ordinalComparator and expects composite bucket ords to look up sub-agg values via bucketDocCount/sub-agg state. Currently left/right are set correctly to the composite ords in the else branch, but the sign is negated (-ordinalComparator.compare), which inverts the intended sort direction for sub-aggregation ordering. Verify whether the negation should apply only to key ordering; otherwise sub-agg ordering will be reversed. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [313-329] @Override protected int comparePivot(int j) { long candidateBucketOrd = candidateBucketOrds.get(j); - long left; - long right; if (isKeyOrder(order)) { - left = bucketOrds.get(candidateBucketOrd); - right = pivotSegmentOrd; - } else { - left = candidateBucketOrd; - right = pivotBucketOrd; + long left = bucketOrds.get(candidateBucketOrd); + long right = pivotSegmentOrd; + return -ordinalComparator.compare(left, right); } if (ordinalComparator != null) { - return -ordinalComparator.compare(left, right); + return ordinalComparator.compare(candidateBucketOrd, pivotBucketOrd); } return Long.compare(bucketDocCount(candidateBucketOrd), bucketDocCount(pivotBucketOrd)); } Suggestion importance[1-10]: 3 __ Why: The suggestion questions the sign of the comparator without clear evidence it is incorrect; the negation was present in the prior code and may be intentional for IntroSelector semantics. The concern is speculative.	Low

Suggestions up to commit cc87077

Category	Suggestion	Impact
General	Avoid boxing when sorting selected candidates Boxing every index into Integer[] and sorting with a lambda comparator allocates unnecessarily on a hot path. Since you already have parallel primitive arrays, sort the surviving prefix in place by segment ordinal (co-swapping candidateBucketOrds) using a simple primitive sort, which avoids boxing and additional allocation. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [351-355] -// Match the historical emit order (ascending by segment ordinal, i.e. -// alphabetical) so downstream reduce ordering assumptions hold. -Integer[] indices = new Integer[effectiveSegmentSize]; +// Sort surviving prefix ascending by segment ordinal in place. +for (int i = 1; i < effectiveSegmentSize; i++) { + long segKey = candidateSegmentOrds[i]; + long bucketKey = candidateBucketOrds[i]; + int j = i - 1; + while (j >= 0 && candidateSegmentOrds[j] > segKey) { + candidateSegmentOrds[j + 1] = candidateSegmentOrds[j]; + candidateBucketOrds[j + 1] = candidateBucketOrds[j]; + j--; + } + candidateSegmentOrds[j + 1] = segKey; + candidateBucketOrds[j + 1] = bucketKey; +} + +List<B> result = new ArrayList<>(effectiveSegmentSize); +long selectedDocCount = 0; for (int i = 0; i < effectiveSegmentSize; i++) { - indices[i] = i; + long bucketOrd = candidateBucketOrds[i]; + long segmentOrd = candidateSegmentOrds[i]; + long docCount = bucketDocCount(bucketOrd); + result.add(buildFinalBucket(bucketOrd, segmentOrd, docCount)); + selectedDocCount += docCount; } -Arrays.sort(indices, (a, b) -> Long.compare(candidateSegmentOrds[a], candidateSegmentOrds[b])); Suggestion importance[1-10]: 5 __ Why: Reasonable performance improvement to avoid boxing on a hot path, though insertion sort may not be optimal for larger effectiveSegmentSize. The optimization is valid but of moderate impact.	Low
	Guard against oversized candidate arrays bucketsInOrd can legitimately exceed Integer.MAX_VALUE for high-cardinality parents, and Math.toIntExact will throw ArithmeticException instead of a user-actionable error. Consider validating this against a sane limit (e.g. MAX_ARRAY_LENGTH) and throwing a clearer exception, or streaming the enumerator into a growable structure rather than pre-sizing a long[]. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [270-271] -long[] candidateBucketOrds = new long[Math.toIntExact(bucketsForOwner)]; +if (bucketsForOwner > Integer.MAX_VALUE - 8) { + throw new IllegalStateException( + "Too many buckets for owning ord: " + bucketsForOwner + ); +} +long[] candidateBucketOrds = new long[(int) bucketsForOwner]; long[] candidateSegmentOrds = new long[candidateBucketOrds.length]; Suggestion importance[1-10]: 4 __ Why: Valid defensive programming concern for high-cardinality cases, providing a clearer error message. However, Math.toIntExact already throws in the extreme case, and this is an edge case unlikely in practice.	Low
	Remove dead fallback comparator branch When partiallyBuiltBucketComparator is used (sub-agg ordering), ensureOrdinalComparator() builds tempBucket1/tempBucket2 whose compareKey compares by bucketOrd. Passing composite bucket ordinals is correct, but this branch also falls through to the non-key comparator path — the fallback bucketDocCount compare below is dead code because ordinalComparator is always non-null after ensureOrdinalComparator(). More importantly, in key-order mode the comparator is fed segment ordinals whereas in count/sub-agg mode it is fed bucket ords; make sure ensureOrdinalComparator is invoked before entering the selector (it currently is) and consider removing the misleading dead fallback to avoid future confusion. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [327-344] @Override protected int comparePivot(int j) { long left, right; if (isKeyOrder(order)) { - // Compare segment ordinals (alphabetical proxy) left = segmentOrdsRef[j]; right = pivotSegmentOrd; } else { - // Compare bucket ordinals (so bucketDocCount reads per-owner counts) left = bucketOrdsRef[j]; right = pivotBucketOrd; } - if (ordinalComparator != null) { - return -ordinalComparator.compare(left, right); - } - long leftDocCount = bucketDocCount(bucketOrdsRef[j]); - long rightDocCount = bucketDocCount(pivotBucketOrd); - return Long.compare(leftDocCount, rightDocCount); + return -ordinalComparator.compare(left, right); } Suggestion importance[1-10]: 3 __ Why: Minor readability improvement. The fallback is likely dead code since ensureOrdinalComparator() is invoked before the selector, but removing it is a style/cleanup change with low impact.	Low

Suggestions up to commit 6844f93

Suggestions up to commit c637106

Category	Suggestion	Impact
Possible issue	Fix key-order fallback comparator branch When isKeyOrder(order) is true and ordinalComparator is null, the fallback compares doc counts instead of the intended segment ordinals, which contradicts the key-order semantics. The fallback should only apply for count/sub-agg ordering; for key order, compare left/right directly to preserve alphabetical ordering. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [327-344] @Override protected int comparePivot(int j) { long left, right; if (isKeyOrder(order)) { // Compare segment ordinals (alphabetical proxy) left = segmentOrdsRef[j]; right = pivotSegmentOrd; } else { // Compare bucket ordinals (so bucketDocCount reads per-owner counts) left = bucketOrdsRef[j]; right = pivotBucketOrd; } if (ordinalComparator != null) { return -ordinalComparator.compare(left, right); } + if (isKeyOrder(order)) { + return InternalOrder.isKeyAsc(order) ? Long.compare(left, right) : Long.compare(right, left); + } long leftDocCount = bucketDocCount(bucketOrdsRef[j]); long rightDocCount = bucketDocCount(pivotBucketOrd); return Long.compare(leftDocCount, rightDocCount); } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a theoretical edge case where the fallback branch could be reached with key ordering when ordinalComparator is null. However, in the code flow, ensureOrdinalComparator() is called before the selector runs and it sets ordinalComparator for both key order and sub-agg order, so the fallback is typically only hit for _count ordering. The impact is minor and mostly defensive.	Low
General	Handle high-cardinality candidate array sizing Math.toIntExact will throw an ArithmeticException if the number of unique terms for an owning bucket exceeds Integer.MAX_VALUE, silently failing the query. Consider validating and throwing a more descriptive error, or use a paginated/growable structure to handle high-cardinality parents safely. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [263-271] long bucketsForOwner = bucketOrds.bucketsInOrd(owningBucketOrd); if (bucketsForOwner == 0) { return new SelectionResult<>(new ArrayList<>(), 0L); } - -// Pair up each candidate as (bucketOrd, segmentOrdinal). We keep them in two -// parallel arrays sized bucketsForOwner. -long[] candidateBucketOrds = new long[Math.toIntExact(bucketsForOwner)]; +if (bucketsForOwner > Integer.MAX_VALUE) { + throw new IllegalStateException( + "Too many buckets for owning ord [" + owningBucketOrd + "]: " + bucketsForOwner + ); +} +long[] candidateBucketOrds = new long[(int) bucketsForOwner]; long[] candidateSegmentOrds = new long[candidateBucketOrds.length]; Suggestion importance[1-10]: 3 __ Why: The suggestion replaces Math.toIntExact with an explicit check that throws essentially the same kind of error. This is a minor readability/error-message improvement but doesn't fundamentally solve the high-cardinality problem it describes.	Low

Suggestions up to commit 772f18a

Category	Suggestion	Impact
Possible issue	Fix fallback count-order sort direction When ordinalComparator is null (count-order fallback), the comparator returns ascending by doc count, which selects the smallest counts instead of the largest. IntroSelector.select picks the first k in sort order, so for a top-N by count you must negate the comparison to keep the largest counts. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [341-343] @Override protected int comparePivot(int j) { long left, right; if (isKeyOrder(order)) { // Compare segment ordinals (alphabetical proxy) left = segmentOrdsRef[j]; right = pivotSegmentOrd; } else { // Compare bucket ordinals (so bucketDocCount reads per-owner counts) left = bucketOrdsRef[j]; right = pivotBucketOrd; } if (ordinalComparator != null) { return -ordinalComparator.compare(left, right); } long leftDocCount = bucketDocCount(bucketOrdsRef[j]); long rightDocCount = bucketDocCount(pivotBucketOrd); - return Long.compare(leftDocCount, rightDocCount); + return Long.compare(rightDocCount, leftDocCount); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a potentially valid concern about count-order sort direction in the fallback path. However, the original code before this PR used the same Long.compare(leftDocCount, rightDocCount) direction, so this may be pre-existing behavior. The fallback is likely rarely triggered since ensureOrdinalComparator() is called before selection, making the impact uncertain.	Low
General	Use accounted arrays for candidates Math.toIntExact(bucketsForOwner) will throw for high-cardinality fields exceeding Integer.MAX_VALUE, and allocating parallel long[] arrays sized by the full candidate count negates the memory-accounted BigArrays used previously. Consider using BigArrays.newLongArray for these buffers to preserve circuit-breaker accounting and support large cardinalities. server/src/main/java/org/opensearch/search/aggregations/bucket/terms/StreamStringTermsAggregator.java [270-271] long bucketsForOwner = bucketOrds.bucketsInOrd(owningBucketOrd); if (bucketsForOwner == 0) { return new SelectionResult<>(new ArrayList<>(), 0L); } -// Pair up each candidate as (bucketOrd, segmentOrdinal). We keep them in two -// parallel arrays sized bucketsForOwner. -long[] candidateBucketOrds = new long[Math.toIntExact(bucketsForOwner)]; -long[] candidateSegmentOrds = new long[candidateBucketOrds.length]; +// Pair up each candidate as (bucketOrd, segmentOrdinal), using accounted arrays. +LongArray candidateBucketOrds = context.bigArrays().newLongArray(bucketsForOwner, false); +LongArray candidateSegmentOrds = context.bigArrays().newLongArray(bucketsForOwner, false); Suggestion importance[1-10]: 5 __ Why: Valid observation that using plain long[] arrays bypasses circuit-breaker accounting and limits cardinality to Integer.MAX_VALUE. However, bucketsForOwner is bounded by per-owner cardinality which is typically much smaller than total cardinality, reducing the practical impact.	Low

[github-actions]

❌ Gradle check result for 366cce4: 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?

[github-actions]

Persistent review updated to latest commit b57e4ec

[github-actions]

❌ Gradle check result for b57e4ec: 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?

[github-actions]

Persistent review updated to latest commit b794d80

[github-actions]

✅ Gradle check result for b794d80: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 93.84615% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.64%. Comparing base (24a14b9) to head (6844f93).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ions/bucket/terms/StreamStringTermsAggregator.java	93.84%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21447      +/-   ##
============================================
+ Coverage     71.58%   71.64%   +0.06%     
- Complexity    77353    77386      +33     
============================================
  Files          6170     6170              
  Lines        359700   359718      +18     
  Branches      52459    52458       -1     
============================================
+ Hits         257493   257725     +232     
+ Misses        81808    81579     -229     
- Partials      20399    20414      +15     

☔ 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.

[github-actions]

Persistent review updated to latest commit f5c9b3f

[github-actions]

❌ Gradle check result for f5c9b3f: 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?

[github-actions]

Persistent review updated to latest commit 772f18a

[github-actions]

Persistent review updated to latest commit c637106

[harshavamsi]

@rishabhmaurya Added an IT for this case.

[github-actions]

✅ Gradle check result for c637106: SUCCESS

[harshavamsi]

{"run-benchmark-test":"id_17"}

[github-actions]

Persistent review updated to latest commit 6844f93

[github-actions]

The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/8609/ . Final results will be published once the job is completed.

[opensearch-ci-bot]

The benchmark job https://build.ci.opensearch.org/job/benchmark-pull-request/8609/ failed.
Please see logs to debug.

[github-actions]

❌ Gradle check result for 6844f93: 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?

[github-actions]

✅ Gradle check result for 6844f93: SUCCESS

[rishabhmaurya]

{"run-benchmark-test":"id_17"}

[github-actions]

The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/8621/ . Final results will be published once the job is completed.

[opensearch-ci-bot]

The benchmark job https://build.ci.opensearch.org/job/benchmark-pull-request/8621/ failed.
Please see logs to debug.

[rishabhmaurya]

@harshavamsi do you mind taking a look at the failure

[rishabhmaurya]

we should not be creating a regular java long array of size with upper bound upto max ordinals, which seems to be the case here. Please make use of bigarrays or it will result into hard OOMs rather than CBE.

[rishabhmaurya]

It is fine to create java array of size of topN requested which is effectiveSegmentSize or segmentSize which is usually much smaller

[rishabhmaurya]

You can see how in previous logic we made use of reusableIndices bigArray until we knew the effective segment size and then we lazily used java int array of that size to perform quick select.

[harshavamsi]

thanks, this is indeed a valid concern. Using BigArrays now.

[rishabhmaurya]

@harshavamsi added some comments, I highly suggest running some stress tests leading to CBEs for this part and validating its working as expected.
So with this change, streaming string terms aggregation will be supporting the sub aggregations as well. How about numeric terms aggs? do we allow it at time of validation numeric term aggs?

[github-actions]

Persistent review updated to latest commit cc87077

[rishabhmaurya]

is it possible in this new logic to retrieve in the ordinal order itself and avoid sorting here

[harshavamsi]

We get each ordinal in insertion order and not segment ordinal order, which means we cannot avoid sorting since downstream reduce expects keys to be sorted

[rishabhmaurya]

but we can retrieve in ordinal order right? by running a loop to max ordinal and then checking which ones made to topN instead of sorting?

[harshavamsi]

So sorting here would be O(NlogN) where N is the topN value which is usually small. But if we want to loop through the entire set of unique values, those could be very high per batch. So it would be O(k) where K could be a much higher value. wdyt?

[rishabhmaurya]

since we already do O(K) throughout the logic, another O(K) still keeps complexity linear in my opinion and would avoid log factor. I'm not too adamant on it, so if you think sort here is simplifying the code, go ahead

[github-actions]

❌ Gradle check result for cc87077: 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?

[github-actions]

Persistent review updated to latest commit 30d2ace

[github-actions]

Persistent review updated to latest commit 4f9f35b

[github-actions]

❌ Gradle check result for 4f9f35b: 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?