From d142366ce4f4ecf26d0b93a013a32fd991807bb2 Mon Sep 17 00:00:00 2001 From: Mark Wu Date: Fri, 24 Jan 2025 14:52:57 -0800 Subject: [PATCH] Fixing bug where mapping accepts both dimension and model-id (#2410) Signed-off-by: Mark Wu --- CHANGELOG.md | 1 + .../index/mapper/KNNVectorFieldMapper.java | 9 ++++++++ .../mapper/KNNVectorFieldMapperTests.java | 22 ++++++++++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7d6214b..61f94e79a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * Fix filter rewrite logic which was resulting in getting inconsistent / incorrect results for cases where filter was getting rewritten for shards (#2359)[https://github.com/opensearch-project/k-NN/pull/2359] * Fixing it to retrieve space_type from index setting when both method and top level don't have the value. [#2374](https://github.com/opensearch-project/k-NN/pull/2374) * Fixing the bug where setting rescore as false for on_disk knn_vector query is a no-op (#2399)[https://github.com/opensearch-project/k-NN/pull/2399] +* Fixing bug where mapping accepts both dimension and model-id (#2410)[https://github.com/opensearch-project/k-NN/pull/2410] ### Infrastructure * Updated C++ version in JNI from c++11 to c++17 [#2259](https://github.com/opensearch-project/k-NN/pull/2259) * Upgrade bytebuddy and objenesis version to match OpenSearch core and, update github ci runner for macos [#2279](https://github.com/opensearch-project/k-NN/pull/2279) diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java index 99c6ebe2a..7a0ee00af 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -378,6 +378,15 @@ public Mapper.Builder parse(String name, Map node, ParserCont ); } + // Ensure user-defined dimension and model are mutually exclusive for indicies created after 2.19 + if (builder.dimension.getValue() != UNSET_MODEL_DIMENSION_IDENTIFIER + && builder.modelId.get() != null + && parserContext.indexVersionCreated().onOrAfter(Version.V_2_19_0)) { + throw new IllegalArgumentException( + String.format(Locale.ROOT, "Cannot specify both a modelId and dimension in the mapping: %s", name) + ); + } + // Check for flat configuration and validate only if index is created after 2.17 if (isKNNDisabled(parserContext.getSettings()) && parserContext.indexVersionCreated().onOrAfter(Version.V_2_17_0)) { validateFromFlat(builder); diff --git a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java index 49b15a0f4..5d4629fec 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -681,7 +681,6 @@ public void testTypeParser_parse_compressionAndModeParameter() { XContentBuilder xContentBuilder4 = XContentFactory.jsonBuilder() .startObject() .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) - .field(DIMENSION, 10) .field(VECTOR_DATA_TYPE_FIELD, VectorDataType.DEFAULT.getValue()) .field(MODEL_ID, "test") .field(MODE_PARAMETER, Mode.ON_DISK.getName()) @@ -892,6 +891,27 @@ public void testTypeParser_parse_fromModel() throws IOException { ); assertEquals(modelId, builder.modelId.get()); + + // Fails if both model_id and dimension are set after 2.19 + XContentBuilder xContentBuilder2 = XContentFactory.jsonBuilder() + .startObject() + .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) + .field(DIMENSION_FIELD_NAME, TEST_DIMENSION) + .field(MODEL_ID, modelId) + .endObject(); + + expectThrows( + IllegalArgumentException.class, + () -> typeParser.parse(fieldName, xContentBuilderToMap(xContentBuilder2), buildParserContext(indexName, settings)) + ); + + // Should not fail if both are set before 2.19 + typeParser.parse( + fieldName, + xContentBuilderToMap(xContentBuilder2), + buildLegacyParserContext(indexName, settings, Version.V_2_18_1) + ); + } public void testTypeParser_parse_fromLegacy() throws IOException {