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 pre-configured "keyword" tokenizer to the analysis-common module #24863

Merged
merged 4 commits into from
Jun 16, 2017

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented May 24, 2017

Moves the keyword tokenizer to the analysis-common module. The keyword tokenizer is special because it is used by CustomNormalizerProvider so I pulled it out into its own PR. To get the move to work I've reworked the lookup from static to one using the AnalysisRegistry. This seems safe enough.

Part of #23658.

@nik9000
Copy link
Member Author

nik9000 commented May 30, 2017

@jpountz, can you have a look at this one?

@jpountz
Copy link
Contributor

jpountz commented May 30, 2017

Does it mean we can't get normalization to work if analysis-common is not on the classpath?

@nik9000
Copy link
Member Author

nik9000 commented May 30, 2017

Does it mean we can't get normalization to work if analysis-common is not on the classpath?

Yes.

@jpountz jpountz self-requested a review June 8, 2017 07:58
@jpountz
Copy link
Contributor

jpountz commented Jun 8, 2017

The change looks correct to me, but I'm a bit confused about how integration tests that rely on normalization work with this change? We don't put analysis-common on the classpath when running integ tests, do we?

@nik9000
Copy link
Member Author

nik9000 commented Jun 14, 2017

I suspect we don't actually test custom normalizers without analysis-common installed. We have unit tests but they don't need run this particular code. I could only find some documentation that builds a custom normalizer. I didn't see anything in the rest-api-spec.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

OK I got it now, existing tests that make sure we apply normalization are done on predefined analyzers such as the standard analyzer, this is why it keeps working. Sorry for the confusion. LGTM.

@nik9000
Copy link
Member Author

nik9000 commented Jun 15, 2017

OK I got it now, existing tests that make sure we apply normalization are done on predefined analyzers such as the standard analyzer, this is why it keeps working. Sorry for the confusion. LGTM.

Thanks for reviewing! I'll see about removing the conflicts later today.

@nik9000 nik9000 merged commit ecc87f6 into elastic:master Jun 16, 2017
@nik9000
Copy link
Member Author

nik9000 commented Jun 16, 2017

Thanks for reviewing @jpountz!

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