-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[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