enhance: knowhere support metric mhjaccard and index minhash lsh for minhash vector by cqy123456 · Pull Request #1203 · zilliztech/knowhere

cqy123456 · 2025-05-22T04:13:36Z

No description provided.

mergify · 2025-05-22T04:14:16Z

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

alexanderguzhva · 2025-05-22T11:56:42Z

+ private:
+    static constexpr size_t multiplier = 31;
+    std::vector<bool> bits;
+    size_t m;


please initialize these m, k, p, n values with zero here

alexanderguzhva · 2025-05-22T11:57:30Z

+    uint64_t total_size = dim * npts / 8;
+    data = std::make_unique<char[]>(total_size);
+    file.read(reinterpret_cast<char*>(data.get()), total_size);
+    return;


return is not needed

alexanderguzhva · 2025-05-22T12:00:20Z

+    for (size_t i = 0; i < element_length; i++) {
+        const char* x_b = x + element_size * i;
+        const char* y_b = y + element_size * i;
+        res += (std::memcmp(x_b, y_b, element_size) == 0);


this will trigger compiler warnings about implicit bool-to-int conversions. I'd use

int res = 0; for (size_t i = 0; i < element_length; i++) { const char* x_b = x + element_size * i; const char* y_b = y + element_size * i; res += (std::memcmp(x_b, y_b, element_size) == 0) ? 1 : 0; } return (float)res / float(element_length);

alexanderguzhva · 2025-05-22T12:00:47Z

+        const char* y_b = y + element_size * i;
+        res += (std::memcmp(x_b, y_b, element_size) == 0);
+    }
+    return res / float(element_length);


what if element_length==0?

element_length == 0 will check in minhashconfigcheck

alexanderguzhva · 2025-05-22T12:01:16Z

