[feat] Add sparse index support to knowhere by zhengbuqian · Pull Request #199 · zilliztech/knowhere

zhengbuqian · 2023-11-17T10:11:20Z

/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 is supported.

Support for growing segment is sub-optimal, we naively double the underlying data structure each time we used up the current space.

Added basic unit tests:

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

sre-ci-robot · 2023-11-17T10:11:24Z

[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

Details

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

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

Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

alexanderguzhva · 2023-11-20T17:24:18Z

include/knowhere/utils.h

+    }
+    table_t
+    pop() {
+        std::pop_heap(pool_.begin(), pool_.begin() + size_--);


Please

std::pop_heap(pool_.begin(), pool_.begin() + size_); size_ -= 1;

alexanderguzhva · 2023-11-20T17:25:38Z

src/common/comp/brute_force.cc

+                        dist += it->second * data[k];
+                    }
+                }
+                if (dist > 0) {


the code does not distinguish two very different situations:

dist=0 because no item was found in query

dist=0 because the match is exact

only IP is supported thus dist=0 doesn't mean exact match?

alexanderguzhva · 2023-11-20T17:29:03Z

src/common/comp/brute_force.cc

+            int result_size = heap.size();
+            for (int64_t j = result_size - 1; j >= 0; --j) {
+                cur_labels[j] = heap.top().id;
+                cur_distances[j] = -heap.top().distance;


working with negative distances twice... I'd rather change MinMaxHeap implementation

alexanderguzhva · 2023-11-20T17:32:30Z

src/index/sparse/sparse_inverted_index.h

+        }
+        std::sort(q_vec.begin(), q_vec.end(),
+                  [](const auto& lhs, const auto& rhs) { return std::abs(lhs.second) > std::abs(rhs.second); });
+        while (q_vec.size() && q_vec[0].second * drop_ratio_search > q_vec.back().second) {


while (q_vec.size() && ...) -> while (!q_vec.empty() && ...)

alexanderguzhva · 2023-11-20T17:35:09Z

src/index/sparse/sparse_inverted_index.h

+
+ private:
+    [[nodiscard]] float
+    dot_product(std::unordered_map<IndicesT, T> q_map, table_t u) const {


dot_product(const std::unordered_map<IndicesT, T>& q_map, table_t u) const {

alexanderguzhva · 2023-11-20T17:43:00Z

src/index/sparse/sparse_inverted_index.h

+    }
+
+    void
+    refine_and_collect(std::unordered_map<IndicesT, T>& q_map, MinMaxHeap<T> inaccurate, size_t k, float* distances,


void refine_and_collect(const std::unordered_map<IndicesT, T>& q_map, MinMaxHeap<T>& inaccurate, size_t k, float* distances, label_t* labels) const

alexanderguzhva · 2023-11-20T17:45:25Z

src/index/sparse/sparse_inverted_index.h

+        if (refine_factor == 1) {
+            collect_result(heap, distances, labels);
+        } else {
+            std::unordered_map<IndicesT, T> q_map;


the size of the q_map is known beforehand (it is len). So, please construct q_map with the provided number of buckets (I'd try 4 * len as a start), so

std::unordered_map<IndicesT, T> q_map(4 * len);

The number of buckets impacts the performance of the following dot_product()

Or I can tweak the performance as soon as this commit lands, there are many things to play with there :)

thanks! I am using 4*len as a start and leaving a TODO. Focusing on the functionality for this PR.

alexanderguzhva · 2023-11-20T17:52:33Z

src/index/sparse/sparse_inverted_index.h

+
+    template <typename HeapType>
+    void
+    collect_result(HeapType heap, float* distances, label_t* labels) const {


HeapType& heap

alexanderguzhva · 2023-11-20T17:54:53Z

src/index/sparse/sparse_inverted_index.h

+            }
+        }
+        Cursor(const Cursor& rhs) = delete;
+        Cursor(Cursor&& rhs) noexcept {


Won't Cursor(Cursor&& rhs) noexcept = default; work?
Same question for Cursor& operator=(Cursor&& rhs) noexcept

I am changing lut_ to be a const ref member, move constructor, move assignment, copy assignment will now be deleted, and I am still explicitly deleting the copy constructor as we don't currently have a usage.

alexanderguzhva · 2023-11-20T17:55:14Z

src/index/sparse/sparse_inverted_index.h

+            swap(lhs.q_value_, rhs.q_value_);
+            // all cursors share the same bitset
+        }
+        const std::vector<Neighbor<T>>* lut_;


const std::vector<Neighbor<T>>* lut_ = nullptr;
Also, using vector<>* leads to double pointer dereferencing, unless the compiler optimizes this away

Good point! I am removing the swap method and changing lut_ to const std::vector<Neighbor<T>>&.

alexanderguzhva · 2023-11-20T17:57:19Z

src/index/sparse/sparse_inverted_index.h

+        size_t num_vec_ = 0;
+        float max_score_ = 0.0f;
+        float q_value_ = 0.0f;
+        const BitsetView* bitset_;


BitsetView is a lightweight structure (a wrapper around the pointer, basically) that should be passed by value.

…to const ref; update multiple InvertedIndex member functions to accept reference type for container types; addressed other review comments Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

alexanderguzhva · 2023-11-21T23:09:23Z

the functionality seems to be fine, but what about the unit tests running time, is it expected?

hhy3 · 2023-11-22T02:47:58Z

src/index/sparse/sparse_inverted_index.h

+
+ private:
+    [[nodiscard]] float
+    dot_product(const std::unordered_map<IndicesT, T>& q_map, table_t u) const {


std::unordered_map seems with a bad performance as I tested. It's better to use two arrays indices and data

…compute refined distance. Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

zhengbuqian · 2023-11-22T09:03:32Z

the functionality seems to be fine, but what about the unit tests running time, is it expected?

I added some large unit tsets so ut took quite long, I have removed them. Will revert the change in ut yaml before merge.

Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

zhengbuqian · 2023-11-22T15:58:49Z

/run-e2e

zhengbuqian · 2023-11-22T15:59:25Z

run-e2e

alexanderguzhva · 2023-11-22T22:41:59Z

/lgtm overall

yellow-shine · 2023-11-23T03:44:08Z

/run-e2e

yellow-shine · 2023-11-23T03:57:08Z

/run-e2e

zhengbuqian · 2023-11-24T09:18:32Z

/hold

zhengbuqian · 2024-01-10T12:49:01Z

Use #341 instead

sre-ci-robot added the kind/feature label Nov 17, 2023

sre-ci-robot requested a review from cydrain November 17, 2023 10:11

sre-ci-robot requested a review from PwzXxm November 17, 2023 10:11

sre-ci-robot added approved size/XXL labels Nov 17, 2023

mergify bot added the dco-passed label Nov 17, 2023

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

26a6b8e

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

buqian-zilliz force-pushed the sparse-v1 branch from 5f21faa to 26a6b8e Compare November 17, 2023 10:16

Rely on std::vector to resize the array for storing the dim-val pairs

936826a

Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

alexanderguzhva reviewed Nov 20, 2023

View reviewed changes

Use MaxMinHeap to track scores to avoid negation; Change Cursor.lut_ …

460800d

…to const ref; update multiple InvertedIndex member functions to accept reference type for container types; addressed other review comments Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

hhy3 reviewed Nov 22, 2023

View reviewed changes

In sparse vector index, sort per row indices and use two pointers to …

853e414

…compute refined distance. Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

sre-ci-robot added the do-not-merge/hold label Nov 22, 2023

Revert ut timeout to 90 mins

1694c8f

Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>

sre-ci-robot removed the do-not-merge/hold label Nov 22, 2023

mergify bot added the ci-passed label Nov 22, 2023

mergify bot removed the ci-passed label Nov 22, 2023

mergify bot added ci-passed and removed ci-passed labels Nov 22, 2023

mergify bot added the ci-passed label Nov 23, 2023

sre-ci-robot added the do-not-merge/hold label Nov 24, 2023

mergify bot removed the ci-passed label Dec 4, 2023

zhengbuqian closed this Jan 10, 2024

zhengbuqian deleted the sparse-v1 branch March 14, 2024 08:03

mergify bot added needs-dco and removed dco-passed labels Apr 24, 2024

Conversation

zhengbuqian commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-ci-robot commented Nov 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderguzhva Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderguzhva Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderguzhva Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderguzhva commented Nov 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhengbuqian commented Nov 22, 2023

Uh oh!

zhengbuqian commented Nov 22, 2023

Uh oh!

zhengbuqian commented Nov 22, 2023

Uh oh!

alexanderguzhva commented Nov 22, 2023

Uh oh!

yellow-shine commented Nov 23, 2023

Uh oh!

yellow-shine commented Nov 23, 2023

Uh oh!

zhengbuqian commented Nov 24, 2023

Uh oh!

zhengbuqian commented Jan 10, 2024

Uh oh!

Reviewers

Assignees

zhengbuqian commented Nov 17, 2023 •

edited

Loading

alexanderguzhva Nov 20, 2023 •

edited

Loading

alexanderguzhva Nov 20, 2023 •

edited

Loading

alexanderguzhva Nov 20, 2023 •

edited

Loading