Adding 'dense_vector' field type #3659

martin-gaievski · 2022-06-23T01:08:22Z

Signed-off-by: Martin Gaievski [email protected]

Description

Adding a new field type dense_vector. It's a first step in enabling KNN search in core OpenSearch. This implementation is based on Lucene HNSW, more details in corresponding RFC #3545

We'll be adding query type support in next PR, limiting this change to field type only to keep PR/change size smaller. With this change user will be able to :

define fields of type dense_vector with an option to index for the KNN search
add documents with dense_vector fields to the index

Issues Resolved

opensearch-project/k-NN#39

Check List

New functionality includes testing.
- All tests pass
Commits are signed 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.

opensearch-ci-bot · 2022-06-23T01:38:57Z

✅ Gradle Check success e6f79103191b342d6dd26eca23fb31f23609aaf7
Log 6242

Reports 6242

opensearch-ci-bot · 2022-06-24T22:37:08Z

✅ Gradle Check success b46bdb08b912c7cb65813b54bda729d766843966
Log 6313

Reports 6313

opensearch-ci-bot · 2022-06-24T22:44:45Z

✅ Gradle Check success 521644c537396b2ccdc00fb057761a3270afdb2b
Log 6314

Reports 6314

dblock · 2022-06-28T15:17:02Z

server/src/internalClusterTest/java/org/opensearch/search/knn/DenseVectorIT.java

