[feat] Add sparse index support to knowhere #341

zhengbuqian · 2024-01-10T12:48:37Z

/kind feature
#193

This PR adds the basic framework of supporting sparse index in knowhere, with 2 index types: inverted index and wand.

In this PR only Search and GetVectorByIds is supported.

Added basic unit tests:

Search without filtering, with different dataset sizes/recall/sparisity, etc.
Search with filtering, with different filter rate.

sre-ci-robot · 2024-01-10T12:48:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhengbuqian

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~~ [zhengbuqian]

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

alexanderguzhva · 2024-01-10T16:36:37Z

src/index/sparse/sparse_inverted_index.h

+        for (size_t i = 0; i < rows; ++i) {
+            raw_data_[current_rows + i] = data[i];
+            for (auto [idx, val] : data[i].data) {
+                if (val < val_threshold) {


if drop_ratio_build_ == 0, then val_threshold is 0, so negative val values will be skipped. Is it by design? This implies that sparse values are expected to be positive only

Thanks Alex! I have changed it so that now we ignore only values that are close enough to zero.

Also the drop strategy has been modified from "drop any value that is below ratio * global max" to "drop the smallest ratio% abs values".

zhengbuqian · 2024-01-17T07:31:40Z

@alexanderguzhva @hhy3 @liliu-z PTAL

alexanderguzhva · 2024-01-18T00:29:08Z

include/knowhere/sparse_utils.h

+
+    SparseRow&
+    operator=(const SparseRow& other) {
+        copy_from(other);


Please do all these nasty C++ things like if (this == &other) here.
Also, typically it is recommended to create a clone object

SparseRow clone(other); swap(*this, clone);

just in case of an exception during the cloning process

alexanderguzhva · 2024-01-18T00:29:43Z

include/knowhere/sparse_utils.h

+        }
+    }
+
+    size_t


move constructor and move assignment operator please :)

alexanderguzhva · 2024-01-18T00:36:43Z

include/knowhere/sparse_utils.h

+
+    IdVal<T>
+    operator[](size_t i) const {
+        table_t index = *(table_t*)(data + i * (sizeof(table_t) + sizeof(T)));


all these raw pointers operations are a nightmare. Please introduce a helper struct, such as

template<typename T> struct SparseRowElement { table_t idx; T value; };

make sure that it is packed and just use reinterpret_cast<SparseRowElement<T>*>(data) or reinterpret_cast<const ...*>(data) for working .

Alternatively, just use IdVal<T> but make sure that it is packed.

Done, added a private structure.

alexanderguzhva · 2024-01-18T00:40:07Z

include/knowhere/sparse_utils.h

+
+// When pushing new elements into a MaxMinHeap, only `capacity` elements with the
+// largest val are kept. pop()/top() returns the smallest element out of them.
+template <typename T>


Don't we have any similar structures in our code already? I'm just curious, the implementation is fine

I don't see one that matches exactly the use case.

alexanderguzhva · 2024-01-18T00:42:36Z

src/index/sparse/sparse_inverted_index.h

+            return Status::success;
+        }
+
+        std::vector<T> vals;


just compute the size that needs to be preallocated, such as

size_t amount = 0; for (size_t i = 0; i < rows; i++) { amount += data[i].size(); } std::vector<T> vals(amount);

alexanderguzhva · 2024-01-18T00:44:33Z

src/index/sparse/sparse_inverted_index.h

+            if (v < q_threshold || i >= n_cols_internal()) {
+                continue;
+            }
+            for (size_t j = 0; j < inverted_lut_[i].size(); j++) {


this loop can be improved via SIMD. Maybe, later

great suggestion! I'll leave that as is for now but I left a TODO.

alexanderguzhva · 2024-01-18T00:48:10Z

src/index/sparse/sparse_inverted_index.h

+
+    // LUT supports size() and operator[] which returns an IdVal.
+    template <typename LUT>
+    class Cursor {


the design of the iterator is somewhat unusual, but it is fine

alexanderguzhva · 2024-01-18T00:53:42Z

src/index/sparse/sparse_inverted_index.h

+    void
+    Search(const SparseRow<T>& query, size_t k, float drop_ratio_search, float* distances, label_t* labels,
+           size_t refine_factor, const BitsetView& bitset) const {
+        std::shared_lock<std::shared_mutex> lock(mu_);


nit: you could move the lock of mu_ a little bit down further, because in the following lines you do nothing but std::fill and not using any of the fields of this struct

alexanderguzhva · 2024-01-18T17:21:34Z

include/knowhere/sparse_utils.h

+        table_t index;
+        T value;
+        ElementProxy() = delete;
+        ElementProxy(const ElementProxy&) = delete;


I would add ElementProxy& operator=(const ElementProxy&) = delete; as well
On the other hand, I don't understand why it is needed to delete the constructors

deleting the default and the copy constructor(so no constructor will be generated) should be sufficient. if no instance can ever be made there is no way to call copy assignment operator.

the reason is this element is used solely to help access what's in the raw data_ pointer, we don't actually have the need to create such objects.

alexanderguzhva · 2024-01-18T17:23:42Z

src/common/comp/brute_force.cc

+    for (auto& fut : futs) {
+        fut.wait();
+        RETURN_IF_ERROR(fut.result().value());
+    }


I'd use the following to make sure that no futs get unfinished

for (auto& fut : futs) fut.wait(); for (auto& fut : futs) RETURN_IF_ERROR(fut.result().value());

that is correct. Considering the lambda does not return a Status other than success, I removed the return value and use fut.get() instead.

there are actually other places that made the same mistake, creating another PR to fix

alexanderguzhva · 2024-01-18T17:29:32Z

src/common/comp/brute_force.cc

+        return expected<DataSetPtr>::Err(status, msg);
+    }
+    int topk = cfg.k.value();
+    auto labels = new sparse::label_t[nq * topk];


A potential memory leak. I'd use something like

auto labels = std::make_unique<sparse::label_t>(nq * topk); auto distances = std::make_unique<float>(nq * topk); SearchSparseWithBuf(base_dataset, query_dataset, labels.get(), distances.get(), config, bitset); return GenResultDataSet(nq, topk, labels.release(), distances.release());

If we're using C++ 17, of course

good catch!

alexanderguzhva · 2024-01-18T17:32:17Z

src/index/sparse/sparse_index_node.cc

+        auto refine_factor = cfg.refine_factor.value_or(10);
+        auto drop_ratio_search = cfg.drop_ratio_search.value_or(0.0f);
+
+        auto p_id = new sparse::label_t[nq * k];


same for using std::make_unique<> as above

alexanderguzhva · 2024-01-18T17:32:23Z

src/index/sparse/sparse_index_node.cc

+        for (auto& fut : futs) {
+            fut.wait();
+        }
+        return GenResultDataSet(nq, k, p_id, p_dist);


for (auto& fut : futs) RETURN_IF_ERROR(fut.result().value()); is missing

changed to fut.get()

I've added comments to #355 . Basically, it does not work as desired.

alexanderguzhva · 2024-01-19T16:34:01Z

/hold
#355-related comments need to be addressed first
but it looks good overall

alexanderguzhva · 2024-01-22T15:49:19Z

/unhold
folly's collectAll() works

alexanderguzhva · 2024-01-22T15:49:57Z

Please change for using collectAll() for futures and I'll lgtm the diff
Thanks

… added related unit test Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

hhy3 · 2024-01-24T06:19:03Z

/lgtm

sre-ci-robot added the kind/feature label Jan 10, 2024

sre-ci-robot requested a review from cqy123456 January 10, 2024 12:48

sre-ci-robot requested a review from foxspy January 10, 2024 12:48

sre-ci-robot added approved size/XXL labels Jan 10, 2024

zhengbuqian mentioned this pull request Jan 10, 2024

[feat] Add sparse index support to knowhere #199

Closed

mergify bot added the dco-passed label Jan 10, 2024

alexanderguzhva reviewed Jan 10, 2024

View reviewed changes

buqian-zilliz force-pushed the sparse-v2 branch from 3958730 to 884f2b6 Compare January 17, 2024 04:05

sre-ci-robot added the do-not-merge/hold label Jan 17, 2024

buqian-zilliz force-pushed the sparse-v2 branch 2 times, most recently from 345c673 to 950a375 Compare January 17, 2024 07:30

buqian-zilliz force-pushed the sparse-v2 branch from 950a375 to 4108734 Compare January 17, 2024 07:33

mergify bot added the ci-passed label Jan 17, 2024

alexanderguzhva reviewed Jan 18, 2024

View reviewed changes

mergify bot removed the ci-passed label Jan 18, 2024

alexanderguzhva reviewed Jan 18, 2024

View reviewed changes

buqian-zilliz force-pushed the sparse-v2 branch from b569ba7 to 1ee5dfb Compare January 19, 2024 08:18

sre-ci-robot removed the do-not-merge/hold label Jan 22, 2024

Add sparse index support to knowhere with linscan and wand algorithm;…

c0f90d3

… added related unit test Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

buqian-zilliz force-pushed the sparse-v2 branch from 1ee5dfb to c0f90d3 Compare January 24, 2024 02:40

mergify bot added the ci-passed label Jan 24, 2024

sre-ci-robot assigned hhy3 Jan 24, 2024

sre-ci-robot added the lgtm label Jan 24, 2024

sre-ci-robot merged commit 0f63f2d into zilliztech:main Jan 24, 2024
9 checks passed

zhengbuqian deleted the sparse-v2 branch January 24, 2024 06:33

+                      }
+                  }
+                  size_t

[feat] Add sparse index support to knowhere #341

[feat] Add sparse index support to knowhere #341

Conversation

zhengbuqian commented Jan 10, 2024

sre-ci-robot commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbuqian commented Jan 17, 2024

alexanderguzhva Jan 18, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderguzhva Jan 18, 2024 • 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 19, 2024 • edited Loading

alexanderguzhva commented Jan 22, 2024

alexanderguzhva commented Jan 22, 2024 • edited Loading

hhy3 commented Jan 24, 2024

alexanderguzhva Jan 18, 2024 •

edited

Loading

alexanderguzhva Jan 18, 2024 •

edited

Loading

alexanderguzhva commented Jan 19, 2024 •

edited

Loading

alexanderguzhva commented Jan 22, 2024 •

edited

Loading