mikemccand commented on code in PR #13987:
URL: https://github.com/apache/lucene/pull/13987#discussion_r1840911855
##########
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:
Hmm why are we collecting the same `doc` twice? (I thought this is smelly
-- we fixed the other two tests to stop doing that?).
Oh I see, is so `score()` happens more than once while on the same doc?
Actually, since `ScoreCachingCollector`'s `LeafCollector` is calling `.score()`
three times for each `.collect()`, you should be able to do only one
`lc.collect(doc)` here?
Does this new test case fail if you revert the bug fix?
##########
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);
+ }
+
+ for (int i = 0; i < scores.length; i++) {
+ assertEquals(scores[i], scc.mscores[i * 2], 0f);
+ assertEquals(scores[i], scc.mscores[i * 2 + 1], 0f);
+ }
+
+ assertEquals(scores.length, countingScorable.count);
Review Comment:
Ahh I see -- you want to assert that indeed `score()` was called (up on top)
multiple times per hit, but then down below in the "true" scorer, it was
dedup'd to just once per hit. So that's why you `.collect(doc)` twice, OK ...
##########
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;
Review Comment:
We don't need the `= 0` since it's java's default already?
##########
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);
Review Comment:
Maybe add a comment about why we need `* 2` here?
--
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
