Fix few bugs on binary index with Faiss HNSW #1850

heemin32 · 2024-07-18T00:14:26Z

Description

Did a manual test on following items. Will raise a new PR implementing this as integ test.

Test case

Can create binary index using Faiss HNSW
Can create binary index with knn index as false
Cannot create binary index using Lucene
Cannot create binary index using Nmslib
Cannot run training API on binary index
Cannot create binary index using encoding(sq)
Cannot run range query on binary index
Can run knn query on binary index
Can run knn query with nested field on binary index
Can run knn query with filtering on binary index
Can run knn query with filtering on nested field with binary index
Can run script scoring with hamming on binary index
Cannot run script scoring with hamming on non binary index
Cannot run script scoring with non hamming on binary index
Can run painless script with hamming on binary index
Cannot run painless script with hamming on non binary index
Cannot run painless script with non hamming on binary index
Can run script scoring with hamming on binary index with knn set as false
Can run painless script with hamming on binary index with knn set as false

Bug1 (2. Can create binary index with knn index as false)

Actual: Following index creation succeeded but got an error while ingesting as default space was set as l2.
Expected: It should set hamming as default space type and ingesting should succeed.

{
  "settings": {
    "index": {
      "knn": false
    }
  },
  "mappings": {
    "properties": {
      "my_vector": {
        "type": "knn_vector",
        "dimension": 8,
        "data_type": "binary"
      }
    }
  }
}

Bug2 (3. Cannot create binary index using Lucene)

Actual: I got error \"hnsw\" configuration does not support space type: \"hammingbit\" which is not appropriate without mentioning what engine it uses.
Expected: The error message should have engine name as well because hnsw with faiss support hammingbit.

Create index with knn as false
{
  "settings": {
    "index": {
      "knn": false
    }
  },
  "mappings": {
    "properties": {
      "my_vector": {
        "type": "knn_vector",
        "dimension": 8,
        "data_type": "binary",
        "method": {
          "name": "hnsw",
          "engine": "lucene",
          "space_type": "hammingbit"
        }
      }
    }
  }
}

Bug 3. (7. Cannot run range query on binary index)

Actual: Runtime error
Expected: It should be Unsupported operation error with a message like "radial search is not supported with binary index"

"root_cause": [
    {
    "type": "runtime_exception",
    "reason": "java.lang.Exception: Query Vector cannot be null"
    }
],

Bug 4 (11. Can run knn query with filtering on nested field with binary index)

Actual: Failed to run filtered query with binary Index. Error was happening while validating if exact search is required or not based on calculation cost
Expected: Filtered query with binary index should work.

    "root_cause": [
      {
        "type": "null_pointer_exception",
        "reason": "Cannot read the array length because the return value of \"org.opensearch.knn.index.query.KNNQuery.getQueryVector()\" is null"
      }
    ],

Related Issues

N/A

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

ryanbogan

LGTM! The integration tests for these cases will be in 2.16 right?

navneet1v · 2024-07-18T16:39:22Z

src/main/java/org/opensearch/knn/index/IndexUtil.java

@@ -190,6 +190,14 @@ public static ValidationException validateKnnField(
            return exception;
        }

+        String vectorDataType = (String) fieldMap.get(VECTOR_DATA_TYPE_FIELD);


can fieldMap.get(VECTOR_DATA_TYPE_FIELD) be null?

It can be null.

if it can be null then the type casting to string will cause a NPE. Please handle this gracefully.

It won't cause a NPE.

why?? casting a null to string cause NPE. I am missing something here.

I tested it and it didn't cause NPE. String is of Object and String can be null.

navneet1v · 2024-07-18T16:39:58Z

src/main/java/org/opensearch/knn/index/IndexUtil.java

@@ -190,6 +190,14 @@ public static ValidationException validateKnnField(
            return exception;
        }

+        String vectorDataType = (String) fieldMap.get(VECTOR_DATA_TYPE_FIELD);
+        if (VectorDataType.BINARY.toString().equalsIgnoreCase(vectorDataType)) {


we should convert the datatype to enum and then use == in if condition

Because vectorDataType can be null, I choose this approach. Why should we convert it to the data type?
FYI, this line of code will be removed once we support binary index for IVF.

src/main/java/org/opensearch/knn/index/IndexUtil.java

src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

jmazanec15 · 2024-07-18T16:47:44Z

@heemin32 Can you provide intended behavior for the bugs as well?

heemin32 · 2024-07-18T17:03:46Z

LGTM! The integration tests for these cases will be in 2.16 right?

Will try. As the last resort, will do the manual test after code freeze once more.

heemin32 · 2024-07-18T17:09:19Z

@heemin32 Can you provide intended behavior for the bugs as well?

Added.

Signed-off-by: Heemin Kim <heemin@amazon.com>

shatejas

shatejas · 2024-07-18T18:14:59Z

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

+            ? knnQuery.getQueryVector().length
+            : knnQuery.getByteQueryVector().length);