+        const char* x_b = x + r * i;
+        const char* y_b = y + r * i;
+        if (std::memcmp(x_b, y_b, r) == 0) {
+            return 1.0;


alexanderguzhva · 2025-05-22T12:04:01Z

+
+using idx_t = faiss::idx_t;
+struct MinHashLSHResultHandler {
+    idx_t* ids_list_;


please initialize these variables with default values

alexanderguzhva · 2025-05-22T12:04:13Z

+        counter_ = 0;
+        for (size_t i = 0; i < topk_; i++) {
+            ids_list_[i] = -1;
+            dis_list_[i] = 0.0;


alexanderguzhva · 2025-05-22T12:06:05Z

+                mask >>= 1;
+                offset++;
+            }
+            result = mid + offset;


it seems that something like __builtin_ctz() is also applicable here

alexanderguzhva · 2025-05-22T12:06:56Z

+    }
+    uint32_t sum = _mm512_reduce_add_epi32(equal_sum);
+    while (count > 0) {
+        sum += (*u32_x) == (*u32_y);


sum += ((*u32_x) == (*u32_y)) ? 1 : 0;

alexanderguzhva · 2025-05-22T12:08:33Z

+        sum += (*u32_x) == (*u32_y);
+        count--;
+        u32_x++;
+        u32_y++;


alternatively, you could use masked operation, such as _mm512_maskz_loadu_si512 and _mm512_mask_add_epi32

alexanderguzhva · 2025-05-22T12:12:55Z

@cqy123456 I'll re-read the code again and write more comments a bit later

alexanderguzhva · 2025-05-22T22:05:10Z

        self.requires("folly/2023.10.30.09@milvus/dev")
        self.requires("libcurl/8.2.1")
        self.requires("simde/0.8.2")
+        self.requires("xxhash/0.8.2")


the most recent version is 0.8.3

update later after pushing xxhash/0.8.3 to milvus JFrog cloud

alexanderguzhva · 2025-05-22T22:33:51Z

+
+        for (bool bit : bits) {
+            char byte = bit ? 1 : 0;
+            writeBinaryPOD(writer, byte);


Do I get it correct that this code writes 1 byte per 1 bit?

alexanderguzhva · 2025-05-22T22:35:16Z

 constexpr const char* INDEX_HNSW_PRQ = "HNSW_PRQ";

 constexpr const char* INDEX_DISKANN = "DISKANN";
+constexpr const char* INDEX_MINHASH_INDEX = "MinHash_LSH";


as far as I understand, this name should be made of uppercase letters, i.e. MINHASH_LSH

alexanderguzhva · 2025-05-22T22:36:03Z

+        KNOWHERE_CONFIG_DECLARE_FIELD(band).description("param of MinHashLSH").set_default(1).for_train().for_search();
+        KNOWHERE_CONFIG_DECLARE_FIELD(mh_element_bit_width)
+            .description("sizeof(hash code), the hash element should be aligned on 8 bits")
+            .set_default(8)


please set the acceptable range as well

alexanderguzhva · 2025-05-22T22:36:59Z

+    file.read(reinterpret_cast<char*>(&d), sizeof(uint32_t));
+    npts = n;
+    dim = d;
+    uint64_t total_size = dim * npts / 8;


(dim + 7) / 8 * npts, maybe?
Please confirm

dimension of binary vector should be divisible by 8, I have added the relevant check.

alexanderguzhva · 2025-05-22T22:51:11Z

+ private:
+    std::unique_ptr<MinHashBandIndex[]> band_index_;
+    bool is_loaded_ = false;
+    size_t block_size_;


alexanderguzhva · 2025-05-22T22:53:41Z

+        futures.emplace_back(build_pool->push([&, idx = i]() {
+            KeyType* blk_i = reinterpret_cast<KeyType*>(data_ + block_size_ * idx);
+            for (auto j = 0; j < num_in_a_blk_[idx]; j++) {
+                bloom_filter.add(blk_i[j]);


is bloom_filter thread safe?

alexanderguzhva · 2025-05-22T22:57:02Z

+// is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+// or implied. See the License for the specific language governing permissions and limitations under the License.
+
+#ifndef MINHASH_TREE_H


is it really needed to have a header-only code here? Many of the classes from this files (both functions and methods) may be safely moved to minhash_lsh.cpp file

alexanderguzhva · 2025-05-22T22:58:19Z

+    Load(FileReader& reader, size_t rows, char* mmap_data, BloomFilter<KeyType>& bloom_filter);
+
+    void
+    Search(KeyType key, MinHashLSHResultHandler* res, faiss::IDSelector* id_selector);


typically, this method should be a const one, so

void Search(KeyType key, MinHashLSHResultHandler* res, faiss::IDSelector* id_selector) const;

Please consider making it const

alexanderguzhva · 2025-05-22T22:58:44Z

+    Search(KeyType key, MinHashLSHResultHandler* res, faiss::IDSelector* id_selector);
+
+    Status
+    WarmUp() {


and other methods as well :)

…minhash vector Signed-off-by: cqy123456 <qianya.cheng@zilliz.com>

foxspy · 2025-05-27T08:00:55Z

+constexpr const char* SEARCH_WITH_JACCARD = "search_with_jaccard";
+constexpr const char* MH_ELEMENT_BIT_WIDTH = "mh_element_bit_width";
+constexpr const char* MH_LSH_REFINE_K = "refine_k";
+constexpr const char* BATCH_SEARCH = "batch_search";


Please add the mh_ prefix to the minhash-specific parameters to distinguish them.

foxspy · 2025-05-27T08:05:48Z

+
+    static expected<Resource>
+    StaticEstimateLoadResource(const float file_size, const knowhere::BaseConfig& config, const IndexVersion& version) {
+        return Resource{file_size * 0.25f, file_size};


please confirm and add comments for the load resource estimata

foxspy · 2025-05-27T08:22:46Z

        return !index_type.compare(IndexEnum::INDEX_DISKANN);
    } else {
-        return !index_type.compare(IndexEnum::INDEX_DISKANN) || !index_type.compare(IndexEnum::INDEX_HNSW);
+        return !index_type.compare(IndexEnum::INDEX_DISKANN) || !index_type.compare(IndexEnum::INDEX_HNSW) ||


Yeah, that's clearer.

foxspy · 2025-05-27T08:26:41Z

+
+    bool
+    HasRawData(const std::string& metric_type) const override {
+        return false;


This should be dynamic, if with_raw_data

foxspy · 2025-05-28T06:01:35Z

/kind improvement

foxspy

/lgtm

sre-ci-robot · 2025-05-28T08:11:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cqy123456, foxspy

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

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

…lsh for minhash vector (zilliztech#1203)" This reverts commit fd0871f.

…lsh for minhash vector (zilliztech#1203)" This reverts commit fd0871f. Signed-off-by: xianliang.li <xianliang.li@zilliz.com>

* update cardinal version Signed-off-by: xianliang.li <xianliang.li@zilliz.com> * Revert "enhance: knowhere support metric mhjaccard and index minhash lsh for minhash vector (#1203)" This reverts commit fd0871f. Signed-off-by: xianliang.li <xianliang.li@zilliz.com> --------- Signed-off-by: xianliang.li <xianliang.li@zilliz.com>

…minhash vector (zilliztech#1203) Signed-off-by: cqy123456 <qianya.cheng@zilliz.com>

sre-ci-robot requested review from hhy3 and liliu-z May 22, 2025 04:13

sre-ci-robot added approved size/XXL labels May 22, 2025

mergify Bot added dco-passed do-not-merge/missing-related-issue labels May 22, 2025

cqy123456 force-pushed the minhash-final branch 4 times, most recently from db09946 to f10de66 Compare May 22, 2025 10:03

alexanderguzhva reviewed May 22, 2025

View reviewed changes

cqy123456 force-pushed the minhash-final branch from f10de66 to 3721b9d Compare May 26, 2025 10:02

enhance: knowhere support metric mhjaccard and index minhash lsh for …

9508a39

…minhash vector Signed-off-by: cqy123456 <qianya.cheng@zilliz.com>

cqy123456 force-pushed the minhash-final branch from 3721b9d to 9508a39 Compare May 26, 2025 11:03

foxspy approved these changes May 27, 2025

View reviewed changes

sre-ci-robot added the kind/improvement label May 28, 2025

foxspy added the ci-passed label May 28, 2025

mergify Bot removed the ci-passed label May 28, 2025

foxspy removed the do-not-merge/missing-related-issue label May 28, 2025

foxspy approved these changes May 28, 2025

View reviewed changes

sre-ci-robot assigned foxspy May 28, 2025

sre-ci-robot added the lgtm label May 28, 2025

foxspy added the ci-passed label May 28, 2025

mergify Bot removed the ci-passed label May 28, 2025

foxspy added the ci-passed label May 28, 2025

mergify Bot removed the ci-passed label May 28, 2025

foxspy approved these changes May 28, 2025

View reviewed changes

foxspy added the ci-passed label May 28, 2025

foxspy merged commit fd0871f into zilliztech:main May 28, 2025
9 of 11 checks passed

mergify Bot removed the ci-passed label May 28, 2025

foxspy added a commit to foxspy/knowhere that referenced this pull request May 28, 2025

Revert "enhance: knowhere support metric mhjaccard and index minhash …

d7345dc

…lsh for minhash vector (zilliztech#1203)" This reverts commit fd0871f.

cqy123456 added a commit to cqy123456/zilliztech-knowhere that referenced this pull request May 29, 2025

enhance: knowhere support metric mhjaccard and index minhash lsh for …

cbcc46b

…minhash vector (zilliztech#1203) Signed-off-by: cqy123456 <qianya.cheng@zilliz.com>

alexanderguzhva mentioned this pull request May 29, 2025

enhance: knowhere support metric mhjaccard and index minhash lsh for minhash vector #1207

Merged

Conversation

cqy123456 commented May 22, 2025

Uh oh!

mergify Bot commented May 22, 2025

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!

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 commented May 22, 2025

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!

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!

foxspy commented May 28, 2025

Uh oh!

foxspy left a comment

Choose a reason for hiding this comment

Uh oh!

sre-ci-robot commented May 28, 2025

Uh oh!

Uh oh!

Reviewers