[GitHub] [lucene-solr] zacharymorn commented on a change in pull request #2205: LUCENE-9668: Deprecate MinShouldMatchSumScorer with WANDScorer

[GitBox <gi...@apache.org>]

zacharymorn commented on a change in pull request #2205:
URL: https://github.com/apache/lucene-solr/pull/2205#discussion_r559041326



##########
File path: lucene/core/src/java/org/apache/lucene/search/DisjunctionMaxScorer.java
##########
@@ -50,7 +50,7 @@
     if (tieBreakerMultiplier < 0 || tieBreakerMultiplier > 1) {
       throw new IllegalArgumentException("tieBreakerMultiplier must be in [0, 1]");
     }
-    if (scoreMode == ScoreMode.TOP_SCORES) {
+    if (scoreMode.needsScores()) {

Review comment:
       Sorry should have put in a nocommit here to explain this change. Basically without this change a NPE would be thrown for test `DisMaxRequestHandlerTest#testSomeStuff` with the following stack trace:
   
   ```
    2> 4261 ERROR (TEST-DisMaxRequestHandlerTest.testSomeStuff-seed#[66968D4353C5888]) [     ] o.a.s.h.RequestHandlerBase java.lang.NullPointerException
     2>    at org.apache.lucene.search.DisjunctionMaxScorer.advanceShallow(DisjunctionMaxScorer.java:78)
     2>    at org.apache.lucene.search.WANDScorer.<init>(WANDScorer.java:179)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.opt(Boolean2ScorerSupplier.java:257)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.getInternal(Boolean2ScorerSupplier.java:124)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.get(Boolean2ScorerSupplier.java:96)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.req(Boolean2ScorerSupplier.java:168)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.getInternal(Boolean2ScorerSupplier.java:146)
     2>    at org.apache.lucene.search.Boolean2ScorerSupplier.get(Boolean2ScorerSupplier.java:96)
     2>    at org.apache.lucene.search.BooleanWeight.scorer(BooleanWeight.java:353)
     2>    at org.apache.lucene.search.Weight.bulkScorer(Weight.java:166)
     2>    at org.apache.lucene.search.BooleanWeight.bulkScorer(BooleanWeight.java:343)
     2>    at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:743)
     2>    at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:533)
     2>    at org.apache.solr.search.SolrIndexSearcher.buildAndRunCollectorChain(SolrIndexSearcher.java:210)
     2>    at org.apache.solr.search.SolrIndexSearcher.getDocListAndSetNC(SolrIndexSearcher.java:1709)
     2>    at org.apache.solr.search.SolrIndexSearcher.getDocListC(SolrIndexSearcher.java:1412)
     2>    at org.apache.solr.search.SolrIndexSearcher.search(SolrIndexSearcher.java:599)
     2>    at org.apache.solr.handler.component.QueryComponent.doProcessUngroupedSearch(QueryComponent.java:1508)
     2>    at org.apache.solr.handler.component.QueryComponent.process(QueryComponent.java:398)
     2>    at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:355)
     2>    at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:214)
     2>    at org.apache.solr.core.SolrCore.execute(SolrCore.java:2617)
     2>    at org.apache.solr.util.TestHarness.query(TestHarness.java:346)
     2>    at org.apache.solr.util.TestHarness.query(TestHarness.java:328)
     2>    at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:982)
     2>    at org.apache.solr.DisMaxRequestHandlerTest.doTestSomeStuff(DisMaxRequestHandlerTest.java:141)
     2>    at org.apache.solr.DisMaxRequestHandlerTest.testSomeStuff(DisMaxRequestHandlerTest.java:72)
   ```
   
   The scoreMode and minShouldMatch combination that caused this exception was `scoreMode == COMPLETE`, `minShouldMatch == 3`. 
   
   I think what's happening here is, `MinShouldMatchSumScorer` was already used in test for other scoring ScoreMode in addition to `ScoreMode.TOP_SCORES` (and probably already used by clients in the wild as well), as the original condition before adding `minShouldMatch` support to `WANDScorer` is 
   
   ```
   if (minShouldMatch > 1) {
     return MinShouldMatchSumScorer...
   } else if (scoreMode == ScoreMode.TOP_SCORES) {
     return WANDScorer...
   } else {
     return DisjunctionSumScorer...
   }
   ```
   
   so I was assuming we may need to preserve this behavior when we deprecate `MinShouldMatchSumScorer` with `WANDScorer`?  This was also the reason it was checking for `needsScore` instead of `scoreMode == TOP_SCORES` in `WANDScorer` constructor, and the long nocommit message in `Boolean2ScorerSupplier.java` to see if it may need a different conditional to differentiate between `WANDScorer` and `DisjunctionSumScorer`, as you noted in other review comments.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org