I don't like the fact that we have two vectors. This will create a lot of branching in the code. We should have gone with generics or something with a different type of query. Something similar to lucene.

We are already aware of it and captured it in #1810

navneet1v · 2024-07-18T19:07:49Z

@heemin32 lets ensure that GH action are successful before we merge this PR

vamshin

LGTM! Thanks

Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64)

Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64) Co-authored-by: Heemin Kim <heemin@amazon.com>

opensearch-project#1856) Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64) Co-authored-by: Heemin Kim <heemin@amazon.com>

heemin32 requested review from navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners July 18, 2024 00:14

heemin32 added backport 2.x skip-changelog labels Jul 18, 2024

heemin32 force-pushed the bugfix branch from 50cff81 to 439a538 Compare July 18, 2024 01:02

junqiu-lei previously approved these changes Jul 18, 2024

View reviewed changes

ryanbogan previously approved these changes Jul 18, 2024

View reviewed changes

navneet1v reviewed Jul 18, 2024

View reviewed changes

heemin32 dismissed stale reviews from ryanbogan and junqiu-lei via 215068f July 18, 2024 17:02

heemin32 requested review from junqiu-lei, ryanbogan and navneet1v July 18, 2024 17:09

heemin32 force-pushed the bugfix branch from 215068f to 008ec15 Compare July 18, 2024 17:15

Fix few bugs on binary index with Faiss HNSW

5d1f6fa

Signed-off-by: Heemin Kim <heemin@amazon.com>

heemin32 force-pushed the bugfix branch from 008ec15 to 5d1f6fa Compare July 18, 2024 17:26

shatejas reviewed Jul 18, 2024

View reviewed changes

navneet1v approved these changes Jul 18, 2024

View reviewed changes

vamshin approved these changes Jul 18, 2024

View reviewed changes

heemin32 merged commit 3a5fe64 into opensearch-project:main Jul 18, 2024
52 checks passed

heemin32 deleted the bugfix branch July 18, 2024 22:37

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 18, 2024

Fix few bugs on binary index with Faiss HNSW (#1850)

ec80b37

Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64)

opensearch-trigger-bot bot mentioned this pull request Jul 18, 2024

[Backport 2.x] Fix few bugs on binary index with Faiss HNSW #1856

Merged

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

Fix few bugs on binary index with Faiss HNSW (#1850) (#1856)

2c6e0bf

Signed-off-by: Heemin Kim <heemin@amazon.com> (cherry picked from commit 3a5fe64) Co-authored-by: Heemin Kim <heemin@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix few bugs on binary index with Faiss HNSW #1850

Fix few bugs on binary index with Faiss HNSW #1850

heemin32 commented Jul 18, 2024 •

edited

Loading

ryanbogan left a comment

navneet1v Jul 18, 2024

heemin32 Jul 18, 2024

navneet1v Jul 18, 2024

heemin32 Jul 18, 2024

navneet1v Jul 18, 2024

heemin32 Jul 18, 2024 •

edited

Loading

navneet1v Jul 18, 2024

heemin32 Jul 18, 2024 •

edited

Loading

jmazanec15 commented Jul 18, 2024

heemin32 commented Jul 18, 2024 •

edited

Loading

heemin32 commented Jul 18, 2024

shatejas left a comment •

edited

Loading

shatejas Jul 18, 2024

heemin32 Jul 18, 2024

navneet1v commented Jul 18, 2024

vamshin left a comment

		? knnQuery.getQueryVector().length
		: knnQuery.getByteQueryVector().length);

Fix few bugs on binary index with Faiss HNSW #1850

Fix few bugs on binary index with Faiss HNSW #1850

Conversation

heemin32 commented Jul 18, 2024 • edited Loading

Description

Test case

Bug1 (2. Can create binary index with knn index as false)

Bug2 (3. Cannot create binary index using Lucene)

Bug 3. (7. Cannot run range query on binary index)

Bug 4 (11. Can run knn query with filtering on nested field with binary index)

Related Issues

Check List

ryanbogan left a 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

heemin32 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heemin32 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

jmazanec15 commented Jul 18, 2024

heemin32 commented Jul 18, 2024 • edited Loading

heemin32 commented Jul 18, 2024

shatejas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Jul 18, 2024

vamshin left a comment

Choose a reason for hiding this comment

heemin32 commented Jul 18, 2024 •

edited

Loading

heemin32 Jul 18, 2024 •

edited

Loading

heemin32 Jul 18, 2024 •

edited

Loading

heemin32 commented Jul 18, 2024 •

edited

Loading

shatejas left a comment •

edited

Loading