-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix potential NPE for KnnVectorQuery #15334
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
Fix potential NPE for KnnVectorQuery #15334
Conversation
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
throws IOException { | ||
// The delegate supports optimistic collection | ||
if (delegate.isOptimistic()) { | ||
if (delegate.isOptimistic() && context.parent != null) { |
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.
I am assuming the same reasoning applies here as well? // ctx.parent could be null if this is a MemoryIndex
. If yes, can be add the comment here as well?
If this is a real problem, why hasn't it surfaced in unit test failures? Or ... maybe it did? |
@msokolov I am not sure, I noticed it in ES: Maybe we just exit early there (which I think we will do, e.g. InMemoryIndex just cannot have kNN happen), but it seems to me better to fix it here as well. |
I guess what I'm wondering is does this point a testing gap in Lucene? I thought that we would randomize the index directory type when running tests ... |
Looking at MemoryIndex:
So, returning no hits and forcing exact (or the caller to change), is the way. But we should definitely not throw an NPE. I can add a test really quick to make sure this won't throw an NPE. |
* Fix potential NPE for KnnVectorQuery * adding test * removing unused test code
When using a particular leaf reader (possibly for a MemoryIndex),
parent
can benull
.Let's verify that and prevent an NPE