Add support of multi vector in jni #1364

heemin32 · 2023-12-28T00:32:51Z

Description

With this PR, QueryIndex and QueryIndex_WithFilter JNI method can take a list of parentId to dedupe search result per parent. To do that, custom result collector is implemented which will be passed as HNSW search parameter to faiss library.

Issues Resolved

N/A

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

jmazanec15 · 2023-12-29T18:47:11Z

@heemin32 Can you give detailed summary on what is being changed in PR description?

jni/include/knn_extension/faiss/MultiVectorResultCollector.h

jni/include/knn_extension/faiss/utils/BitSet.h

jni/src/knn_extension/faiss/MultiVectorResultCollectorFactory.cpp

jni/src/knn_extension/faiss/utils/BitSet.cpp

jni/include/org_opensearch_knn_jni_FaissService.h

codecov · 2023-12-29T19:57:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7b47bae) 85.10% compared to head (411dbfc) 85.10%.

Additional details and impacted files

@@                   Coverage Diff                   @@
##             feature/multi-vector    #1364   +/-   ##
=======================================================
  Coverage                   85.10%   85.10%           
  Complexity                   1251     1251           
=======================================================
  Files                         162      162           
  Lines                        5101     5101           
  Branches                      477      477           
=======================================================
  Hits                         4341     4341           
  Misses                        554      554           
  Partials                      206      206

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jni/CMakeLists.txt

jni/include/knn_extension/faiss/MultiVectorResultCollector.h

jni/include/knn_extension/faiss/MultiVectorResultCollectorFactory.h

jni/include/knn_extension/faiss/utils/BitSet.h

jni/src/knn_extension/faiss/utils/BitSet.cpp

jni/include/knn_extension/faiss/utils/BitSet.h

jni/src/knn_extension/faiss/utils/BitSet.cpp

jni/include/knn_extension/faiss/utils/BitSet.h

jni/src/faiss_wrapper.cpp

jni/src/knn_extension/faiss/MultiVectorResultCollector.cpp

jni/include/knn_extension/faiss/MultiVectorResultCollector.h

jni/include/knn_extension/faiss/utils/BitSet.h

jni/include/knn_extension/faiss/utils/Heap.h

jni/src/knn_extension/faiss/utils/BitSet.cpp

src/main/java/org/opensearch/knn/jni/FaissService.java

jni/tests/knn_extension/faiss/utils/HeapTest.cpp

jni/tests/knn_extension/faiss/MultiVectorResultCollectorTest.cpp

jni/include/knn_extension/faiss/utils/BitSet.h

jni/include/knn_extension/faiss/utils/Heap.h

jmazanec15 · 2024-01-03T19:28:14Z

jni/src/knn_extension/faiss/utils/BitSet.cpp

+    uint64_t word = this->bitmap[i] >> (index & 63); // Equivalent of bitmap[i] >> (index % 64)
+
+    if (word != 0) {
+      return index + __builtin_ctzll(word);


Could you add a comment around usage of this? What does word represent? It looks like its the 64-bit int that is like the "bit page" shifted over offset bits.

Take a look at the comment in Heap.h file and let me know if you still need some comment here to understand the code.

Not sure which comment in Heap.h you are referring

I mean

* Here a block is 64 bit. However, for simplicity let's assume its size is 8 bits. * Then, if have an array of 3, 7, and 10, it will be represented in bitmap as follow. * [0] [1] * bitmap: 10001000 00000100 * * for next_set_bit call with 4 * 1. it looks for bitmap[0] * 2. bitmap[0] >> 4 * 3. count trailing zero of the result from step 2 which is 3 * 4. return 4(current index) + 3(result from step 3)

Thats not in Heap.h, but makes sense

Sorry. BitSet.h :)

jni/tests/knn_extension/faiss/utils/BitSetTest.cpp

Signed-off-by: Heemin Kim <[email protected]>

jmazanec15 · 2024-01-03T20:38:08Z

jni/src/knn_extension/faiss/utils/BitSet.cpp

