zacharymorn commented on a change in pull request #2141:
URL: https://github.com/apache/lucene-solr/pull/2141#discussion_r553153647
##########
File path:
lucene/core/src/java/org/apache/lucene/search/Boolean2ScorerSupplier.java
##########
@@ -230,10 +232,13 @@ private Scorer opt(
for (ScorerSupplier scorer : optional) {
optionalScorers.add(scorer.get(leadCost));
}
- if (minShouldMatch > 1) {
+
+ if (scoreMode == ScoreMode.TOP_SCORES) {
+ return new WANDScorer(weight, optionalScorers, minShouldMatch);
+ } else if (minShouldMatch > 1) {
+ // nocommit minShouldMath > 1 && scoreMode != ScoreMode.TOP_SCORES
still requires MinShouldMatchSumScorer.
+ // Do we want to deprecate this entirely now ?
Review comment:
I've taken a second look at this, and had the following thoughts /
questions:
Previously, in Boolean2ScorerSupplier#opt, the scorer picking logic is
```
if (minShouldMatch > 1) {
return MinShouldMatchSumScorer...
} else if (scoreMode == ScoreMode.TOP_SCORES) {
return WANDScorer...
} else {
return DisjunctionSumScorer...
}
```
With the current changes in this PR, we now have
```
if (scoreMode == ScoreMode.TOP_SCORES) {
return WANDScorer...
} else if (minShouldMatch > 1) {
return MinShouldMatchSumScorer...
} else {
return DisjunctionSumScorer...
}
```
I noticed that with trivial changes, WANDScorer seems to be usable for all
ScoreMode when `scoreMode.needsScore == true` as well (passed all existing
tests with trivial updates). So we can arrive at something like this
```
if (scoreMode.needsScore()) {
return WANDScorer...
} else if (minShouldMatch > 1) {
return MinShouldMatchSumScorer...
} else {
return DisjunctionSumScorer...
}
```
Is there a reason `WANDScorer` is only used for TOP_DOCS before? I did read
up the documentation and discussion in
https://issues.apache.org/jira/browse/LUCENE-4100, but it's a bit long so I'm
not sure I'm following all the details there.
Following the above, since `WANDScorer` can already handle the case where
`minShouldMatch == 0`, then I can see MinShouldMatchSumScorer be merged into
WANDScorer mostly to handle non-scoring mode, arriving at
```
if (?) {
return WANDScorer...
} else {
return DisjunctionSumScorer...
}
```
At this point, it actually seems like `WANDScorer` can handle all the cases
already, so `DisjunctionSumScorer` can be deprecated as well to just return
`WANDScorer` here?
Is the above the right reasoning of how things would go for the deprecation?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
