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.

Sign up for GitHub

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

Jump to bottom

Add preserve_original setting in ngram token filter #55432

Merged
merged 1 commit into from
May 4, 2020
Merged

Add preserve_original setting in ngram token filter #55432

merged 1 commit into from
May 4, 2020

Conversation

amitmbm
Copy link
Contributor

@amitmbm amitmbm commented Apr 19, 2020

No description provided.

@amitmbm amitmbm mentioned this pull request Apr 19, 2020
Closed
@markharwood markharwood added the :Search Relevance/Analysis How text is split into tokens label Apr 20, 2020
@elasticmachine
Copy link
Collaborator

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

@markharwood
Copy link
Contributor

Thanks for this contribution, @amitmbm

A couple of extra things to consider -

  1. Adding a test here
  2. Documenting the parameter here

@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 20, 2020

@markharwood thanks for the suggestions, I'll address them soon and let you know.

@markharwood
Copy link
Contributor

Adding a test here

My bad - that's the wrong test class - it's for the NGramTokenizer rather than the NGramTokenFilter you modified. I'm not sure there is an existing class for testing that TokenFilter

@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 20, 2020

@markharwood Sorry I forgot, yeah yesterday I searched for the test class of this but couldn't find one and should have mentioned that.

Also, I added the documentation of newly added preserve_original param.

Now please let me know if I need to do something else, would like to close this before I go to bed today :)

@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 22, 2020

@markharwood Hope you are doing well. Please go through my last commit and let me know if I need to address some more comments.

markharwood
markharwood reviewed Apr 24, 2020
View reviewed changes
docs/reference/analysis/tokenfilters/ngram-tokenfilter.asciidoc
`preserve_original`::
(Optional, boolean)
If set to true then it would also emit the original token. Defaults to `true`.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default is false? (And needs to be for backwards-compatibility reasons).

Copy link
Contributor Author

@amitmbm amitmbm Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@markharwood thanks for pointing this, Addressed it in my latest comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markharwood I also rebased my feature branch with the latest master code and squashed my commits. Let me know if anything else required from my side.

@amitmbm amitmbm force-pushed the feature/expose-preserve-original-in-ngram-token-filter branch from a55e1b4 to 6e45024 Compare April 24, 2020 15:25
@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 25, 2020

@markharwood Added the missing test file NGramTokenFilterFactoryTests and ran the test locally and these are passing. and the following is the output:

A build scan will not be published due to this build running offline.
1:21:23 PM: Tasks execution finished ':modules:analysis-common:cleanTest :modules:analysis-common:test --tests "org.elasticsearch.analysis.common.NGramTokenFilterFactoryTests"'.

@amitmbm amitmbm force-pushed the feature/expose-preserve-original-in-ngram-token-filter branch from 4ebc046 to f1dcee9 Compare April 28, 2020 10:58
@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 28, 2020

@elasticmachine ok to test

@amitmbm amitmbm force-pushed the feature/expose-preserve-original-in-ngram-token-filter branch from da2bf3d to 5059c42 Compare April 28, 2020 11:07
@amitmbm amitmbm mentioned this pull request Apr 28, 2020
Merged
@markharwood
Copy link
Contributor

Hi Amit.
Apologies for not replying sooner - I've been tied up on adding a different feature..
I see Christoph has helped in the meantime and another PR has been merged.
Can I close this now?
Thanks very much for your contribution!

@amitmbm
Copy link
Contributor Author

amitmbm commented Apr 29, 2020

@markharwood It's OK I can understand. Christoph helped me with similar PR in edge-ngram token-filter while this PR is for ngram token filter, it also needs to be merged.

Could you please do the final review, please note that I've added a missing test class as well, as pointed by you earlier and addressed all other comments already.

@markharwood markharwood merged commit 00fef6d into elastic:master May 4, 2020
markharwood pushed a commit to markharwood/elasticsearch that referenced this pull request May 4, 2020
@amitmbm @markharwood
Analysis enhancement - add preserve_original setting in ngram-token-f…
f4e97b9 
…ilter (elastic#55432)
@markharwood markharwood mentioned this pull request May 4, 2020
Merged
markharwood added a commit that referenced this pull request May 4, 2020
@markharwood
Analysis enhancement - add preserve_original setting in ngram-token-f…
e197b6c 
…ilter (#55432) (#56100)

Authored-by: Amit Khandelwal <[email protected]>
@markharwood
Copy link
Contributor

Many thanks for this, Amit.
I've merged to master (8.0) and backported to 7.x (7.8) branches.

@pugnascotia pugnascotia changed the title Completed TODO task of adding preserve_original setting in ngram toke… Add preserve_original setting in ngram token filter May 6, 2020
@russcam russcam mentioned this pull request May 29, 2020
Closed
17 tasks
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 4, 2020
@russcam
Add PreserveOriginal to NGramTokenFilter
dc33de1 
Relates: #4718, elastic/elasticsearch#55432
@russcam russcam mentioned this pull request Jun 4, 2020
Merged
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
@russcam
Add PreserveOriginal to NGramTokenFilter (#4736)
e924689 
Relates: #4718, elastic/elasticsearch#55432
Added in Elasticsearch 7.8.0
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
@russcam @github-actions
Add PreserveOriginal to NGramTokenFilter (#4736)
ec9a071 
Relates: #4718, elastic/elasticsearch#55432
Added in Elasticsearch 7.8.0
github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
@russcam @github-actions
Add PreserveOriginal to NGramTokenFilter (#4736)
1d5e0c7 
Relates: #4718, elastic/elasticsearch#55432
Added in Elasticsearch 7.8.0
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
@github-actions @russcam
Add PreserveOriginal to NGramTokenFilter (#4736) (#4755)
690fa58 
Relates: #4718, elastic/elasticsearch#55432
Added in Elasticsearch 7.8.0

Co-authored-by: Russ Cam <[email protected]>
russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020
@github-actions @russcam
Add PreserveOriginal to NGramTokenFilter (#4736) (#4756)
fd76897 
Relates: #4718, elastic/elasticsearch#55432
Added in Elasticsearch 7.8.0

Co-authored-by: Russ Cam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markharwood markharwood markharwood left review comments

Assignees
No one assigned
Labels
>enhancement :Search Relevance/Analysis How text is split into tokens v7.8.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amitmbm @elasticmachine @markharwood @jakelandis @pugnascotia