Conversation
|
@CLiqing 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
| continue; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please confirm this — it looks like it should be an else?
| return Status::success; | ||
| } | ||
| } | ||
|
|
src/index/hnsw/faiss_hnsw.cc
Outdated
| // Create IndexIVFRaBitQWrapper with METRIC_L2 (data will be normalized for cosine) | ||
| expected<std::unique_ptr<knowhere::IndexIVFRaBitQWrapper>> ivfrabitq_result; | ||
| if (is_cosine) { | ||
| // For cosine: use normalized data for IVF_RABITQ, L2 metric |
There was a problem hiding this comment.
What’s the reason for this？
| knowhere::feature::MMAP | knowhere::feature::MV | knowhere::feature::EMB_LIST) | ||
| KNOWHERE_SIMPLE_REGISTER_DENSE_FLOAT_ALL_GLOBAL(HNSW_RABITQ, BaseFaissRegularIndexHNSWRaBitQNodeTemplate, | ||
| knowhere::feature::MMAP | knowhere::feature::MV | | ||
| knowhere::feature::EMB_LIST) |
There was a problem hiding this comment.
I didn’t see the implementation of EMB_LIST — it might need to be added.
src/index/hnsw/faiss_hnsw.cc
Outdated
| if (wrapper_storage != nullptr) { | ||
| auto* ivfrabitq_idx = wrapper_storage->get_ivfrabitq_index(); | ||
| if (ivfrabitq_idx != nullptr) { | ||
| ivfrabitq_idx->qb = qb; |
There was a problem hiding this comment.
Modification behavior shouldn’t occur within the const search interface.
|
Please add some unit tests to ensure the logic runs correctly. |
| if (hnsw_index != nullptr && hnsw_index->storage != nullptr) { | ||
| // The storage was serialized as the internal faiss::Index | ||
| // Now wrap it back in IndexHNSWRaBitQWrapper (specialized for HNSW) | ||
| std::unique_ptr<faiss::Index> storage_index(hnsw_index->storage); |
src/index/hnsw/faiss_hnsw.cc
Outdated
| if (hnsw_index != nullptr) { | ||
| auto* wrapper_storage = dynamic_cast<knowhere::IndexIVFRaBitQWrapper*>(hnsw_index->storage); | ||
| if (wrapper_storage != nullptr && wrapper_storage->index != nullptr) { | ||
| // Temporarily replace storage with the internal index for serialization |
There was a problem hiding this comment.
Please add a comment here explaining why “Temporarily replace“ is needed.
|
|
||
| // RaBitQ Params | ||
| constexpr const char* RABITQ_QUERY_BITS = "rbq_bits_query"; | ||
| constexpr const char* RBQ_QUERY_NBITS = "rbq_query_nbits"; // for HNSW_RABITQ |
There was a problem hiding this comment.
Why is it necessary to create a new one instead of directly using rbq_bits_query?
| knowhere::feature::MMAP | knowhere::feature::MV | | ||
| knowhere::feature::EMB_LIST) | ||
| KNOWHERE_SIMPLE_REGISTER_DENSE_INT_GLOBAL(HNSW_RABITQ, BaseFaissRegularIndexHNSWRaBitQNodeTemplate, | ||
| knowhere::feature::MMAP | knowhere::feature::MV | knowhere::feature::EMB_LIST) |
There was a problem hiding this comment.
https://github.com/zilliztech/knowhere/blob/main/include/knowhere/index/index_table.h
Newly added indexes need to be registered in the IndexTable.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CLiqing, marcelo-cjl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Cliqing <2208529306@qq.com>
4 participants
Implement HNSW_RABITQ
kind/feature