From ad7bcdcc8fe1568950f8e168c17be2a7317e82aa Mon Sep 17 00:00:00 2001 From: Vijayan Balasubramanian Date: Tue, 22 Oct 2024 16:10:13 -0700 Subject: [PATCH] Update test method Signed-off-by: Vijayan Balasubramanian --- .../org/opensearch/knn/bwc/IndexingIT.java | 17 +++++ .../index/mapper/KNNVectorFieldMapper.java | 10 ++- .../opensearch/knn/index/query/KNNWeight.java | 2 +- .../knn/index/engine/EngineResolverTests.java | 6 +- .../mapper/KNNVectorFieldMapperTests.java | 68 +++++++++++-------- .../knn/index/query/KNNWeightTests.java | 6 +- 6 files changed, 71 insertions(+), 38 deletions(-) diff --git a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java index bf4c8f7ec..c344581fe 100644 --- a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java @@ -61,6 +61,23 @@ public void testKNNIndexDefaultLegacyFieldMapping() throws Exception { } } + // ensure that index is created using legacy mapping in old cluster, and, then docs are added, and, new docs are added + // in new cluster, search should return docs when it was indexed during old cluster and new cluster. + public void testKNNIndexDefaultEngine() throws Exception { + waitForClusterHealthGreen(NODES_BWC_CLUSTER); + if (isRunningAgainstOldCluster()) { + createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS)); + addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, 5); + // Flush to ensure that index is not re-indexed when node comes back up + flush(testIndex, true); + } else { + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 5, 5); + addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, 5, 5); + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 10, 10); + deleteKNNIndex(testIndex); + } + } + // Ensure that when segments created with old mapping are forcemerged in new cluster, they // succeed public void testKNNIndexDefaultLegacyFieldMappingForceMerge() throws Exception { 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 6ef3f01ab..beceadde5 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -500,9 +500,7 @@ private void resolveKNNMethodComponents( .build() ); - // If the original parameters are from legacy, and it is created on or before 2_17_2 since default is changed to - // FAISS starting 2_18, which doesn't support accepting algo params from index settings - if (parserContext.indexVersionCreated().onOrBefore(Version.V_2_17_2) && builder.originalParameters.isLegacyMapping()) { + if (useKNNMethodContextFromLegacy(builder, parserContext)) { // Then create KNNMethodContext to be used from the legacy index settings builder.originalParameters.setResolvedKnnMethodContext( createKNNMethodContextFromLegacy(parserContext.getSettings(), parserContext.indexVersionCreated(), resolvedSpaceType) @@ -551,6 +549,12 @@ private void setEngine(final KNNMethodContext knnMethodContext, KNNEngine knnEng } } + static boolean useKNNMethodContextFromLegacy(Builder builder, Mapper.TypeParser.ParserContext parserContext) { + // If the original parameters are from legacy, and it is created on or before 2_17_2 since default is changed to + // FAISS starting 2_18, which doesn't support accepting algo params from index settings + return parserContext.indexVersionCreated().onOrBefore(Version.V_2_17_2) && builder.originalParameters.isLegacyMapping(); + } + // We store the version of the index with the mapper as different version of Opensearch has different default // values of KNN engine Algorithms hyperparameters. protected Version indexCreatedVersion; diff --git a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java index b5b2a5d22..04c2ce587 100644 --- a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java +++ b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java @@ -251,7 +251,7 @@ private Map doANNSearch( spaceType = modelMetadata.getSpaceType(); vectorDataType = modelMetadata.getVectorDataType(); } else { - String engineName = fieldInfo.attributes().getOrDefault(KNN_ENGINE, KNNEngine.NMSLIB.getName()); + String engineName = fieldInfo.attributes().getOrDefault(KNN_ENGINE, KNNEngine.DEFAULT.getName()); knnEngine = KNNEngine.getEngine(engineName); String spaceTypeName = fieldInfo.attributes().getOrDefault(SPACE_TYPE, SpaceType.L2.getValue()); spaceType = SpaceType.getSpace(spaceTypeName); diff --git a/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java b/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java index df195883a..291f0c671 100644 --- a/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java +++ b/src/test/java/org/opensearch/knn/index/engine/EngineResolverTests.java @@ -41,10 +41,10 @@ public void testResolveEngine_whenModeAndCompressionAreFalse_thenDefault() { ); } - public void testResolveEngine_whenModeSpecifiedAndCompressionIsNotSpecified_thenDefault() { + public void testResolveEngine_whenModeSpecifiedAndCompressionIsNotSpecified_thenNMSLIB() { assertEquals(KNNEngine.DEFAULT, ENGINE_RESOLVER.resolveEngine(KNNMethodConfigContext.builder().build(), null, false)); assertEquals( - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, ENGINE_RESOLVER.resolveEngine( KNNMethodConfigContext.builder().mode(Mode.IN_MEMORY).build(), new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY, false), @@ -63,7 +63,7 @@ public void testResolveEngine_whenCompressionIs1x_thenEngineBasedOnMode() { ) ); assertEquals( - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, ENGINE_RESOLVER.resolveEngine(KNNMethodConfigContext.builder().compressionLevel(CompressionLevel.x1).build(), null, false) ); } 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 d2237736d..714723a8e 100644 --- a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java +++ b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java @@ -65,6 +65,7 @@ import java.util.Optional; import java.util.stream.Collectors; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -284,36 +285,43 @@ public void testTypeParser_withDifferentSpaceTypeCombinations_thenSuccess() thro ); assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); - // if space type is provided and legacy mappings is hit - xContentBuilder = XContentFactory.jsonBuilder() - .startObject() - .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) - .field(DIMENSION_FIELD_NAME, TEST_DIMENSION) - .field(KNNConstants.TOP_LEVEL_PARAMETER_SPACE_TYPE, topLevelSpaceType.getValue()) - .endObject(); - builder = (KNNVectorFieldMapper.Builder) typeParser.parse( - "test-field-name-1", - xContentBuilderToMap(xContentBuilder), - buildParserContext("test", settings) - ); + // mock useKNNMethodContextFromLegacy to simulate index ix created before 2_18 + try (MockedStatic utilMockedStatic = Mockito.mockStatic(KNNVectorFieldMapper.class)) { + utilMockedStatic.when(() -> KNNVectorFieldMapper.useKNNMethodContextFromLegacy(any(), any())).thenReturn(true); + // if space type is provided and legacy mappings is hit + xContentBuilder = XContentFactory.jsonBuilder() + .startObject() + .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE) + .field(DIMENSION_FIELD_NAME, TEST_DIMENSION) + .field(KNNConstants.TOP_LEVEL_PARAMETER_SPACE_TYPE, topLevelSpaceType.getValue()) + .endObject(); + builder = (KNNVectorFieldMapper.Builder) typeParser.parse( + "test-field-name-1", + xContentBuilderToMap(xContentBuilder), + buildParserContext("test", settings) + ); - builderContext = new Mapper.BuilderContext(settings, new ContentPath()); - knnVectorFieldMapper = builder.build(builderContext); - assertTrue(knnVectorFieldMapper instanceof MethodFieldMapper); - assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isPresent()); - assertEquals(topLevelSpaceType, knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().get().getSpaceType()); - // this check ensures that legacy mapping is hit, as in legacy mapping we pick M from index settings - assertEquals( - mForSetting, - knnVectorFieldMapper.fieldType() - .getKnnMappingConfig() - .getKnnMethodContext() - .get() - .getMethodComponentContext() - .getParameters() - .get(METHOD_PARAMETER_M) - ); - assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); + builderContext = new Mapper.BuilderContext(settings, new ContentPath()); + knnVectorFieldMapper = builder.build(builderContext); + assertTrue(knnVectorFieldMapper instanceof MethodFieldMapper); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().isPresent()); + assertEquals( + topLevelSpaceType, + knnVectorFieldMapper.fieldType().getKnnMappingConfig().getKnnMethodContext().get().getSpaceType() + ); + // this check ensures that legacy mapping is hit, as in legacy mapping we pick M from index settings + assertEquals( + mForSetting, + knnVectorFieldMapper.fieldType() + .getKnnMappingConfig() + .getKnnMethodContext() + .get() + .getMethodComponentContext() + .getParameters() + .get(METHOD_PARAMETER_M) + ); + assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty()); + } } public void testTypeParser_withSpaceTypeAndMode_thenSuccess() throws IOException { @@ -1703,7 +1711,7 @@ public void testTypeParser_whenModeAndCompressionAreSet_thenHandle() throws IOEx assertFalse(builder.getOriginalParameters().isLegacyMapping()); validateBuilderAfterParsing( builder, - KNNEngine.DEFAULT, + KNNEngine.NMSLIB, SpaceType.L2, VectorDataType.FLOAT, CompressionLevel.x1, diff --git a/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java b/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java index 877e6e6b1..511895026 100644 --- a/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java +++ b/src/test/java/org/opensearch/knn/index/query/KNNWeightTests.java @@ -175,7 +175,11 @@ public void tearDownAfterTest() { @SneakyThrows public void testQueryResultScoreNmslib() { for (SpaceType space : List.of(SpaceType.L2, SpaceType.L1, SpaceType.COSINESIMIL, SpaceType.INNER_PRODUCT, SpaceType.LINF)) { - testQueryScore(space::scoreTranslation, SEGMENT_FILES_NMSLIB, Map.of(SPACE_TYPE, space.getValue())); + testQueryScore( + space::scoreTranslation, + SEGMENT_FILES_NMSLIB, + Map.of(SPACE_TYPE, space.getValue(), KNN_ENGINE, KNNEngine.NMSLIB.getName()) + ); } }