romseygeek commented on code in PR #16434:
URL: https://github.com/apache/lucene/pull/16434#discussion_r3683113225
##########
lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java:
##########
@@ -45,20 +45,18 @@ private abstract class TopFieldLeafCollector implements
LeafCollector {
final LeafFieldComparator comparator;
final int reverseMul;
+ // Whether the search sort is a prefix of this segment's index sort
(decided per segment).
+ final boolean searchSortPartOfIndexSort;
Scorable scorer;
boolean collectedAllCompetitiveHits = false;
TopFieldLeafCollector(FieldValueHitQueue<Entry> queue, Sort sort,
LeafReaderContext context)
throws IOException {
- // 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);
- if (searchSortPartOfIndexSort) {
- firstComparator.disableSkipping();
- }
- }
+ // Whether the search sort is a prefix of the index sort is decided per
segment: a MultiReader
+ // may combine segments with different index sorts, so this cannot be
cached across leaves
+ // (GITHUB#14399).
+ final Sort indexSort = context.reader().getMetaData().sort();
Review Comment:
It would be nice to use Sort.getPrimarySortField() here but I think that's
going to end up being fairly complex in its interaction with sort prefixes, so
we can leave that for a follow-up.
##########
lucene/core/src/test/org/apache/lucene/search/TestTopFieldCollectorEarlyTermination.java:
##########
@@ -264,4 +268,88 @@ public void testCanEarlyTerminateOnPrefix() {
new SortField("c", SortField.Type.LONG),
new SortField("b", SortField.Type.STRING))));
}
+
+ /**
+ * GITHUB#14399: TopFieldCollector caches whether the search sort is a
prefix of the index sort
+ * after inspecting only the first leaf. That is safe for a single index
(IndexWriter enforces one
+ * index sort), but a MultiReader can span indexes with different index
sorts. Here the first
+ * index is sorted so the search sort IS a prefix (early termination is
eligible) while the second
+ * index is sorted the opposite way (it is NOT). The cached "yes" wrongly
early-terminates the
+ * second leaf and drops results that should rank first.
+ */
+ public void testMultiReaderWithDifferentIndexSorts() throws IOException {
+ final Sort ascSort = new Sort(new SortField("ndv", SortField.Type.LONG));
+
+ // Index A: sorted ndv ASC, many docs with a moderate value (50). Under an
ASC search sort this
+ // leaf is prefix-sorted, so the collector caches "search sort is part of
index sort" = true and
+ // calls disableSkipping().
+ Directory dirA = newDirectory();
+ IndexWriterConfig iwcA = newIndexWriterConfig().setIndexSort(ascSort);
+ iwcA.setMergeScheduler(new SerialMergeScheduler());
Review Comment:
Do we need to set the merge scheduler if we're force-merging down to a
single segment?
--
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]