+    uint64_t word = this->bitmap[i] >> (index & 63); // Equivalent of bitmap[i] >> (index % 64)
+
+    if (word != 0) {
+      return index + __builtin_ctzll(word);


Not sure which comment in Heap.h you are referring

jni/src/knn_extension/faiss/utils/BitSet.cpp

jni/include/knn_extension/faiss/utils/Heap.h

Signed-off-by: Heemin Kim <[email protected]>

* Add patch to support multi vector in faiss (#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]>

Signed-off-by: Heemin Kim <[email protected]>

* Add patch to support multi vector in faiss (opensearch-project#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (opensearch-project#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (opensearch-project#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (opensearch-project#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (opensearch-project#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (opensearch-project#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit 709b448)

* Add patch to support multi vector in faiss (#1358) Signed-off-by: Heemin Kim <[email protected]> * Initialize id_map as null (#1363) Signed-off-by: Heemin Kim <[email protected]> * Add support of multi vector in jni (#1364) Signed-off-by: Heemin Kim <[email protected]> * Multi vector support for Faiss HNSW (#1371) Apply the parentId filter to the Faiss HNSW search method. This ensures that documents are deduplicated based on their parentId, and the method returns k results for documents with nested fields. Signed-off-by: Heemin Kim <[email protected]> * Add data generation script for nested field (#1388) Signed-off-by: Heemin Kim <[email protected]> * Add perf test for nested field (#1394) Signed-off-by: Heemin Kim <[email protected]> --------- Signed-off-by: Heemin Kim <[email protected]> (cherry picked from commit 709b448)

heemin32 added the skip-changelog label Dec 28, 2023

heemin32 requested review from navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski and ryanbogan as code owners December 28, 2023 00:32

heemin32 force-pushed the multi-vector branch 4 times, most recently from e88ce8a to f743e66 Compare December 29, 2023 17:54

jmazanec15 reviewed Dec 29, 2023

View reviewed changes

jni/include/org_opensearch_knn_jni_FaissService.h Show resolved Hide resolved

heemin32 force-pushed the multi-vector branch 2 times, most recently from d6b2116 to 7a00cab Compare December 29, 2023 19:40

heemin32 requested a review from jmazanec15 December 29, 2023 20:05

navneet1v reviewed Dec 29, 2023

View reviewed changes

jni/CMakeLists.txt Show resolved Hide resolved

navneet1v reviewed Dec 29, 2023

View reviewed changes

jni/include/knn_extension/faiss/MultiVectorResultCollector.h Outdated Show resolved Hide resolved

navneet1v reviewed Dec 29, 2023

View reviewed changes

navneet1v reviewed Dec 30, 2023

View reviewed changes

jni/src/knn_extension/faiss/utils/BitSet.cpp Outdated Show resolved Hide resolved

navneet1v reviewed Dec 30, 2023

View reviewed changes

jni/src/knn_extension/faiss/utils/BitSet.cpp Outdated Show resolved Hide resolved

navneet1v reviewed Dec 30, 2023

View reviewed changes

jni/include/knn_extension/faiss/utils/BitSet.h Show resolved Hide resolved

navneet1v reviewed Dec 30, 2023

View reviewed changes

jni/src/faiss_wrapper.cpp Outdated Show resolved Hide resolved

navneet1v reviewed Dec 30, 2023

View reviewed changes

jni/src/knn_extension/faiss/MultiVectorResultCollector.cpp Show resolved Hide resolved

martin-gaievski reviewed Jan 2, 2024

View reviewed changes

heemin32 force-pushed the multi-vector branch from 7a00cab to 0b252e7 Compare January 2, 2024 23:18

heemin32 requested review from navneet1v and martin-gaievski January 2, 2024 23:19

martin-gaievski approved these changes Jan 3, 2024

View reviewed changes

jmazanec15 reviewed Jan 3, 2024

View reviewed changes

navneet1v reviewed Jan 3, 2024

View reviewed changes

jni/tests/knn_extension/faiss/utils/BitSetTest.cpp Show resolved Hide resolved

Add support of multi vector in jni

411dbfc

Signed-off-by: Heemin Kim <[email protected]>

heemin32 force-pushed the multi-vector branch from 0b252e7 to 411dbfc Compare January 3, 2024 20:24

navneet1v approved these changes Jan 3, 2024

View reviewed changes

jmazanec15 reviewed Jan 3, 2024

View reviewed changes

jmazanec15 approved these changes Jan 3, 2024

View reviewed changes

heemin32 merged commit df6d1fa into opensearch-project:feature/multi-vector Jan 3, 2024
23 of 47 checks passed

heemin32 deleted the multi-vector branch January 3, 2024 21:52

heemin32 added a commit to heemin32/k-NN that referenced this pull request Jan 5, 2024

Add support of multi vector in jni (opensearch-project#1364)

e64f762

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit to heemin32/k-NN that referenced this pull request Jan 16, 2024

Add support of multi vector in jni (opensearch-project#1364)

3d7e6b7

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit that referenced this pull request Jan 16, 2024

Add support of multi vector in jni (#1364)

ead4411

Signed-off-by: Heemin Kim <[email protected]>

heemin32 added a commit that referenced this pull request Jan 19, 2024

Add support of multi vector in jni (#1364)

279fd1b

Signed-off-by: Heemin Kim <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of multi vector in jni #1364

Add support of multi vector in jni #1364

heemin32 commented Dec 28, 2023 •

edited

Loading

jmazanec15 commented Dec 29, 2023

codecov bot commented Dec 29, 2023 •

edited

Loading

jmazanec15 Jan 3, 2024

heemin32 Jan 3, 2024

jmazanec15 Jan 3, 2024

heemin32 Jan 3, 2024

jmazanec15 Jan 3, 2024

heemin32 Jan 3, 2024

jmazanec15 Jan 3, 2024

Add support of multi vector in jni #1364

Add support of multi vector in jni #1364

Conversation

heemin32 commented Dec 28, 2023 • edited Loading

Description

Issues Resolved

Check List

jmazanec15 commented Dec 29, 2023

codecov bot commented Dec 29, 2023 • edited Loading

Codecov Report

jmazanec15 Jan 3, 2024

Choose a reason for hiding this comment

heemin32 Jan 3, 2024

Choose a reason for hiding this comment

jmazanec15 Jan 3, 2024

Choose a reason for hiding this comment

heemin32 Jan 3, 2024

Choose a reason for hiding this comment

jmazanec15 Jan 3, 2024

Choose a reason for hiding this comment

heemin32 Jan 3, 2024

Choose a reason for hiding this comment

jmazanec15 Jan 3, 2024

Choose a reason for hiding this comment

heemin32 commented Dec 28, 2023 •

edited

Loading

codecov bot commented Dec 29, 2023 •

edited

Loading