+ * compatible open source license.
+ */
+
+/*


This is new code, isn't it? Doesn't need an ES license grant.

Yes, I'm adding several new classes. I actually wasn't sure on which one to use, checked few recent check-ins and classes in the same package, they used ES style license.
Let me update PR and exclude ES part

Only SPDX header is required for new code AFAIK.

I don't mind changing, it's the same we use in kNN repo. I just checked few random PRs for this repo before making this change, found the header I used here, e.g. #3004 or #690. At the same time I haven't found any example with the compact header.

Do you mean it's some recent requirement for the SPDX header and there might be no examples in the repo yet?

nknize

Thanks @martin-gaievski this is coming together! I left some initial feedback. Nothing too major. Mostly around test scaffolding and some extensibility protections.

nknize · 2022-06-29T15:34:33Z

server/src/test/java/org/opensearch/index/mapper/DenseVectorFieldTypeTests.java

+import static org.opensearch.index.mapper.KnnAlgorithmContextFactory.HNSW_PARAMETER_BEAM_WIDTH;
+import static org.opensearch.index.mapper.KnnAlgorithmContextFactory.HNSW_PARAMETER_MAX_CONNECTIONS;
+
+public class DenseVectorFieldTypeTests extends OpenSearchSingleNodeTestCase {


Most of these are FieldMapperTests (parsing the mapping) not FieldTypeTests so lets refactor them over to DenseVectorFieldMapperTests.

Also, OpenSearchSingleNodeTestCase is slow and big so we try to avoid it. Let's refactor this test class as follows:

Suggested change

public class DenseVectorFieldTypeTests extends OpenSearchSingleNodeTestCase {

public class DenseVectorFieldTypeTests extends FieldTypeTestCase {

For guidance on this test see the CollationFieldTypeTests as an example.

nknize · 2022-06-29T15:38:34Z

server/src/test/java/org/opensearch/index/mapper/DenseVectorMapperTests.java

+import static org.opensearch.index.mapper.KnnAlgorithmContextFactory.HNSW_PARAMETER_BEAM_WIDTH;
+import static org.opensearch.index.mapper.KnnAlgorithmContextFactory.HNSW_PARAMETER_MAX_CONNECTIONS;
+
+public class DenseVectorMapperTests extends MapperServiceTestCase {


Suggested change

public class DenseVectorMapperTests extends MapperServiceTestCase {

public class DenseVectorFieldMapperTests extends FieldMapperTestCase2<DenseVectorFieldMapper.Builder> {

We should use the FieldMapperTestCase2 helper class here to ensure we covering mapper merge conflict and serialization tests that are important for the REST client interfaces.

For guidance on this test see the LegacyGeoShapeFieldMapperTests example

I can't use FieldMapperTestCase2 directly as DenseVectorFieldMapper.Builder extends ParametrizedFieldMapper.Builder that is not supported by FieldMapperTestCase2, seems that majority of other mapper builders extend ParametrizedFieldMapper.Builder as well.

@nknize what would be your suggestion, is it better to keep new mapper for sense_vector as ParametrizedFieldMapper.Builder and write custom test cases for merge conflicts, or change it to FieldMapper.Builder? I do not see any strong argument for keeping it as ParametrizedFieldMapper.Builder except that it will require additional efforts to re-write current implementation, while with FieldMapper.Builder we obviously will have merge and serialization tests for free?

I've done refactoring, but to b honest I'm not sure if that is better than a previous version. While test coverage is better, there is a lot of additional code added that has been taking care of by the ParametrizedFieldMapper. Please check and let me know what do you think. Thx!

nknize · 2022-06-29T15:48:15Z

server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java

+        }
+
+        public DenseVectorFieldType(String name, Map<String, String> meta, int dimension, KnnContext knnContext, boolean hasDocValues) {
+            super(name, false, false, false, TextSearchInfo.NONE, meta);


Suggested change

super(name, false, false, false, TextSearchInfo.NONE, meta);

super(name, false, false, hasDocValues, TextSearchInfo.NONE, meta);

nknize · 2022-06-29T15:48:58Z

server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java

+
+        private final int dimension;
+        private final KnnContext knnContext;
+        private final boolean hasDocValues;


MappedFieldType already takes care of hasDocValues for you. Let's remove this variable and use the built in mechanism.

let me check MappedFieldType logic, initially I had to add custom code because unit tests were failing. Probably some piece was missing.

nknize · 2022-06-29T15:51:41Z

server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java

+import java.util.Map;
+
+/**
+ * Field Mapper for Dense vector type. Extends ParametrizedFieldMapper in order to easily configure mapping parameters.


Do we want external plugins (e.g., KNN) to support extending this class to add plugin specific logic? If we don't have anything as of yet, then can we make this final? Also let's annotate w/ @opensearch.internal.

Suggested change

* Field Mapper for Dense vector type. Extends ParametrizedFieldMapper in order to easily configure mapping parameters.

* Field Mapper for Dense vector type. Extends ParametrizedFieldMapper in order to easily configure mapping parameters.

*

* @opensearch.internal

nknize · 2022-06-29T15:54:42Z

server/src/main/java/org/opensearch/index/mapper/KnnContext.java

+/**
+ * Abstracts KNN segment of dense_vector field type
+ */
+public class KnnContext implements ToXContentFragment, Writeable {


Can we mark this class as final to prevent any subclassing fragmentation?

Suggested change

public class KnnContext implements ToXContentFragment, Writeable {

public final class KnnContext implements ToXContentFragment, Writeable {

nknize · 2022-06-29T15:54:53Z

server/src/main/java/org/opensearch/index/mapper/KnnContext.java

+import java.util.Objects;
+
+/**
+ * Abstracts KNN segment of dense_vector field type


Suggested change

* Abstracts KNN segment of dense_vector field type

* Abstracts KNN segment of dense_vector field type

*

* @opensearch.internal

nknize · 2022-06-29T15:56:45Z

server/src/main/java/org/opensearch/index/mapper/Metric.java

+/**
+ * Abstracts supported metrics for dense_vector and KNN search
+ */
+public enum Metric {


can this be package private?

Suggested change

public enum Metric {

final enum Metric {

I think package private should be possible, let me check. I'm not sure if we can/need do enum final

nknize · 2022-06-29T15:57:01Z

server/src/main/java/org/opensearch/index/mapper/Metric.java

+import java.util.Optional;
+
+/**
+ * Abstracts supported metrics for dense_vector and KNN search


Suggested change

* Abstracts supported metrics for dense_vector and KNN search

* Abstracts supported metrics for dense_vector and KNN search

*

* @opensearch.internal

reta · 2022-06-29T16:41:36Z

server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java

+            int value = XContentMapValues.nodeIntegerValue(o);
+            if (value > MAX_DIMENSION) {
+                throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "[dimension] value cannot be greater than %d for vector [%s]", MAX_DIMENSION, name)


Suggested change

String.format(Locale.ROOT, "[dimension] value cannot be greater than %d for vector [%s]", MAX_DIMENSION, name)

String.format(Locale.ROOT, "[dimension] value %d cannot be greater than %d for vector [%s]", value, MAX_DIMENSION, name)

reta · 2022-06-29T16:42:07Z

server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java

+            }
+            if (value <= 0) {
+                throw new IllegalArgumentException(
+                    String.format(Locale.ROOT, "[dimension] value must be greater than 0 for vector [%s]", name)


Suggested change

String.format(Locale.ROOT, "[dimension] value must be greater than 0 for vector [%s]", name)

String.format(Locale.ROOT, "[dimension] value %d must be greater than 0 for vector [%s]", value, name)

github-actions · 2022-07-01T17:27:45Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/122/
CommitID: 4814a08ced4c8efb4550dcd17cccd6908ad8b3b0

github-actions · 2022-07-02T04:35:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/125/
CommitID: 3682a665a6ebcacaeb76d802629a6dddd293e5cc

VijayanB · 2022-07-06T16:34:44Z

server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java

+    }
+
+    private boolean isDenseVectorFieldType(final MappedFieldType mappedFieldType) {
+        if (mappedFieldType != null && mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType) {


Do we need explicit null check before instanceof ? i think instanceof takes care of that.

you're right, I learned a new thing today thanks to you - https://docs.oracle.com/javase/specs/jls/se11/html/jls-15.html#jls-15.20.2. I'll update the code

Signed-off-by: Martin Gaievski <[email protected]>

…avadocs Signed-off-by: Martin Gaievski <[email protected]>

Signed-off-by: Martin Gaievski <[email protected]>

VijayanB · 2022-07-06T17:35:49Z

server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java

+    private final Method method;
+    private final Map<String, Object> parameters;
+
+    private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50;


nit: should we move all constants to beginning of class?

VijayanB · 2022-07-06T17:36:50Z

server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java

+    private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50;
+
+    public KnnAlgorithmContext(Method method, Map<String, Object> parameters) {
+        this.method = method;


requireNonNull check for params?

github-actions · 2022-07-06T17:39:38Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/173/
CommitID: ecb18fd

VijayanB · 2022-07-06T17:41:06Z

server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java

+                    Object v = e.getValue();
+                    if (v instanceof Map) {
+                        throw new MapperParsingException(
+                            String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", e.getValue())


Suggested change

String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", e.getValue())

String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", v)

VijayanB · 2022-07-06T17:45:30Z

server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java

+        if (parameters == null) {
+            builder.field(PARAMETERS, (String) null);
+        } else {
+            builder.startObject(PARAMETERS);
+            parameters.forEach((key, value) -> {
+                try {
+                    builder.field(key, value);
+                } catch (IOException ioe) {
+                    throw new RuntimeException("Unable to generate xcontent for method component");
+                }
+
+            });
+            builder.endObject();
+        }
+        return builder;


Suggested change

if (parameters == null) {

builder.field(PARAMETERS, (String) null);

} else {

builder.startObject(PARAMETERS);

parameters.forEach((key, value) -> {

try {

builder.field(key, value);

} catch (IOException ioe) {

throw new RuntimeException("Unable to generate xcontent for method component");

}

});

builder.endObject();

}

return builder;

if (parameters == null) {

return builder.field(PARAMETERS, (String) null);

}

builder.startObject(PARAMETERS);

parameters.forEach((key, value) -> {

try {

builder.field(key, value);

} catch (IOException ioe) {

throw new RuntimeException("Unable to generate xcontent for method component");

}

});

return builder.endObject();

github-actions · 2022-07-06T17:46:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/174/
CommitID: 6ad1bda

VijayanB · 2022-07-06T18:12:10Z

server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java

+ *
+ * @opensearch.internal
+ */
+public class KnnVectorFormatFactory {


WDYT?

Suggested change

public class KnnVectorFormatFactory {

public class KNNVectorFormatFactory {

I'm not sure to be honest - we're wrapping Lucene classes and they have notation as "Knn" (e.g. https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/KnnVectorField.java), so I've follow them

VijayanB · 2022-07-06T18:18:15Z

server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java

+        if (mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType) {
+            return true;
+        }
+        return false;


Suggested change

if (mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType) {

return true;

}

return false;

return (mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType);

or you don't need method if it is used once?

github-actions · 2022-07-06T18:42:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/178/
CommitID: dc6128c4f13fb05ff4699a4c8a0d7032b2ade722

VijayanB · 2022-07-06T18:50:13Z

server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java

+        Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null");
+        Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null");
+


nit:

Suggested change

Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null");

Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null");

this.method = Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null");

this.parameters = Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null");

Signed-off-by: Martin Gaievski <[email protected]>

github-actions · 2022-07-06T19:22:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/181/
CommitID: 24dfcdb

dblock · 2022-07-07T17:34:43Z

@nknize Take a look at the updates?

martin-gaievski · 2022-07-20T16:03:07Z

Closing this PR as team decided to go with implementation in k-NN plugin

martin-gaievski force-pushed the lucene-knn branch from b46bdb0 to 521644c Compare June 24, 2022 22:06

martin-gaievski marked this pull request as ready for review June 24, 2022 22:25

martin-gaievski requested review from a team and reta as code owners June 24, 2022 22:25

dblock reviewed Jun 28, 2022

View reviewed changes

dblock requested a review from andrross June 28, 2022 15:17

martin-gaievski force-pushed the lucene-knn branch from d1203f8 to 92ba3a1 Compare June 28, 2022 21:47

nknize requested changes Jun 29, 2022

View reviewed changes

reta reviewed Jun 29, 2022

View reviewed changes

reta mentioned this pull request Jul 4, 2022

[Meta] Migrations to OpenSearch #3757

Open

VijayanB reviewed Jul 6, 2022

View reviewed changes

martin-gaievski added 5 commits July 6, 2022 10:11

Adding 'dense_vector' field type

15fc51d

Signed-off-by: Martin Gaievski <[email protected]>

Adding checks on size limits for field type and vector data

d317584

Signed-off-by: Martin Gaievski <[email protected]>

Updating license header, exclude ES grant part

65c0944

Signed-off-by: Martin Gaievski <[email protected]>

Review comments: inherit hasValues from base class, error messages, j…

0325d0b

…avadocs Signed-off-by: Martin Gaievski <[email protected]>

Refactor mapper to extend FieldMapper, adjust tests accordingly

ecb18fd

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the lucene-knn branch from 3682a66 to ecb18fd Compare July 6, 2022 17:14

Remove redundant null-check

6ad1bda

Signed-off-by: Martin Gaievski <[email protected]>

VijayanB reviewed Jul 6, 2022

View reviewed changes

Refactoring code to address comments

24dfcdb

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the lucene-knn branch from dc6128c to 24dfcdb Compare July 6, 2022 18:57

martin-gaievski closed this Jul 20, 2022

	public class DenseVectorFieldTypeTests extends OpenSearchSingleNodeTestCase {
	public class DenseVectorFieldTypeTests extends FieldTypeTestCase {

	public class DenseVectorMapperTests extends MapperServiceTestCase {
	public class DenseVectorFieldMapperTests extends FieldMapperTestCase2<DenseVectorFieldMapper.Builder> {

	super(name, false, false, false, TextSearchInfo.NONE, meta);
	super(name, false, false, hasDocValues, TextSearchInfo.NONE, meta);

	public class KnnContext implements ToXContentFragment, Writeable {
	public final class KnnContext implements ToXContentFragment, Writeable {

	String.format(Locale.ROOT, "[dimension] value cannot be greater than %d for vector [%s]", MAX_DIMENSION, name)
	String.format(Locale.ROOT, "[dimension] value %d cannot be greater than %d for vector [%s]", value, MAX_DIMENSION, name)

	String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", e.getValue())
	String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", v)

	public class KnnVectorFormatFactory {
	public class KNNVectorFormatFactory {

		Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null");
		Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null");

Adding 'dense_vector' field type #3659

Adding 'dense_vector' field type #3659

Conversation

martin-gaievski commented Jun 23, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Jun 23, 2022

opensearch-ci-bot commented Jun 24, 2022

opensearch-ci-bot commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-gaievski Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize Jun 29, 2022 • edited Loading

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

nknize Jun 29, 2022 • edited Loading

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

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

github-actions bot commented Jul 1, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 2, 2022

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jul 6, 2022

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jul 6, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VijayanB Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 6, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 6, 2022

Gradle Check (Jenkins) Run Completed with:

dblock commented Jul 7, 2022

martin-gaievski commented Jul 20, 2022

martin-gaievski Jun 28, 2022 •

edited

Loading

nknize Jun 29, 2022 •

edited

Loading

nknize Jun 29, 2022 •

edited

Loading

VijayanB Jul 6, 2022 •

edited

Loading