-
Notifications
You must be signed in to change notification settings - Fork 114
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 few bugs on binary index with Faiss HNSW #1850
Conversation
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! The integration tests for these cases will be in 2.16 right?
@@ -190,6 +190,14 @@ public static ValidationException validateKnnField( | |||
return exception; | |||
} | |||
|
|||
String vectorDataType = (String) fieldMap.get(VECTOR_DATA_TYPE_FIELD); |
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.
can fieldMap.get(VECTOR_DATA_TYPE_FIELD)
be 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.
It can be 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.
if it can be null then the type casting to string will cause a NPE. Please handle this gracefully.
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.
It won't cause a NPE.
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.
why?? casting a null to string cause NPE. I am missing something here.
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 tested it and it didn't cause NPE. String is of Object and String can be null.
@@ -190,6 +190,14 @@ public static ValidationException validateKnnField( | |||
return exception; | |||
} | |||
|
|||
String vectorDataType = (String) fieldMap.get(VECTOR_DATA_TYPE_FIELD); | |||
if (VectorDataType.BINARY.toString().equalsIgnoreCase(vectorDataType)) { |
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.
we should convert the datatype to enum and then use ==
in if
condition
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.
Because vectorDataType can be null, I choose this approach. Why should we convert it to the data type?
FYI, this line of code will be removed once we support binary index for IVF.
src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java
Outdated
Show resolved
Hide resolved
@heemin32 Can you provide intended behavior for the bugs as well? |
Will try. As the last resort, will do the manual test after code freeze once more. |
Added. |
Signed-off-by: Heemin Kim <heemin@amazon.com>
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.
? knnQuery.getQueryVector().length | ||
: knnQuery.getByteQueryVector().length); |
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 don't like the fact that we have two vectors. This will create a lot of branching in the code. We should have gone with generics or something with a different type of query. Something similar to lucene.
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.
We are already aware of it and captured it in #1810
@heemin32 lets ensure that GH action are successful before we merge this PR |
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! Thanks
Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64)
opensearch-project#1856) Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64) Co-authored-by: Heemin Kim <heemin@amazon.com>
Description
Did a manual test on following items. Will raise a new PR implementing this as integ test.
Test case
Bug1 (2. Can create binary index with knn index as false)
Bug2 (3. Cannot create binary index using Lucene)
\"hnsw\" configuration does not support space type: \"hammingbit\"
which is not appropriate without mentioning what engine it uses.Bug 3. (7. Cannot run range query on binary index)
Bug 4 (11. Can run knn query with filtering on nested field with binary index)
Related Issues
N/A
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.