Support MV only for HNSW #1020

chasingegg · 2025-01-09T03:01:23Z

issue: #1019
/kind feature

codecov · 2025-01-09T04:56:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.81%. Comparing base (3c46f4c) to head (c4e3d10).
Report is 290 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #1020       +/-   ##
=========================================
+ Coverage      0   73.81%   +73.81%     
=========================================
  Files         0       82       +82     
  Lines         0     7488     +7488     
=========================================
+ Hits          0     5527     +5527     
- Misses        0     1961     +1961

see 82 files with indirect coverage changes

PwzXxm · 2025-01-10T03:27:19Z

include/knowhere/comp/index_param.h

@@ -95,6 +95,8 @@ constexpr const char* JSON_ID_SET = "json_id_set";
 constexpr const char* TRACE_ID = "trace_id";
 constexpr const char* SPAN_ID = "span_id";
 constexpr const char* TRACE_FLAGS = "trace_flags";
+constexpr const char* SCALAR_INFO = "scalar_info";
+constexpr const char* MV_ONLY_ENABLED = "mv_only_enabled";


Can we rely on SCALAR_INFO empty or not to indicate whether we enable mv_only?

there is a hidden config partition_key_isolation in cardinal

This config is only used in UT, could remove it

PwzXxm · 2025-01-10T03:50:56Z

