Simplify logic in ScoreCachingWrappingScorer #14012
Conversation
This is functionally equivalent to the logic that was present, but makes the behavior clearer.
| @@ -31,8 +31,7 @@ | |||
| */ | |||
| public final class ScoreCachingWrappingScorer extends Scorable { | |||
|
|
|||
| private int lastDoc = -1; | |||
| private int curDoc = -1; | |||
| private boolean computeScore; | |||
There was a problem hiding this comment.
Minor: how about renaming this to something that tells whether the score is cached already e.g. scoreCached ?And the flag gets unset whenever we collect a new doc (we would reverse the boolean meaning with this name).
There was a problem hiding this comment.
+1 I had a similar reaction when reading this PR. It's easier for me to reason about when the cached score should be invalidated than when the score should be computed.
| @@ -31,8 +31,7 @@ | |||
| */ | |||
| public final class ScoreCachingWrappingScorer extends Scorable { | |||
|
|
|||
| private int lastDoc = -1; | |||
| private int curDoc = -1; | |||
| private boolean computeScore; | |||
There was a problem hiding this comment.
+1 I had a similar reaction when reading this PR. It's easier for me to reason about when the cached score should be invalidated than when the score should be computed.
Keep track current state instead of future plans.
|
Thanks folks! I made the suggested change and took this PR out of draft. I think this one makes more sense than my previous one over at #13987 |
Description
This is functionally equivalent to the logic that was present, but makes the behavior clearer.