From 8da34f19f3e6911bb004f688c9cb95f76026fb66 Mon Sep 17 00:00:00 2001 From: Vijayan Balasubramanian Date: Mon, 2 Dec 2024 12:31:56 -0800 Subject: [PATCH] Validate method parameters only after version 2_17_0 k-NN introduced validation to prevent index creation api to accept method paramters that are used by approximate k-NN search algorithm. For ex: create index api accepts model_id, or, method params even if knn index setting was not true before 2.17. However, for index that were created before 2.17 and upgraded to 2.17, will prevent cluster to parse those previously accepted index mapping. Hence, k-NN will allow those paramters for indices that were created before 2.17, but doesn't support those parameters starting 2.17. Signed-off-by: Vijayan Balasubramanian --- .../index/mapper/KNNVectorFieldMapper.java | 13 ++++++++--- .../opensearch/knn/index/OpenSearchIT.java | 23 +++++++++++++++++++ .../org/opensearch/knn/KNNRestTestCase.java | 16 +++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) 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 beceadde5..cb4496b60 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -194,6 +194,8 @@ public static class Builder extends ParametrizedFieldMapper.Builder { @Setter @Getter private OriginalMappingParameters originalParameters; + @Setter + private boolean isKnnIndex; public Builder( String name, @@ -207,6 +209,7 @@ public Builder( this.indexCreatedVersion = indexCreatedVersion; this.knnMethodConfigContext = knnMethodConfigContext; this.originalParameters = originalParameters; + this.isKnnIndex = true; } @Override @@ -262,7 +265,7 @@ public KNNVectorFieldMapper build(BuilderContext context) { ); } - if (originalParameters.getResolvedKnnMethodContext() == null) { + if (originalParameters.getResolvedKnnMethodContext() == null || isKnnIndex == false) { return FlatVectorFieldMapper.createFieldMapper( buildFullName(context), name, @@ -374,10 +377,14 @@ public Mapper.Builder parse(String name, Map node, ParserCont String.format(Locale.ROOT, "Method and model can not be both specified in the mapping: %s", name) ); } - // Check for flat configuration if (isKNNDisabled(parserContext.getSettings())) { - validateFromFlat(builder); + builder.setKnnIndex(false); + if (parserContext.indexVersionCreated().onOrAfter(Version.V_2_17_0)) { + // on and after 2_17_0 we validate to makes sure that mapping doesn't contain parameters that are + // specific to approximate knn search algorithms + validateFromFlat(builder); + } } else if (builder.modelId.get() != null) { validateFromModel(builder); } else { diff --git a/src/test/java/org/opensearch/knn/index/OpenSearchIT.java b/src/test/java/org/opensearch/knn/index/OpenSearchIT.java index 3d6137db9..0d81af5f7 100644 --- a/src/test/java/org/opensearch/knn/index/OpenSearchIT.java +++ b/src/test/java/org/opensearch/knn/index/OpenSearchIT.java @@ -907,4 +907,27 @@ private List getResults(final String indexName, final String fieldNam return parseSearchResponse(searchResponseBody, fieldName); } + public void testCreateNonKNNIndex_withKNNModelID() throws Exception { + Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build(); + ResponseException ex = expectThrows( + ResponseException.class, + () -> createKnnIndex(INDEX_NAME, settings, createKnnIndexMapping(FIELD_NAME, "random-model-id")) + ); + String expMessage = "Cannot set modelId or method parameters when index.knn setting is false"; + assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage)); + } + + public void testCreateNonKNNIndex_withKNNMethodParams() throws Exception { + Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build(); + ResponseException ex = expectThrows( + ResponseException.class, + () -> createKnnIndex( + INDEX_NAME, + settings, + createKnnIndexMapping(FIELD_NAME, 2, "hnsw", KNNEngine.FAISS.getName(), SpaceType.DEFAULT.getValue(), false) + ) + ); + String expMessage = "Cannot set modelId or method parameters when index.knn setting is false"; + assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage)); + } } diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 03a99c0bd..92118e737 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -411,6 +411,22 @@ protected String createKnnIndexMapping(final String fieldName, final Integer dim .toString(); } + /** + * Utility to create a Knn Index Mapping with model id + */ + protected String createKnnIndexMapping(String fieldName, String modelId) throws IOException { + return XContentFactory.jsonBuilder() + .startObject() + .startObject("properties") + .startObject(fieldName) + .field("type", "knn_vector") + .field("model_id", modelId) + .endObject() + .endObject() + .endObject() + .toString(); + } + /** * Utility to create a Knn Index Mapping with specific algorithm and engine */