serhiy-bzhezytskyy opened a new pull request, #16434:
URL: https://github.com/apache/lucene/pull/16434
Fixes #14399.
`TopFieldCollector` decided whether the search sort is a prefix of the index
sort by inspecting only the **first** segment, then cached that decision for
the whole search:
```java
// as all segments are sorted in the same way, enough to check only the 1st
segment for indexSort
if (searchSortPartOfIndexSort == null) {
final Sort indexSort = context.reader().getMetaData().sort();
searchSortPartOfIndexSort = canEarlyTerminate(sort, indexSort);
...
}
```
The premise holds for a single index — `IndexWriter` enforces one index sort
— but not for a `MultiReader`, which can combine indexes sorted differently.
The cached decision is then wrong for later segments, and early termination
drops documents that should have ranked first.
**This is a wrong-results bug, not only a missed optimisation.** The added
test builds a `MultiReader` over two indexes sorted in *opposite* directions
(`ndv` ascending and `ndv` descending) and searches with `ndv` ascending. The
competitive document sits at the end of the second segment's docid order, so
the decision cached from the first segment early-terminates the second one and
discards it. Without the fix:
```
AssertionError: the smallest value (1, trailing docid of leaf B) must win
expected:<1> but was:<50>
```
### The fix
Decide it per segment, inside `TopFieldLeafCollector`, rather than caching
across leaves.
That change requires one adjustment: the old code called `disableSkipping()`
on the shared `FieldComparator`, which is the wrong scope once the decision is
per segment. `disableSkipping()` is now a default method on
`LeafFieldComparator` and is called on that segment's leaf comparators;
`NumericComparator` and `TermOrdValComparator` override it to drop their
competitive iterator. This is close to what @jpountz suggested on the issue,
except that the method moves to `LeafFieldComparator` rather than being removed
from `FieldComparator` — the leaf is the object whose lifetime matches the
decision.
### Verification
- The new test fails on unmodified `main` with the assertion above, and
passes with the fix. Checked both ways rather than only the passing direction.
- `TestTopFieldCollector*`, `TestSortOptimization`, `TestSearchAfter` and
the `comparators` package pass.
- `./gradlew check -x test` is green.
--
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]
