msfroh commented on code in PR #13987:
URL: https://github.com/apache/lucene/pull/13987#discussion_r1841119813


##########
lucene/core/src/test/org/apache/lucene/search/TestScoreCachingWrappingScorer.java:
##########
@@ -157,4 +157,40 @@ public void testGetScores() throws Exception {
     ir.close();
     directory.close();
   }
+
+  private static class CountingScorable extends FilterScorable {
+    int count = 0;
+
+    public CountingScorable(Scorable in) {
+      super(in);
+    }
+
+    @Override
+    public float score() throws IOException {
+      count++;
+      return in.score();
+    }
+  }
+
+  public void testRepeatedCollectReusesScore() throws Exception {
+    Scorer s = new SimpleScorer();
+    CountingScorable countingScorable = new CountingScorable(s);
+    ScoreCachingCollector scc = new ScoreCachingCollector(scores.length * 2);
+    LeafCollector lc = scc.getLeafCollector(null);
+    lc.setScorer(countingScorable);
+
+    // We need to iterate on the scorer so that its doc() advances.
+    int doc;
+    while ((doc = s.iterator().nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
+      lc.collect(doc);
+      lc.collect(doc);

Review Comment:
   The new test does fail if I revert the fix. 
   
   As I mentioned in 
https://github.com/apache/lucene/pull/13987#issuecomment-2472044035, the 
condition for this being an actual bug is probably not really valid (i.e. you 
need to call `collect()` on the `ScoreCachingWrappingLeafCollector` multiple 
times for the same document).
   
   The caching *does* work if the wrapped `LeafCollector` calls `score()` 
multiple times, which can happen in `MultiCollector` (for example). The 
layering is roughly:
   
   ```
   ScoreCachingWrappingLeafCollector wraps
     Some other LeafCollector whose score is
       ScoreCachingWrappingScorer, which wraps
         The real Scorable
   ```
   
   The multiple calls to `score()` from `Some other LeafCollector` work just 
fine. Again, it's the call to `collect()` on 
`ScoreCachingWrappingLeafCollector` that was invalidating the cache.
   
   Those other two tests broke because my "fix" causes the caching to work (but 
they didn't expect it).



-- 
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-unsubscr...@lucene.apache.org

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

Reply via email to