LUCENE-10036: Add factory method to ScoreCachingWrappingScorer that ensures unnecessary wrapping doesn't occur (#226)

lucene/CHANGES.txt

@@ -357,6 +357,9 @@ API Changes
   Users can now access the count of an ordinal directly without constructing an extra FacetLabel.
   Also use variable length arguments for the getOrdinal call in TaxonomyReader. (Gautam Worah)
 
+* LUCENE-10036: Replaced the ScoreCachingWrappingScorer ctor with a static factory method that
+  ensures unnecessary wrapping doesn't occur. (Greg Miller)
+
 New Features
 ---------------------
 (No changes)

lucene/core/src/java/org/apache/lucene/search/FieldComparator.java

@@ -187,11 +187,7 @@ public abstract class FieldComparator<T> {
       // wrap with a ScoreCachingWrappingScorer so that successive calls to
       // score() will not incur score computation over and
       // over again.
-      if (!(scorer instanceof ScoreCachingWrappingScorer)) {
-        this.scorer = new ScoreCachingWrappingScorer(scorer);
-      } else {
-        this.scorer = scorer;
-      }
+      this.scorer = ScoreCachingWrappingScorer.wrap(scorer);
     }
 
     @Override

lucene/core/src/java/org/apache/lucene/search/MultiCollector.java

@@ -158,7 +158,7 @@ public class MultiCollector implements Collector {
     @Override
     public void setScorer(Scorable scorer) throws IOException {
       if (cacheScores) {
-        scorer = new ScoreCachingWrappingScorer(scorer);
+        scorer = ScoreCachingWrappingScorer.wrap(scorer);
       }
       if (skipNonCompetitiveScores) {
         for (int i = 0; i < collectors.length; ++i) {

lucene/core/src/java/org/apache/lucene/search/PositiveScoresOnlyCollector.java

@@ -37,7 +37,7 @@ public class PositiveScoresOnlyCollector extends FilterCollector {
 
       @Override
       public void setScorer(Scorable scorer) throws IOException {
-        this.scorer = new ScoreCachingWrappingScorer(scorer);
+        this.scorer = ScoreCachingWrappingScorer.wrap(scorer);
         in.setScorer(this.scorer);
       }
 

lucene/core/src/java/org/apache/lucene/search/ScoreCachingWrappingScorer.java

@@ -35,8 +35,22 @@ public final class ScoreCachingWrappingScorer extends Scorable {
   private float curScore;
   private final Scorable in;
 
+  /**
+   * Wraps the provided {@link Scorable} unless it's already an instance of {@code
+   * ScoreCachingWrappingScorer}, in which case it will just return the provided instance.
+   *
+   * @param scorer Underlying {@code Scorable} to wrap
+   * @return Instance of {@code ScoreCachingWrappingScorer} wrapping the underlying {@code scorer}
+   */
+  public static Scorable wrap(Scorable scorer) {
+    if (scorer instanceof ScoreCachingWrappingScorer) {
+      return scorer;
+    }
+    return new ScoreCachingWrappingScorer(scorer);
+  }
+
   /** Creates a new instance by wrapping the given scorer. */
-  public ScoreCachingWrappingScorer(Scorable scorer) {
+  private ScoreCachingWrappingScorer(Scorable scorer) {
     this.in = scorer;
   }
 

lucene/core/src/test/org/apache/lucene/search/TestScoreCachingWrappingScorer.java

@@ -105,7 +105,7 @@ public class TestScoreCachingWrappingScorer extends LuceneTestCase {
 
     @Override
     public void setScorer(Scorable scorer) {
-      this.scorer = new ScoreCachingWrappingScorer(scorer);
+      this.scorer = ScoreCachingWrappingScorer.wrap(scorer);
     }
 
     @Override
@@ -156,4 +156,28 @@ public class TestScoreCachingWrappingScorer extends LuceneTestCase {
     ir.close();
     directory.close();
   }
+
+  public void testNoUnnecessaryWrap() throws Exception {
+    Scorable base =
+        new Scorable() {
+          @Override
+          public float score() throws IOException {
+            return -1;
+          }
+
+          @Override
+          public int docID() {
+            return -1;
+          }
+        };
+
+    // Wrapping the first time should produce a different instance:
+    Scorable wrapped = ScoreCachingWrappingScorer.wrap(base);
+    assertNotEquals(base, wrapped);
+
+    // But if we try to wrap an instance of ScoreCachingWrappingScorer, it shouldn't unnecessarily
+    // wrap again:
+    Scorable doubleWrapped = ScoreCachingWrappingScorer.wrap(wrapped);
+    assertSame(wrapped, doubleWrapped);
+  }
 }

lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysScorer.java

@@ -152,7 +152,7 @@ class DrillSidewaysScorer extends BulkScorer {
     // if (DEBUG) {
     //  System.out.println("  doQueryFirstScoring");
     // }
-    setScorer(collector, new ScoreCachingWrappingScorer(baseScorer));
+    setScorer(collector, ScoreCachingWrappingScorer.wrap(baseScorer));
 
     int docID = baseScorer.docID();