Skip to content

Add preserve_original setting in ngram token filter#55432

Merged
markharwood merged 1 commit intoelastic:masterfrom
amitmbm:feature/expose-preserve-original-in-ngram-token-filter
May 4, 2020
Merged

Add preserve_original setting in ngram token filter#55432
markharwood merged 1 commit intoelastic:masterfrom
amitmbm:feature/expose-preserve-original-in-ngram-token-filter

Conversation

@amitmbm
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Apr 20, 2020

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

@markharwood
Copy link
Copy Markdown
Contributor

markharwood commented Apr 20, 2020

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
Copy Markdown
Contributor Author

amitmbm commented Apr 20, 2020

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

@markharwood
Copy link
Copy Markdown
Contributor

markharwood commented Apr 20, 2020

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
Copy Markdown
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
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Contributor

@markharwood markharwood Apr 24, 2020

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
Copy Markdown
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
Copy Markdown
Contributor Author

@amitmbm amitmbm Apr 24, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

markharwood commented Apr 29, 2020

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
Copy Markdown
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 <amitmbm87@gmail.com>
@markharwood
Copy link
Copy Markdown
Contributor

markharwood commented May 4, 2020

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 <russ.cam@elastic.co>
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 <russ.cam@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@markharwood markharwood markharwood left review comments

Reviewers whose approvals may not affect merge requirements

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