Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bug to prevent NullPointerException while doing PUT mappings #2582

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fixing bug to prevent NullPointerException while doing PUT mappings #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0b0fed0
Updating NPE bug fixes for #2556
oaganesh Mar 5, 2025
bd881b5
Updating Changelog file for bug fix #2556
oaganesh Mar 5, 2025
3a81328
Applying spotless changes for bug fix
oaganesh Mar 5, 2025
8902245
Adding unit test to verify null method context
oaganesh Mar 5, 2025
814e8d7
Updating error message for method key
oaganesh Mar 5, 2025
3251586
Updating error message for method config bug.
oaganesh Mar 5, 2025
50c9412
Updating error response to utilize conflictSerializer for PUT mappings.
oaganesh Mar 7, 2025
c43a314
Adding unit and integration tests for method map bug
oaganesh Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Enhancements
* Introduce node level circuit breakers for k-NN [#2509](https://github.com/opensearch-project/k-NN/pull/2509)
### Bug Fixes
* Fixing bug to prevent NullPointerException while doing PUT mappings [#2556](https://github.com/opensearch-project/k-NN/issues/2556)
### Infrastructure
* Removed JDK 11 and 17 version from CI runs [#1921](https://github.com/opensearch-project/k-NN/pull/1921)
* Upgrade min JDK compatibility to JDK 21 [#2422](https://github.com/opensearch-project/k-NN/pull/2422)
Expand Down
18 changes: 12 additions & 6 deletions src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.opensearch.knn.index.engine.ResolvedMethodContext;
import org.opensearch.knn.index.engine.SpaceTypeResolver;
import org.opensearch.knn.indices.ModelDao;

import static org.opensearch.knn.common.KNNConstants.DEFAULT_VECTOR_DATA_TYPE_FIELD;
import static org.opensearch.knn.common.KNNConstants.KNN_METHOD;
import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
Expand Down Expand Up @@ -157,11 +156,18 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
() -> null,
(n, c, o) -> KNNMethodContext.parse(o),
m -> toType(m).originalMappingParameters.getKnnMethodContext()
).setSerializer(((b, n, v) -> {
b.startObject(n);
v.toXContent(b, ToXContent.EMPTY_PARAMS);
b.endObject();
}), m -> m.getMethodComponentContext().getName());
).setSerializer(
// Main serializer - handles null values properly
(b, f, v) -> {
if (v != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Is there a way for this to just serialize the null value as opposed to skipping? Like, b.field(f, null) ort something

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I dont think we'll hit this because https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java#L195, acceptsNull is false by default, but not sure. Were you able to repro a failure case for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jack,
Do you mean something like this?
(b, f, v) -> { b.startObject(f); if (v == null) { b.nullField("value"); } else { v.toXContent(b, ToXContent.EMPTY_PARAMS); } b.endObject(); }

Also I have not reproduced a failure case for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think itd be:

if v == null
 b.nullField(f)
else:
 b.startObject(f)
...
 b.endObject()

b.startObject(f);
v.toXContent(b, ToXContent.EMPTY_PARAMS);
b.endObject();
}
},
// Conflict serializer - simple string representation for error messages
v -> v == null ? null : v.getMethodComponentContext().getName()
).acceptsNull();

protected final Parameter<String> mode = Parameter.restrictedStringParam(
KNNConstants.MODE_PARAMETER,
Expand Down
37 changes: 37 additions & 0 deletions src/test/java/org/opensearch/knn/index/OpenSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,43 @@ public void testKNNIndexSearchFieldsParameterDocsWithOnlyOtherFields() throws Ex
assertEquals(0, parseSearchResponseFieldsCount(EntityUtils.toString(response4.getEntity()), "text1"));
}

public void testKNNVectorMappingUpdate_whenMethodRemoved_thenThrowsException() throws Exception {
String indexName = "test-knn-index";
String fieldName = "my_vector2";

XContentBuilder initialMapping = XContentFactory.jsonBuilder()
.startObject()
.startObject("properties")
.startObject(fieldName)
.field("type", "knn_vector")
.field("dimension", "4")
.startObject("method")
.field("engine", "faiss")
.field("name", "hnsw")
.endObject()
.endObject()
.endObject()
.endObject();
createKnnIndex(indexName, getKNNDefaultIndexSettings(), initialMapping.toString());

XContentBuilder updatedMapping = XContentFactory.jsonBuilder()
.startObject()
.startObject("properties")
.startObject(fieldName)
.field("type", "knn_vector")
.field("dimension", 4)
.endObject()
.endObject()
.endObject();

ResponseException exception = expectThrows(ResponseException.class, () -> putMappingRequest(indexName, updatedMapping.toString()));

assertThat(
EntityUtils.toString(exception.getResponse().getEntity()),
containsString("Cannot update parameter [method] from [hnsw] to [null]")
);
}

private List<KNNResult> getResults(final String indexName, final String fieldName, final float[] vector, final int k)
throws IOException, ParseException {
final Response searchResponseField = searchKNNIndex(indexName, new KNNQueryBuilder(fieldName, vector, k), k);
Expand Down
48 changes: 48 additions & 0 deletions src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,54 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx
);
}

public void testKNNVectorFieldMapper_UpdateMethodParameter_ThrowsException() throws IOException {
String fieldName = "test-field-name";
String indexName = "test-index-name";

Settings settings = Settings.builder().put(settings(CURRENT).build()).put(KNN_INDEX, true).build();
ModelDao modelDao = mock(ModelDao.class);
KNNVectorFieldMapper.TypeParser typeParser = new KNNVectorFieldMapper.TypeParser(() -> modelDao);

// Define initial mapping with method parameter
XContentBuilder initialMapping = XContentFactory.jsonBuilder()
.startObject()
.field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE)
.field(DIMENSION_FIELD_NAME, 4)
.startObject(KNN_METHOD)
.field("engine", "faiss")
.field("name", "hnsw")
.endObject()
.endObject();

KNNVectorFieldMapper.Builder initialBuilder = (KNNVectorFieldMapper.Builder) typeParser.parse(
fieldName,
xContentBuilderToMap(initialMapping),
buildParserContext(indexName, settings)
);

Mapper.BuilderContext builderContext = new Mapper.BuilderContext(settings, new ContentPath());
KNNVectorFieldMapper initialMapper = initialBuilder.build(builderContext);

// Define updated mapping without the method parameter
XContentBuilder updatedMapping = XContentFactory.jsonBuilder()
.startObject()
.field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE)
.field(DIMENSION_FIELD_NAME, 4)
.endObject();

KNNVectorFieldMapper.Builder updatedBuilder = (KNNVectorFieldMapper.Builder) typeParser.parse(
fieldName,
xContentBuilderToMap(updatedMapping),
buildParserContext(indexName, settings)
);

KNNVectorFieldMapper updatedMapper = updatedBuilder.build(builderContext);

// Expect an IllegalArgumentException when merging the updated mapping
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> initialMapper.merge(updatedMapper));
assertTrue(exception.getMessage().contains("Cannot update parameter [method] from [hnsw] to [null]"));
}

private void validateBuilderAfterParsing(
KNNVectorFieldMapper.Builder builder,
KNNEngine expectedEngine,
Expand Down