Replace Map<String,Object> with IntObjectHashMap for KnnVectorsReader #13763
Conversation
| @@ -223,7 +224,8 @@ public void checkIntegrity() throws IOException { | |||
|
|
|||
| @Override | |||
| public FloatVectorValues getFloatVectorValues(String field) throws IOException { | |||
| FieldEntry fieldEntry = fields.get(field); | |||
| final FieldInfo info = fieldInfos.fieldInfo(field); | |||
There was a problem hiding this comment.
couldn't final FieldInfo info be null? (we should check for null, see #13641 which should be merged soon).
There was a problem hiding this comment.
Yes, this is also what I want to discuss. You can ref to my comment below, if we all agree to check for null, I can fix this.
There was a problem hiding this comment.
In my opinion, the most important thing is to remove the leniency we have in some places, like if (fieldEntry == null) return EMPTY_VECTOR_VALUES, which could hide bugs. Checking for null explicitly is not required, though it's obviously nice.
| if (knnVectorsReader == null) { | ||
| final FieldInfo info = fieldInfos.fieldInfo(field); | ||
| KnnVectorsReader reader; | ||
| if (info == null || (reader = fields.get(info.number)) == null) { | ||
| return null; |
There was a problem hiding this comment.
According to the Javadoc of KnnVectorsReader#getFloatVectorValues:
Returns the FloatVectorValues for the given field. The behavior is undefined if the given field doesn't have KNN vectors enabled on its FieldInfo. The return value is never null.
Maybe we should return an empty instance if the target field doesn't exists in the segment or we should correct the Javadoc.
There was a problem hiding this comment.
The javadocs are correct. AssertingKnnVectorsReader checks for this.
There was a problem hiding this comment.
forgive me if i misunderstand something, but the javadoc says that the return value is never null, and we do return null in PerFieldKnnVectorsFormat. If we check the field info in caller, AssertingKnnVectorsReader will always get the non-null value from the delegate.
There was a problem hiding this comment.
It's probably not worded as well as it could, it wants to say that the return value is never null when the field has vector enabled on its field infos.
There was a problem hiding this comment.
These checks on info==null and reader == null are not hurting, but they are not needed either since KnnVectorsFormat#getFloatVectorValues should not be called when the field doesn't have vectors enabled on its infos.
There was a problem hiding this comment.
Unfortunately, the current implementation will throw a NullPointerException if we don't perform a check. We may address this in a followup PR, if desired.
|
@benwtrent @jpountz I have merged main branch into this one, can I get a review on this? |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
|
Yes, let's add this in 10.1. |
|
Would you like to make this PR up-to-date and open a new one for the other change (a cherry-pick isn't clean due to other changes)? |
@jpountz sure, I‘d be happy to do this. |
|
@jpountz I have merged main branch, PTAL:) |
Description
Following up on #13686.