Skip to content

Commit

Permalink
Move check to util class
Browse files Browse the repository at this point in the history
Signed-off-by: Vijayan Balasubramanian <[email protected]>
  • Loading branch information
VijayanB committed Oct 24, 2024
1 parent 0d1db53 commit 93f9c24
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.createKNNMethodContextFromLegacy;
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.createStoredFieldForByteVector;
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.createStoredFieldForFloatVector;
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.useKNNMethodContextFromLegacy;
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateIfCircuitBreakerIsNotTriggered;
import static org.opensearch.knn.index.mapper.KNNVectorFieldMapperUtil.validateIfKNNPluginEnabled;
import static org.opensearch.knn.index.mapper.ModelFieldMapper.UNSET_MODEL_DIMENSION_IDENTIFIER;
Expand Down Expand Up @@ -500,7 +501,7 @@ private void resolveKNNMethodComponents(
.build()
);

if (useKNNMethodContextFromLegacy(builder, parserContext)) {
if (useKNNMethodContextFromLegacy(builder.originalParameters, parserContext)) {
// Then create KNNMethodContext to be used from the legacy index settings
builder.originalParameters.setResolvedKnnMethodContext(
createKNNMethodContextFromLegacy(parserContext.getSettings(), parserContext.indexVersionCreated(), resolvedSpaceType)
Expand Down Expand Up @@ -549,12 +550,6 @@ 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.lucene.util.BytesRef;
import org.opensearch.Version;
import org.opensearch.common.settings.Settings;
import org.opensearch.index.mapper.Mapper;
import org.opensearch.knn.index.KNNSettings;
import org.opensearch.knn.index.KnnCircuitBreakerException;
import org.opensearch.knn.index.SpaceType;
Expand Down Expand Up @@ -193,6 +194,15 @@ private static int getEfConstruction(Settings indexSettings, Version indexVersio
return Integer.parseInt(efConstruction);
}

static boolean useKNNMethodContextFromLegacy(
OriginalMappingParameters originalMappingParameters,
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) && originalMappingParameters.isLegacyMapping();
}

static KNNMethodContext createKNNMethodContextFromLegacy(
Settings indexSettings,
Version indexCreatedVersion,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,13 @@ public void testTypeParser_withDifferentSpaceTypeCombinations_thenSuccess() thro
assertTrue(knnVectorFieldMapper.fieldType().getKnnMappingConfig().getModelId().isEmpty());

// mock useKNNMethodContextFromLegacy to simulate index ix created before 2_18
try (MockedStatic<KNNVectorFieldMapper> utilMockedStatic = Mockito.mockStatic(KNNVectorFieldMapper.class)) {
utilMockedStatic.when(() -> KNNVectorFieldMapper.useKNNMethodContextFromLegacy(any(), any())).thenReturn(true);
try (
MockedStatic<KNNVectorFieldMapperUtil> utilMockedStatic = Mockito.mockStatic(
KNNVectorFieldMapperUtil.class,
Mockito.CALLS_REAL_METHODS
)
) {
utilMockedStatic.when(() -> KNNVectorFieldMapperUtil.useKNNMethodContextFromLegacy(any(), any())).thenReturn(true);
// if space type is provided and legacy mappings is hit
xContentBuilder = XContentFactory.jsonBuilder()
.startObject()
Expand Down

0 comments on commit 93f9c24

Please sign in to comment.