Remove special-casing of Synonym filters in AnalysisRegistry#33868
Closed
romseygeek wants to merge 2 commits intoelastic:masterfrom
romseygeek:synonyms-to-common-analysis
Closed
Remove special-casing of Synonym filters in AnalysisRegistry#33868romseygeek wants to merge 2 commits intoelastic:masterfrom romseygeek:synonyms-to-common-analysis
romseygeek wants to merge 2 commits intoelastic:masterfrom
romseygeek:synonyms-to-common-analysis
Conversation
The synonym filters no longer need access to the AnalysisRegistry in their constructors, so we can remove the special-case code and move them to the common analysis module
Collaborator
|
Pinging @elastic/es-search-aggs |
75 tasks
martijnvg
reviewed
Sep 20, 2018
Member
martijnvg
left a comment
There was a problem hiding this comment.
Great to see these token filter being moved to analysis common module!
There are a couple of tests in MultiMatchQueryTests and TextFieldMapperTests (and I guess more when it gets to the integTest phase) that rely on the synonym token filter being available in server module. In the past I have dealing with these tests in the following ways:
- Verify whether the test is still valid and otherwise remove the test.
- Some tests are using a specific analysis component but don't really need to, so these tests can be changed to not use the analysis component in question.
- Use a mock analysis component. I think for this change we can use
MockSynonymFilterfrom the lucene test framework. - Move the test to the common-analysis module.
Can you also add a rest test to 40_token_filters.yml file? This is just to verify that everything is wired up correctly and a token filter can actually be used.
Contributor
Author
|
Lots of integration test failures: |
Contributor
Author
|
Closed in favour of #34034 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The synonym filters no longer need access to the AnalysisRegistry in their
constructors, so we can remove the special-case code and move them to the
common analysis module