src/index/hnsw/faiss_hnsw.cc

    }

    Status
    Serialize(BinarySet& binset) const override {
-        if (index == nullptr) {
+        if (indexes.empty()) {


Maybe wrap 173-180 to an Empty() func?

PwzXxm · 2025-01-10T03:51:19Z

src/index/hnsw/faiss_hnsw.cc

            return Status::empty_index;
        }
+        for (auto& index : indexes) {


Maybe const here?

PwzXxm · 2025-01-10T03:56:43Z

src/index/hnsw/faiss_hnsw.cc


        try {
            MemoryIOWriter writer;
-            faiss::write_index(index.get(), &writer);
+            if (indexes.size() > 1) {


The only diffs are write_mv and writeHeader?

PwzXxm · 2025-01-10T05:42:20Z

src/index/hnsw/faiss_hnsw.cc

-        if (this->index == nullptr) {
-            LOG_KNOWHERE_ERROR_ << "Can not add data to an empty index.";
-            return Status::empty_index;
+    // std::shared_ptr<faiss::Index> index;


PwzXxm · 2025-01-10T07:58:31Z

src/index/hnsw/faiss_hnsw.cc

        }
+        size_t first_valid_index = bitset.get_first_valid_index();
+        auto it = std::lower_bound(index_rows_sum.begin(), index_rows_sum.end(),
+                                   label_to_internal_offset[first_valid_index] + 1);


Do we need +1 here?

Yes, offset starts from 0 but rows starts from 1

PwzXxm · 2025-01-10T08:25:54Z

src/index/hnsw/faiss_hnsw.cc

@@ -1220,10 +1535,16 @@ class BaseFaissRegularIndexHNSWNode : public BaseFaissRegularIndexNode {
    expected<std::vector<IndexNode::IteratorPtr>>
    AnnIterator(const DataSetPtr dataset, std::unique_ptr<Config> cfg, const BitsetView& bitset,
                bool use_knowhere_search_pool) const override {
-        if (index == nullptr) {
-            LOG_KNOWHERE_WARNING_ << "creating iterator on empty index";
+        if (this->indexes.empty()) {


PwzXxm · 2025-01-10T09:12:27Z

src/index/hnsw/faiss_hnsw.cc

+        index_rows_sum.resize(tmp_combined_scalar_ids.size() + 1);
+        labels.resize(tmp_combined_scalar_ids.size());
+        indexes.resize(tmp_combined_scalar_ids.size());
+        for (auto i = 0; i < tmp_combined_scalar_ids.size(); ++i) {


This logic is similar in PQ/PRQ, Train/TrainInternal, is it possible to abstract the common?

alexanderguzhva · 2025-01-12T20:44:01Z

src/index/hnsw/faiss_hnsw.cc

@@ -319,6 +428,48 @@ static constexpr DataFormatEnum datatype_v = DataType2EnumHelper<T>::value;

 namespace {

+bool
+convert_rows_to_fp32(const void* const __restrict src_in, float* const __restrict dst,


please add __restrict to pointers in order to help the compiler

alexanderguzhva · 2025-01-12T20:46:40Z

src/index/hnsw/faiss_hnsw.cc

+                indexes_to_reconstruct_from[0]->reconstruct(id, result);
+            } else {
+                auto it =
+                    std::lower_bound(index_rows_sum.begin(), index_rows_sum.end(), label_to_internal_offset[id] + 1);


any checks for it pointing to lower_bound() returning empty results?

alexanderguzhva · 2025-01-12T20:53:04Z

include/knowhere/bitsetview_idselector.h


-    inline BitsetViewIDSelector(BitsetView bitset_view, const size_t offset = 0)
-        : bitset_view{bitset_view}, id_offset(offset) {
+    inline BitsetViewIDSelector(BitsetView bitset_view, const size_t offset = 0,


I would to it differently. I'd add a new class which is derived from faiss::IDSelector, which is a special case of what you wrote, which includes out_id_mapping field that is guaranteed to be non-null. I'd keep BitsetViewIDSelector as it was. And I'd modify faiss::IDSelector* id_selector = (bitset.empty()) ? nullptr : &bw_idselector; line (Search() call) in faiss_hnsw.cc correspondingly to allow this new BitsetViewWithMappingIDSelector to be used

alexanderguzhva · 2025-01-12T20:57:59Z

src/index/hnsw/faiss_hnsw.cc

+        labels.resize(tmp_combined_scalar_ids.size());
+        indexes.resize(tmp_combined_scalar_ids.size());
+        const void* data = dataset->GetTensor();
+        for (auto i = 0; i < tmp_combined_scalar_ids.size(); ++i) {


is this block the same in multiple functions down below?

alexanderguzhva · 2025-01-12T21:00:17Z

thirdparty/faiss/faiss/index_io.h

@@ -97,6 +97,15 @@ InvertedLists* read_InvertedLists(IOReader* reader, int io_flags = 0);
 // for backward compatibility
 Index *read_index_nm(IOReader *f, int io_flags = 0);
 void write_index_nm(const Index* idx, IOWriter* writer);
+
+// additional helper function


please add a comment that these are knowhere-specific functions

alexanderguzhva · 2025-01-13T22:43:14Z

/lgtm overall

Signed-off-by: chasingegg <[email protected]>

PwzXxm · 2025-01-14T08:46:42Z

/lgtm
/approve

sre-ci-robot · 2025-01-14T08:46:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chasingegg, PwzXxm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [PwzXxm,chasingegg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sre-ci-robot added the kind/feature label Jan 9, 2025

sre-ci-robot requested review from foxspy and Presburger January 9, 2025 03:01

sre-ci-robot added approved size/XXL labels Jan 9, 2025

mergify bot added the dco-passed label Jan 9, 2025

chasingegg force-pushed the support-mv branch 2 times, most recently from 96d5ecc to b3f6a57 Compare January 9, 2025 03:29

mergify bot added the ci-passed label Jan 9, 2025

chasingegg force-pushed the support-mv branch from b3f6a57 to 7bcfa6a Compare January 9, 2025 12:05

mergify bot added ci-passed and removed ci-passed labels Jan 9, 2025

PwzXxm reviewed Jan 10, 2025

View reviewed changes

src/index/hnsw/faiss_hnsw.cc Outdated

return Status::empty_index;

}

for (auto& index : indexes) {

Copy link

Collaborator

PwzXxm Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe const here?

PwzXxm reviewed Jan 10, 2025

View reviewed changes

alexanderguzhva reviewed Jan 12, 2025

View reviewed changes

chasingegg force-pushed the support-mv branch from 7bcfa6a to 7253ece Compare January 13, 2025 07:21

mergify bot removed the ci-passed label Jan 13, 2025

chasingegg force-pushed the support-mv branch 5 times, most recently from b45f69b to d416ba2 Compare January 13, 2025 10:07

mergify bot added the ci-passed label Jan 13, 2025

chasingegg force-pushed the support-mv branch from d416ba2 to 1f4f4f7 Compare January 13, 2025 12:35

mergify bot removed the ci-passed label Jan 13, 2025

chasingegg force-pushed the support-mv branch from 1f4f4f7 to 0fdff8d Compare January 13, 2025 12:48

mergify bot added the ci-passed label Jan 13, 2025

chasingegg force-pushed the support-mv branch from 0fdff8d to 6dec4cf Compare January 14, 2025 04:21

mergify bot removed the ci-passed label Jan 14, 2025

chasingegg force-pushed the support-mv branch from 6dec4cf to c41622c Compare January 14, 2025 05:32

Support MV only for HNSW

c4e3d10

Signed-off-by: chasingegg <[email protected]>

chasingegg force-pushed the support-mv branch from c41622c to c4e3d10 Compare January 14, 2025 06:18

mergify bot added the ci-passed label Jan 14, 2025

sre-ci-robot assigned PwzXxm Jan 14, 2025

sre-ci-robot added the lgtm label Jan 14, 2025

sre-ci-robot merged commit 2a2f5ef into zilliztech:main Jan 14, 2025
13 of 14 checks passed

chasingegg deleted the support-mv branch January 14, 2025 08:58

chasingegg mentioned this pull request Jan 16, 2025

Add IsAdditionalScalarSupported check for mv_only #1036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MV only for HNSW #1020

Support MV only for HNSW #1020

chasingegg commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

PwzXxm Jan 10, 2025 •

edited

Loading

PwzXxm Jan 10, 2025

chasingegg Jan 10, 2025

PwzXxm Jan 10, 2025

PwzXxm Jan 10, 2025

PwzXxm Jan 10, 2025 •

edited

Loading

chasingegg Jan 13, 2025

PwzXxm Jan 10, 2025

PwzXxm Jan 10, 2025

chasingegg Jan 13, 2025

PwzXxm Jan 10, 2025

PwzXxm Jan 10, 2025

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

chasingegg Jan 13, 2025

alexanderguzhva commented Jan 13, 2025

PwzXxm commented Jan 14, 2025

sre-ci-robot commented Jan 14, 2025

Support MV only for HNSW #1020

Support MV only for HNSW #1020

Conversation

chasingegg commented Jan 9, 2025

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

PwzXxm Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PwzXxm Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderguzhva commented Jan 13, 2025

PwzXxm commented Jan 14, 2025

sre-ci-robot commented Jan 14, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

PwzXxm Jan 10, 2025 •

edited

Loading

PwzXxm Jan 10, 2025 •

edited

Loading