- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.2k
[10.3] Fix returned Impacts when frequencies are not indexed #15263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
        
          
                lucene/core/src/java/org/apache/lucene/codecs/lucene103/Lucene103PostingsReader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | Added a single test here and open #15266 for a bigger coverage in the active branches. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems correct to me.
curious as to what @gf2121 thinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| If not already the case, let’s forward port the test to main. | 
| @ChrisHegarty see #15266 | 
| (No changes) | ||
| * GITHUB#15263: Fix the returned Impact returned from Lucene103PostingsReader when frequencies | ||
| are not indexed. It was returning a wrong frequency in that case affecting scoring which | ||
| might led to performance issues. (Ignacio Vera) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe replace "wrong" with "suboptimal" since it's correct to return impact scores that are much higher than they should, just suboptimal as it hurts dynamic pruning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 25d6374
| For the record, this looks good to me! | 
| Wasn't this already fixed in #14511 ? | 
| yes @msfroh apparently in all the shuffle with the new postings, it was accidentally overwritten and then fixed again in main, but never backported. Quite the kerfuffle :D | 
| Ahh -- looks like #14557 backported the 101 postings reader to 10_x, but missed the 103 postings reader. | 
Return an Impact object with freqs equal to 1 instead of Integer.MAX_VALUE.
Note that this bug does not exists in main as it was fixed in #15151
Iam working on adding a test