Skip to content
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

Move more token filters to analysis-common module #25384

Conversation

martijnvg
Copy link
Member

The following token filters were moved: stemmer, stemmer_override, kstem, dictionary_decompounder, hyphenation_decompounder, reverse, elision and truncate.

Relates to #23658

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.

Left a few small things but LGTM.

@@ -458,7 +456,6 @@ public void testPrefixLength() throws IOException {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties")
.startObject("body").field("type", "text").field("analyzer", "body").endObject()
.startObject("body_reverse").field("type", "text").field("analyzer", "reverse").endObject()
Copy link
Member

Choose a reason for hiding this comment

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

Weird that this wasn't used!

@@ -17,10 +17,6 @@
},
"my":{
"type":"myfilter"
},
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it is time to remove this entire file somehow!

Copy link
Member

Choose a reason for hiding this comment

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

Oh! I see. You don't need to get rid of it because you can plugin mocks into it. Great.

Copy link
Member

Choose a reason for hiding this comment

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

Naming this the same as the one for compound analysis seems bad. Maybe each one should have its own name.

@@ -67,6 +68,39 @@ public CommonAnalysisFactoryTests() {
filters.put("uppercase", UpperCaseTokenFilterFactory.class);
filters.put("ngram", NGramTokenFilterFactory.class);
filters.put("edgengram", EdgeNGramTokenFilterFactory.class);
filters.put("bulgarianstem", StemmerTokenFilterFactory.class);
Copy link
Member

Choose a reason for hiding this comment

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

Woah. Makes sense, but woah.

The following token filters were moved: stemmer, stemmer_override, kstem, dictionary_decompounder, hyphenation_decompounder, reverse, elision and truncate.

Relates to elastic#23658
@martijnvg martijnvg force-pushed the move_more_token_filters_to_analyis-commons_module2 branch from 3ac20d1 to a34f5fa Compare June 26, 2017 08:04
@martijnvg martijnvg merged commit a34f5fa into elastic:master Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants