Skip to content

Move MappedFieldType.similarity() to TextSearchInfo #58439

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.

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

Merged
merged 10 commits into from
Jun 24, 2020

Conversation

romseygeek
Copy link
Contributor

Similarities only apply to a few text-based field types, but are currently set directly on
the base MappedFieldType class. This commit moves similarity information into
TextSearchInfo, and removes any mentions of it from MappedFieldType or FieldMapper.

It was previously possible to include a similarity parameter on a number of field types
that would then ignore this information. To make it obvious that this has no effect, setting
this parameter on non-text field types now issues a deprecation warning.

@romseygeek romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.9.0 labels Jun 23, 2020
@romseygeek romseygeek self-assigned this Jun 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Mapping)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 23, 2020
}
// TODO should we allow to configure the prefix field
}
parseTextField(builder, name, node, parserContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shared parseTextField() method does a deprecation check for similarity now, so we need to move it to after we've processed the similarity field. To ensure that mapper-specific fields still get checked for null, a new TypeParsers.checkNull() method is called for each entry.

}
}
parseTextField(builder, fieldName, node, parserContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same process here as in SearchAsYouTypeFieldMapper

new Modifier("similarity", false, (a, b) -> {
a.similarity(new SimilarityProvider("BM25", new BM25Similarity()));
b.similarity(new SimilarityProvider("boolean", new BooleanSimilarity()));
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been moved directly into the applicable FieldMappers

@romseygeek romseygeek requested review from nik9000 and javanna June 23, 2020 14:45
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

@romseygeek romseygeek merged commit 83ce7a9 into elastic:master Jun 24, 2020
@romseygeek romseygeek deleted the mapper/similarity branch June 24, 2020 08:55
romseygeek added a commit that referenced this pull request Jun 24, 2020
Similarities only apply to a few text-based field types, but are currently set directly on
the base MappedFieldType class. This commit moves similarity information into
TextSearchInfo, and removes any mentions of it from MappedFieldType or FieldMapper.

It was previously possible to include a similarity parameter on a number of field types
that would then ignore this information. To make it obvious that this has no effect, setting
this parameter on non-text field types now issues a deprecation warning.
@@ -163,7 +163,8 @@ Similarity getDefaultSimilarity() {
@Override
public Similarity get(String name) {
MappedFieldType fieldType = mapperService.fieldType(name);
return (fieldType != null && fieldType.similarity() != null) ? fieldType.similarity().get() : defaultSimilarity;
return (fieldType != null && fieldType.getTextSearchInfo().getSimilarity() != null)
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to avoid having null similarity provider in TextSearchInfo and rather pass in directly the default similarity, or have some placeholder instead of null?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants