enhance: knowhere support data view index node #1016

cqy123456 · 2025-01-08T06:32:53Z

issue: #909

sre-ci-robot · 2025-01-08T06:32:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cqy123456

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

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

mergify · 2025-01-08T06:33:31Z

@cqy123456 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

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!.

include/knowhere/comp/index_param.h

foxspy · 2025-01-08T06:43:50Z

include/knowhere/object.h

 template <typename T>
 class Pack : public Object {
-    static_assert(std::is_same_v<T, std::shared_ptr<knowhere::FileManager>>,
+    static_assert(std::is_same_v<T, std::shared_ptr<knowhere::FileManager>> || std::is_same_v<T, knowhere::ViewDataOp>,
                  "IndexPack only support std::shared_ptr<knowhere::FileManager> by far.");


This message should be updated

src/index/data_view_dense_index/data_view_dense_index.h

src/index/ivf/ivf.cc

src/index/data_view_dense_index/data_view_dense_index.h

foxspy · 2025-01-08T08:47:09Z

src/index/data_view_dense_index/data_view_dense_index.h

+     * @param range_filter
+     */
+    virtual RangeSearchResult
+    RangeSearchWithIds(const idx_t n, const void* x, const idx_t* ids_num_lims, const idx_t* ids, const float radius,


src/index/data_view_dense_index/index_node_with_data_view_refiner.h

include/knowhere/object.h

src/index/data_view_dense_index/data_view_dense_index.h

chasingegg · 2025-01-10T09:11:55Z

src/index/data_view_dense_index/data_view_index_config.h

+namespace knowhere {
+class IndexWithDataViewRefinerConfig : public ScannConfig {
+ public:
+    CFG_INT reorder_k;


this config is in scannconfig, could be excluded here

chasingegg · 2025-01-10T09:25:57Z

src/index/ivf/ivf.cc

@@ -1261,4 +1261,7 @@ KNOWHERE_MOCK_REGISTER_DENSE_FLOAT_ALL_GLOBAL(IVF_SQ8, IvfIndexNode, knowhere::f
 KNOWHERE_MOCK_REGISTER_DENSE_FLOAT_ALL_GLOBAL(IVF_SQ_CC, IvfIndexNode, knowhere::feature::NONE,
                                              faiss::IndexIVFScalarQuantizerCC)

+// faiss index + data view refiner combination
+KNOWHERE_SIMPLE_REGISTER_DENSE_FLOAT_ALL_GLOBAL(SCANN_DVR, IndexNodeWithDataViewRefiner, knowhere::feature::NONE,
+                                                IvfIndexNode<fp32, faiss::IndexScaNN>)


do we need IVFFLAT and IVFSQ?

we can all more xxx_with_data_view_refiner index in the future.

chasingegg · 2025-01-10T09:35:12Z

include/knowhere/comp/index_param.h

@@ -118,7 +119,7 @@ constexpr const char* WITH_RAW_DATA = "with_raw_data";
 constexpr const char* ENSURE_TOPK_FULL = "ensure_topk_full";
 constexpr const char* CODE_SIZE = "code_size";
 constexpr const char* RAW_DATA_STORE_PREFIX = "raw_data_store_prefix";
-
+constexpr const char* SUB_DIM = "sub_dim";


we could use m like PQ and not introduce a new config, but m is slightly different from sub_dim

Using this parameter is a bit like the meaning of diskann's pq_code_budget_gb, and it is ealier to maintain a constant memory/(raw data size) ratio for different dim in milvus.

codecov · 2025-01-12T19:14:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.00%. Comparing base (3c46f4c) to head (b768594).
Report is 291 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #1016       +/-   ##
=========================================
+ Coverage      0   73.00%   +73.00%     
=========================================
  Files         0       86       +86     
  Lines         0     8138     +8138     
=========================================
+ Hits          0     5941     +5941     
- Misses        0     2197     +2197

see 86 files with indirect coverage changes

include/knowhere/utils.h

src/common/utils.cc

src/index/data_view_dense_index/data_view_dense_index.h

alexanderguzhva · 2025-01-12T20:18:07Z

src/index/data_view_dense_index/data_view_dense_index.h

+        // use copy to avoid concurrent add and search
+        std::shared_lock lock(norms_mutex_);
+        base_norms = std::shared_ptr<float[]>(new float[norms_.size()]);
+        std::memcpy(base_norms.get(), norms_.data(), sizeof(float) * norms_.size());


just to clarify: a single query with n == 1 leads to copying of all norms?

src/index/data_view_dense_index/data_view_dense_index.h

alexanderguzhva · 2025-01-12T20:21:24Z

src/index/data_view_dense_index/data_view_dense_index.h

+            if (is_cosine_) {
+                for (auto j = 0; j < base_n; j++) {
+                    if (base_ids[j] != -1) {
+                        std::shared_lock lock(norms_mutex_);


putting a lock here could impose a very hardcore overhead. Isn't it better to put it around for-loop or clone norms_?

alexanderguzhva · 2025-01-12T20:35:49Z

thirdparty/faiss/faiss/IndexIVFFastScan.h

@@ -88,6 +89,8 @@ struct IndexIVFFastScan : IndexIVF {
    // // todo aguzhva: get rid of this
    std::vector<float> norms;

+    std::shared_ptr<std::shared_mutex> mutex = nullptr;


Why this cannot be resolved by surrounding a corresponding faiss index with a mutex rather than adding a mutex as a field? Technically, adding such a mutex implies a lot of unit testing and thorough analysis of the whole class

foxspy · 2025-01-13T07:10:33Z

thirdparty/faiss/faiss/IndexIVFFastScan.cpp

@@ -361,6 +364,7 @@ void IndexIVFFastScan::search_preassigned(
        bool store_pairs,
        const IVFSearchParameters* params,
        IndexIVFStats* stats) const {
+    std::shared_lock<std::shared_mutex> lock(*mutex.get());


check for range_search and iterator

include/knowhere/comp/rw_lock.h

src/index/data_view_dense_index/data_view_dense_index.h

src/index/data_view_dense_index/data_view_index_config.h

alexanderguzhva · 2025-01-15T00:47:04Z

src/index/data_view_dense_index/index_node_with_data_view_refiner.h

+
+namespace {
+constexpr int64_t kBatchSize = 4096;
+constexpr int64_t kMaxTrainSize = 5000;


this is for training what exactly? 5k points seems to be a bit small

DVR index will only uses in milvus growing segment, and the row count of growing segment is not very large.

The difference may be large for different dims, so dynamic parameters may need to be considered here.

why different dims need different train row count?

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

foxspy · 2025-01-15T08:41:18Z

/hold wait v2.5.4 release

sre-ci-robot requested review from cydrain and Presburger January 8, 2025 06:32

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

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

mergify bot added the do-not-merge/missing-related-issue label Jan 8, 2025

cqy123456 force-pushed the data-view-index branch from 4ed53dd to c488c94 Compare January 8, 2025 06:35

cqy123456 changed the title ~~enhance: knowhere support data view index~~ enhance: knowhere support data view index node Jan 8, 2025

cqy123456 force-pushed the data-view-index branch 2 times, most recently from 669d387 to d9dc4cb Compare January 8, 2025 06:53

foxspy reviewed Jan 8, 2025

View reviewed changes

cqy123456 force-pushed the data-view-index branch 2 times, most recently from 1667fc3 to d4ff301 Compare January 8, 2025 13:47

chasingegg reviewed Jan 10, 2025

View reviewed changes

cqy123456 force-pushed the data-view-index branch 2 times, most recently from cb686ab to d3b1a95 Compare January 12, 2025 17:53

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

alexanderguzhva reviewed Jan 12, 2025

View reviewed changes

foxspy reviewed Jan 13, 2025

View reviewed changes

cqy123456 force-pushed the data-view-index branch from d3b1a95 to 8f30983 Compare January 14, 2025 01:07

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

cqy123456 force-pushed the data-view-index branch 2 times, most recently from 0b71169 to 506348a Compare January 14, 2025 06:58

foxspy reviewed Jan 14, 2025

View reviewed changes

include/knowhere/comp/rw_lock.h Show resolved Hide resolved

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

cqy123456 force-pushed the data-view-index branch from 506348a to dbbc39e Compare January 14, 2025 10:00

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

cqy123456 force-pushed the data-view-index branch 2 times, most recently from c3b89ea to 505699f Compare January 14, 2025 11:14

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

alexanderguzhva reviewed Jan 15, 2025

View reviewed changes

include/knowhere/comp/rw_lock.h Show resolved Hide resolved

alexanderguzhva reviewed Jan 15, 2025

View reviewed changes

cqy123456 force-pushed the data-view-index branch from 505699f to 2e306a3 Compare January 15, 2025 06:31

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

enhance: knowhere support data view index node

b768594

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

cqy123456 force-pushed the data-view-index branch from 2e306a3 to b768594 Compare January 15, 2025 06:35

sre-ci-robot added the do-not-merge/hold label Jan 15, 2025

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance: knowhere support data view index node #1016

enhance: knowhere support data view index node #1016

cqy123456 commented Jan 8, 2025 •

edited

Loading

sre-ci-robot commented Jan 8, 2025

mergify bot commented Jan 8, 2025

foxspy Jan 8, 2025

cqy123456 Jan 8, 2025

foxspy Jan 8, 2025

chasingegg Jan 10, 2025

chasingegg Jan 10, 2025

cqy123456 Jan 13, 2025

chasingegg Jan 10, 2025

cqy123456 Jan 13, 2025

codecov bot commented Jan 12, 2025 •

edited

Loading

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

alexanderguzhva Jan 12, 2025

foxspy Jan 13, 2025

alexanderguzhva Jan 15, 2025

cqy123456 Jan 15, 2025

foxspy Jan 15, 2025

cqy123456 Jan 15, 2025

foxspy commented Jan 15, 2025

enhance: knowhere support data view index node #1016

Are you sure you want to change the base?

enhance: knowhere support data view index node #1016

Conversation

cqy123456 commented Jan 8, 2025 • edited Loading

sre-ci-robot commented Jan 8, 2025

mergify bot commented Jan 8, 2025

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

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

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

foxspy commented Jan 15, 2025

cqy123456 commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading