Add preserve_original setting in edge ngram token filter by amitmbm · Pull Request #55766 · elastic/elasticsearch

amitmbm · 2020-04-25T09:00:35Z

Raised this PR to address the bug #55767

…c and new test class

elasticmachine · 2020-04-27T07:24:44Z

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

cbuescher

HI @amitmbm, thanks for opening this PR, looks great. I only left a few very minor remarks around formatting etc., the rest is okay. I will enabling running the tests so everything should be run past CI once you push another commit.

cbuescher · 2020-04-27T13:08:08Z

...common/src/test/java/org/elasticsearch/analysis/common/EdgeNGramTokenFilterFactoryTests.java

+package org.elasticsearch.analysis.common;
+
+import org.apache.lucene.analysis.Tokenizer;
+import org.apache.lucene.analysis.core.WhitespaceTokenizer;


nit: this seems unused, our checkstyle rules will complain about unused imports, so better to remove it now before running the tests.

My intelliJ removed unused import wasn't configured for elasticsearch project, enabled it now :)

cbuescher · 2020-04-27T13:09:22Z

docs/reference/analysis/tokenfilters/edgengram-tokenfilter.asciidoc


+`preserve_original`::
+(Optional, boolean)
+If set to true then it would also emit the original token. Defaults to `false`.


nit: wording might be better sth like "Emits original token then set to true. Defaults to false.

changed to Emits original token when set to true. Defaults to false. --> notice changed to when from then in the suggested edit.

cbuescher · 2020-04-27T13:26:31Z

...common/src/test/java/org/elasticsearch/analysis/common/EdgeNGramTokenFilterFactoryTests.java

+/**
+ * Test class for edge_ngram token filter.
+ *
+ * @author Amit Khandelwal


nit: we usually don't add @author tags to classes or test classes but rely on the commit history rather than code comments to track authors

Ok, removed it.

cbuescher · 2020-04-27T13:26:51Z

...common/src/test/java/org/elasticsearch/analysis/common/EdgeNGramTokenFilterFactoryTests.java

+ * @author Amit Khandelwal
+ */
+public class EdgeNGramTokenFilterFactoryTests extends ESTokenStreamTestCase {
+    public void testDefault() throws IOException {


nit: maybe add newline befor first test method.

Just observed this in so many other test classes and copy-pasted the initial test setup :). nvm removed this.

cbuescher · 2020-04-27T13:28:42Z

@elasticmahine ok to test

cbuescher · 2020-04-27T17:25:49Z

@elasticmachine run elasticsearch-ci/bwc

amitmbm · 2020-04-27T17:36:13Z

@cbuescher thanks for kicking another test try for elasticsearch-ci/bwc, I looked at the test failures and it was related to UpgradeClusterClientYamlTestSuiteIT class which no way related to the code I've written and seems got failure due to timeout.

cbuescher · 2020-04-27T19:04:59Z

... which no way related to the code I've written

I agree, we'd still like to get a clean test run. Lets try this again.
@elasticmachine run elasticsearch-ci/bwc

cbuescher · 2020-04-27T20:21:28Z

@elasticmachine update branch

…gram-token-filter

amitmbm · 2020-04-27T22:24:31Z

@cbuescher looks like merging master into my feature branch fixed the test failures. Let me know if you can merge it if all looks OK.

cbuescher · 2020-04-28T08:23:52Z

Hi @amitmbm, I merged your change to master and will also port it to the latest 7.x branch. Thanks for picking this up.

The Lucene `preserve_original` setting is currently not supported in the `edge_ngram` token filter. This change adds it with a default value of `false`. Closes #55767

amitmbm · 2020-04-28T11:14:13Z

@cbuescher I'm really glad as it's my first commit merged to Elastic code base, I had raised another similar PR #55432 which is almost reviewed by your colleague Mark Harwood, but then there is no update on this PR from last 4 days. Hope he is safe and if you get time please look into this.

After this, I want to pick some more changes and one of them is deprecating XLowerCaseTokenizerFactory mentioned in
https://github.com/elastic/elasticsearch/blob/master/modules/analysis-common/src/main/java/org/elasticsearch/analysis/common/CommonAnalysisPlugin.java#L372 Please let me know how if there is any documentation on the deprecation process at Elastic? So that I can pick this issue and several others related to deprecation.

cbuescher · 2020-04-28T12:18:35Z

Thanks, great to hear you enjoyed working on the PR. We try to review user PRs in a timely manner but please don't expect anyone to respond to new commits etc... immediately because we all handle this differently and asynchronously.
Regarding deprecation processes: there is not one clear-cut approach, we generally aim at not changing / remove existing functionality in a minor version, and if we do so in a major version (e.g. 8.0) it is still preferred to provide a clear upgrade scenario, e.g. when removing a functionality, then we try to warn users on 7.x about the upcoming change of behaviour for example by returning warning messages with each http requerst and logging deprecation warnings. In the case that you mentioned, it's even a bit more complicated since existing indices (e.g. the ones from 7.x) still need to work with the analysis components used when they were created, so simply removing them on 8.0 isn't an option. We'd probably have to discuss the approach here in more detail on an issue.

amitmbm · 2020-04-28T13:22:58Z

@cbuescher I understand that Elastic as a whole company work in async mode and my intent is not to push my PRs for review, it was stuck so I thought to bring this to you notice. Anyway thanks a lot for explaining this and I would keep this in mind.

Also, reg. the deprecation changes, As you pointed out it requires more discussion, I would open a new issue and will discuss it there. Have a great day ahead 😄

mmoreram · 2022-03-07T16:54:16Z

Do we have any update on this PR? Did you translate the discussion?
Could you please take a look again and consider merging?
Thanks!

amitmbm · 2022-03-08T04:47:31Z

@mmoreram I raised PR but as @cbuescher mention in this comment , AS it's related to deprecation, hence he wanted to discuss this more with his team, I am also waiting for his update.

@cbuescher let me know if you have any update.

Todo of exposing preserve_original in edge-ngram token filter with do…

ba598da

…c and new test class

amitmbm force-pushed the feature/expose-preserve-original-in-edge-ngram-token-filter branch from 1c4cf4e to ba598da Compare April 25, 2020 09:05

astefan added the :Search Relevance/Analysis How text is split into tokens label Apr 27, 2020

cbuescher self-assigned this Apr 27, 2020

cbuescher reviewed Apr 27, 2020

View reviewed changes

Addressed minor formatting comments

106e2e9

Merge branch 'master' into feature/expose-preserve-original-in-edge-n…

b4a8d31

…gram-token-filter

cbuescher merged commit 9e41fed into elastic:master Apr 28, 2020

cbuescher added v7.8.0 v8.0.0 labels Apr 28, 2020

pugnascotia added the >bug label May 6, 2020

pugnascotia changed the title ~~Feature/expose preserve original in edge ngram token filter~~ Add preserve_original setting in edge ngram token filter May 7, 2020

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

amitmbm mentioned this pull request Feb 15, 2022

Side param in edge_ngram token filter is deprecated long back even in 2.X but code cleanup is not done #83934

Closed

Conversation

amitmbm commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 27, 2020

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

amitmbm Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

amitmbm Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

amitmbm Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

amitmbm Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Apr 27, 2020

Uh oh!

cbuescher commented Apr 27, 2020

Uh oh!

amitmbm commented Apr 27, 2020

Uh oh!

cbuescher commented Apr 27, 2020

Uh oh!

cbuescher commented Apr 27, 2020

Uh oh!

amitmbm commented Apr 27, 2020

Uh oh!

cbuescher commented Apr 28, 2020

Uh oh!

amitmbm commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbuescher commented Apr 28, 2020

Uh oh!

amitmbm commented Apr 28, 2020

Uh oh!

mmoreram commented Mar 7, 2022

Uh oh!

amitmbm commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

amitmbm commented Apr 25, 2020 •

edited

Loading

amitmbm Apr 27, 2020 •

edited

Loading

amitmbm commented Apr 28, 2020 •

edited

Loading