Change in behavior using SimpleCollector+TopScoreDocCollector between 10.2 and 10.3 when scoreMode==COMPLETE

[hossman]

Description

Attempting to upgrade Solr to use Lucene 10.3 caused a regression in one of our tests, which i tracked down a to a change in behavior when using a SimpleCollector (subclass) that wraps a TopScoreDocCollector instance

It's not clear to me if this new 10.3 behavior is technically "wrong", or if Solr is (miss|ab)using some of the Collector APIs -- so i wanted to file this report for completeness (Even if Solr ultimately makes other changes to work around this change in behavior)

---

I've boiled the problem down to the simplest test case patch i can think of that demonstrates the problem. In 10.2, both asserts in this test pass, in 10.3, neither assert will pass.

The crux of the issue as i understand it, is that TopScoreDocCollector tracks competitive scoring info that it cares about in the Scorable, and that info is (now) being used by the Scorer to limit what documents are collected -- even though the Collector says it wants ScoreMode.COMPLETE collection.

collector_behavior_change_10_3.patch.txt

(FWIW: the patch modifies TestMultiCollector simply because i didn't see a more obvious place to add tests of subclassing SimpleCollector)

Version and environment details

No response

[hossman]

git bisect indicates the behavior change (causing test in the patch to fail) was introduced by adbf9a9 /#14709

[msokolov]

I wasn't sure so I checked the javadoc for ScoreMode.COMPLETE and it says "Produced scorers will allow visiting all matches and get their score." I guess you knew this, but I just thought it might be helpful to copy it here since it does seem to indicate "all matches" will be visited. So, IMO, it's a bug, yes.

[hossman]

Sure ... but is the bug in the code or in the docs?

The "Collector" API/concept has evolved enough over the last few years -- especially with the interplay of concepts like ScoreMode.COMPLETE vs Scorable.setMinCompetitiveScore(...) -- that I didn't want to make any assumptions about what the "expected" behavior is in this kind of situation.

My main goal is to point out that a change in behavior has occurred -- and ask the folks that best understand this code (and it's intent) what the expected behavior should be -- and then code/docs/tests can be updated from there.

/ping @jpountz @gf2121

[iverase]

I wonder if this a side effect of #15263
The attached test still fails.

[jimczi]

The tests fail because setMinCompetitiveScore is being called when the score mode is not compatible.
It is the responsibility of the collector to prevent such calls, so this is not a bug.

For example, see the MultiCollector. When passing the scorer into the leaf collector, you need to wrap it so that it ignores calls to setMinCompetitiveScore.

You can fix this by changing the simple collector in your test as follows:

@Override
public void setScorer(Scorable scorer) throws IOException {
    scorer = new FilterScorable(scorer) {
        @Override
        public void setMinCompetitiveScore(float minScore) throws IOException {
            // Ignore calls to setMinCompetitiveScore
        }
    };
    leafIn.setScorer(scorer);
}

Alternatively, you can use the option provided by the TopScoreDocCollectorManager.
If you want to see all hits, simply set the totalHitsThreshold to Integer.MAX_VALUE.

[hossman]

The tests fail because setMinCompetitiveScore is being called when the score mode is not compatible. It is the responsibility of the collector to prevent such calls ... For example, see the MultiCollector.

I understand why the code is failing now, and how the code in MultiCollector prevents such failures from happening (I already worked up a similar patch for Solr) -- but:

1. As Michael pointed out above: the docs aren't clear on what happens if/when setMinCompetitiveScore is called used even if scoreMode == COMPLETE.
2. My understand -- which seemed to be true in previous versions of Lucene -- was always that if ScoreMode.isExhaustive() was true, that effectively superceeded setMinCompetitiveScore and made it irrelevant.
3. That "expected behavior" did stop being true between 10.2 and 10.3, w/o a clear indication that it was an intentional change.

You say: "It is the responsibility of the collector to prevent such calls" but I can't find anything in the docs for Collectors/ScoreMode/Scorable that make explicit.

---

If the change in behavior in this type of situation is EXPECTED as a side effect of the optimizations added in #14709, then that's fine -- but the docs should be updated to make it clear.

[msokolov]

so, perhaps we could change the ScoreMode.COMPLETE javadocs to say "Produced scorers will allow for visiting all matches and getting their score unless overridden by calling setMinCompetitiveScore"? or something?

[jimczi]

Re-reading the code I think the new BatchScoreBulkScorer should check the score mode before using the minimum score to skip. It's not a bug per say since the minimum score should not be set if the underlying collector is in complete node.
What makes this case weird is that multiple collectors with different strategy are used.
Following what we do in queries (we check the score mode eagerly before skipping), I think we should do the same in BatchScoreBulkScorer. That won't fix the need to override setMinCompetitiveScore for this specific case since the underlying collector is built with a different score mode.