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

_analyze api does not correctly use normalizers when specified #48866

Merged
merged 3 commits into from
Nov 14, 2019

Conversation

gaobinlong
Copy link
Contributor

#48650
_analyze api does not correctly use normalizers when specified, because the custom normalizer has never been used. The unit test function-testNormalizerWithIndex() in TransportAnalyzeActionTests class passed because the test case is simple.

This commit fix the bug and use another test case in testNormalizerWithIndex.

@elasticcla
Copy link

Hi @gaobinlong, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@gaobinlong
Copy link
Contributor Author

Hi @elasticcla , sorry, my fault, I have added the e-mail used in the commit into my Github profile. Is there someting else need to do?

@jrodewig jrodewig added the :Search Relevance/Analysis How text is split into tokens label Nov 7, 2019
@elasticmachine
Copy link
Collaborator

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

@cbuescher cbuescher self-assigned this Nov 11, 2019
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @gaobinlong, great catch and the fix and the test changes look great to me!
I left a short comment and a suggestion around explaining the assumptions in the test better but other than that LGTM. I will wait for you to come back before running some final tests before merging.

Add a comment to explain the result when using custom normalizer
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Thanks for the update, LGTM. I will run our CI tests before merging.

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher
Copy link
Member

@elasticmachine update branch

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher cbuescher merged commit a666fb2 into elastic:master Nov 14, 2019
cbuescher pushed a commit that referenced this pull request Nov 14, 2019
Currently the `_analyze` endpoint doesn't correctly use normalizers specified
in the request. This change fixes that by returning the resolved normalizer from
TransportAnalyzeAction#getAnalyzer and updates test to be able to catch this
in the future.

Closes #48650
@cbuescher
Copy link
Member

Thanks again @gaobinlong for working on this bug, the PR was much appreciated. I merged it to master and the 7.x branch, so the fix will be included in the upcoming 7.6 release.

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.

6 participants