Add HNSW changes to support Faiss byte vector #1823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
naveentatikonda
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
junqiu-lei,
martin-gaievski,
ryanbogan and
luyuncheng
as code owners
naveentatikonda
added
Features
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
3 times, most recently
from
5d532b0 to
374b81c
Compare
Vikasht34
reviewed
Jul 13, 2024
src/main/java/org/opensearch/knn/index/codec/transfer/VectorTransferByteToFloat.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/transfer/VectorTransferByteToFloat.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java
Outdated
Show resolved
Hide resolved
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
from
4391fa2 to
66ed657
Compare
navneet1v
requested changes
Jul 13, 2024
src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/transfer/VectorTransferByteToFloat.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/transfer/VectorTransferByteToFloat.java
Outdated
Show resolved
Hide resolved
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
4 times, most recently
from
68db524 to
1b344be
Compare
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
2 times, most recently
from
0e173b1 to
66ec044
Compare
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
2 times, most recently
from
5e0bea8 to
fa9af8d
Compare
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
3 times, most recently
from
b500228 to
1533a91
Compare
shatejas
approved these changes
Aug 21, 2024
jmazanec15
reviewed
Aug 22, 2024
src/main/java/org/opensearch/knn/index/codec/transfer/OffHeapBinaryVectorTransfer.java
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/mapper/MethodFieldMapper.java
Outdated
Show resolved
Hide resolved
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
from
1533a91 to
80a6494
Compare
Signed-off-by: Naveen Tatikonda <[email protected]>
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
from
80a6494 to
a48ddeb
Compare
navneet1v
reviewed
Aug 22, 2024
navneet1v
reviewed
Aug 22, 2024
| // If VectorDataType is Byte using Faiss engine then manipulate Index Description to use "SQ8_direct_signed" scalar quantizer | ||
| // For example, Index Description "HNSW16,Flat" will be updated as "HNSW16,SQ8_direct_signed" | ||
| String indexDescription = methodAsMapBuilder.indexDescription; | ||
| if (indexDescription != null && indexDescription.isEmpty() == false) { |
There was a problem hiding this comment.
why not just use StringUtils.isEmpty() from apache commons.
Also what will happen in the case indexDescription is null or empty. I think we should throw an exception rather than just silently not doing anything.
There was a problem hiding this comment.
Just a heads up - as part of the mode/compression support, Im going to adjust the default logic to provide kinda of a default provider as opposed to just a default. So we wont need this additional logic once thats changed, because we will be able to supply default quantizer based on data type.
There was a problem hiding this comment.
Updated to use StringUtils for now. But, I'm not throwing an exception for the else case as Jack mentioned that we will be refactoring it.
jmazanec15
reviewed
Aug 22, 2024
| append | ||
| ); | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
Should this return the vector address? Just thinking in append scenario
There was a problem hiding this comment.
Actually, we don't need to check if list is empty or not because we are calling this if list size is equal to transfer limit or during flush (where we are verifying if list is not empty)
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
2 times, most recently
from
08eca5d to
ec1570d
Compare
navneet1v
reviewed
Aug 23, 2024
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
from
ec1570d to
2b2435d
Compare
Signed-off-by: Naveen Tatikonda <[email protected]>
naveentatikonda
force-pushed
the
faiss-byte-vector-hnsw
branch
from
2b2435d to
3cd458b
Compare
vibrantvarun
approved these changes
Aug 23, 2024
navneet1v
approved these changes
Aug 23, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
* Add HNSW changes to support Faiss byte vector Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 59c312b)
naveentatikonda
added a commit
that referenced
this pull request
Aug 26, 2024
* Add HNSW changes to support Faiss byte vector Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 59c312b) Co-authored-by: Naveen Tatikonda <[email protected]>
akashsha1
pushed a commit
to akashsha1/k-NN
that referenced
this pull request
Sep 16, 2024
* Add HNSW changes to support Faiss byte vector Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> Signed-off-by: Akash Shankaran <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add HNSW changes to support Faiss byte vector which behind the scenes uses Faiss
SQ8_direct_signedscalar quantizer.Issues Resolved
#1659
Check List
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.