jpountz commented on a change in pull request #2141:
URL: https://github.com/apache/lucene-solr/pull/2141#discussion_r550805393
##########
File path:
lucene/core/src/java/org/apache/lucene/search/Boolean2ScorerSupplier.java
##########
@@ -74,11 +74,13 @@ private long computeCost() {
return minRequiredCost.getAsLong();
} else {
final Collection<ScorerSupplier> optionalScorers =
subs.get(Occur.SHOULD);
- final long shouldCost =
- MinShouldMatchSumScorer.cost(
- optionalScorers.stream().mapToLong(ScorerSupplier::cost),
- optionalScorers.size(),
- minShouldMatch);
+ // nocommit The cost calculation here copies that in WANDScorer's
constructor, and may need to be adjusted?
+ final long shouldCost = scoreMode == ScoreMode.TOP_SCORES ?
+
optionalScorers.stream().mapToLong(ScorerSupplier::cost).sum() :
+ MinShouldMatchSumScorer.cost(
+
optionalScorers.stream().mapToLong(ScorerSupplier::cost),
+ optionalScorers.size(),
+ minShouldMatch);
Review comment:
Actually this bit was correct, we should instead fix WANDScorer to take
minShouldMatch into account the same way MinShouldMatchSumScorer does.
##########
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:
Maybe not now in order to keep this PR contained, but it would be nice
if we could handle this case with WANDScorer too indeed.
##########
File path: lucene/core/src/java/org/apache/lucene/search/WANDScorer.java
##########
@@ -447,6 +461,23 @@ private int doNextCompetitiveCandidate() throws
IOException {
}
}
+ // nocommit Do we still need the following given
TwoPhaseIterator.matches already performs the check
+// minCompetitiveScore satisfied, now checks if the doc has enough
required number of matches
+// while (freq < minShouldMatch) {
+// if (freq + tailSize >= minShouldMatch) {
+// // a match on doc is still possible, try to
+// // advance scorers from the tail
+// advanceTail();
Review comment:
I don't think we should advanceTail here, which may be costly. We should
just make sure that `freq + tailSize >= minShouldMatch` and otherwise call your
`else` block.
##########
File path: lucene/core/src/java/org/apache/lucene/search/WANDScorer.java
##########
@@ -130,10 +130,21 @@ private static long scaleMinScore(float minScore, int
scalingFactor) {
int upTo; // upper bound for which max scores are valid
- WANDScorer(Weight weight, Collection<Scorer> scorers) throws IOException {
+ int minShouldMatch;
+ int freq;
+
+ WANDScorer(Weight weight, Collection<Scorer> scorers, int minShouldMatch)
throws IOException {
super(weight);
+ if (minShouldMatch >= scorers.size()) {
+ throw new IllegalArgumentException("minShouldMatch should be < the
number of scorers");
+ }
+
this.minCompetitiveScore = 0;
+
+ assert minShouldMatch >=0 : "minShouldMatch should not be negative, but
got " + minShouldMatch;
Review comment:
```suggestion
assert minShouldMatch >= 0 : "minShouldMatch should not be negative, but
got " + minShouldMatch;
```
##########
File path: lucene/core/src/java/org/apache/lucene/search/WANDScorer.java
##########
@@ -130,10 +130,21 @@ private static long scaleMinScore(float minScore, int
scalingFactor) {
int upTo; // upper bound for which max scores are valid
- WANDScorer(Weight weight, Collection<Scorer> scorers) throws IOException {
+ int minShouldMatch;
Review comment:
Let's make it final?
##########
File path: lucene/core/src/java/org/apache/lucene/search/WANDScorer.java
##########
@@ -447,6 +461,23 @@ private int doNextCompetitiveCandidate() throws
IOException {
}
}
+ // nocommit Do we still need the following given
TwoPhaseIterator.matches already performs the check
+// minCompetitiveScore satisfied, now checks if the doc has enough
required number of matches
Review comment:
Indeed I don't think we need it, but I like doing it so that the
approximation doesn't return documents knowing that no match is possible.
----------------------------------------------------------------
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
