Rework similarities to use lucene's PerFieldSimilarityWrapper on MapperService #57053
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
If this works then we should also be able to do the same thing for the index, search and search quote analyzers, further simplifying the MappedFieldType class. |
jtibshirani
reviewed
May 26, 2020
| @@ -172,6 +161,10 @@ public final String simpleName() { | |||
| * Both {@code this} and {@code mergeWith} will be left unmodified. */ | |||
| public abstract Mapper merge(Mapper mergeWith); | |||
|
|
|||
| public void collectPerFieldResources(PerFieldResourceCollector collector) { | |||
There was a problem hiding this comment.
If I'm understanding correctly, it's still possible to set a similarity on a non-text field, but we now ignore it. Although I agree that similarities aren't nearly as useful for non-text fields, this behavior seems trappy and it's technically a breaking change.
My vote would be to maintain the previous behavior where similarities on types like keyword were respected. If we want to disallow setting similarities on non-text types, we could open a dedicated issue to discuss it.
|
Superseded by #58439 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similarity providers for text fields are currently held directly on MappedFieldType,
and accessing them requires both a MapperService and a SimilarityService. This
makes things unnecessarily complicated (for example, Mapper.ParserContext needs
to hold references to both), and clutters the field type interface with
a field that only makes sense for text-based mappers.
This PR refactors things so that the MapperService is responsible for providing a
Similarity for an index, using lucene's PerFieldSimilarityWrapper to make field
configurations possible. Similarities for text fields are collected by visiting all
FieldMappers after a merge, and combining them appropriately.