[GitHub] [solr] cpoerschke commented on a change in pull request #228: SOLR-15496 LTRRescorer unnecessarily (re)creates Comparator

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

cpoerschke commented on a change in pull request #228:
URL: https://github.com/apache/solr/pull/228#discussion_r673272242



##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRRescorer.java
##########
@@ -53,6 +53,21 @@ public LTRRescorer(LTRScoringQuery scoringQuery) {
     this.scoringQuery = scoringQuery;
   }
 
+  final protected static Comparator<ScoreDoc> docComparator = (a, b) -> a.doc - b.doc;

Review comment:
       ```suggestion
     final private static Comparator<ScoreDoc> docComparator = (a, b) -> a.doc - b.doc;
   ```
   
   i.e. minimum necessary visibility.
   
   And I've seen `Comparator.comparingInt` used elsewhere in the code base recently and would be curious if using that here too might be an alternative, what do you think?

##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRRescorer.java
##########
@@ -131,31 +146,12 @@ public TopDocs rescore(IndexSearcher searcher, TopDocs firstPassTopDocs,
   }
 
   protected static void sortByScore(ScoreDoc[] reranked) {

Review comment:
       So now the `sortByScore` contains much less code than before. I wonder if its future might look like this:
   * step 1: mark it deprecated (in this pull request) so that no new code starts calling it
   * step 2: change existing code (in this pull request) to sort directly with the new comparator (which already has protected visibility equivalent to the visibility of this method)
   * step 3: remove the deprecated method (after this pull request is merged) on `main` branch only for Solr 9.0 onwards but retain it in `branch_8x` branch for Solr 8.10 onwards, to maintain build backwards compatibility for anyone who may have built a custom plugin with a custom rescorer.
   
   ```suggestion
     @Deprecated
     protected static void sortByScore(ScoreDoc[] reranked) {
   ```




-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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