Deprecating `levenstein` in favor of `levensHtein` #27409

olcbean · 2017-11-16T10:06:34Z

elasticmachine · 2017-11-16T10:06:37Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

DaveCTurner · 2017-11-16T15:32:54Z

Thanks @olcbean but I'm afraid it's a bit more complicated than this. This is a breaking change, so we need to support both spellings, and emit a deprecation warning for the incorrect one, before the incorrect one can be removed in a subsequent major release.

olcbean · 2017-11-16T15:56:52Z

@DaveCTurner do you mean to deprecate it for 6.1?
Just one more question : do you prefer to have 2 PR against master (deprecation and removal) or 1 against 6.1 and one against master

rjernst · 2017-11-16T21:08:24Z

@olcbean 2 PRs would be normal, do the deprecation first, then removal in master as a followup.

DaveCTurner · 2017-11-17T08:18:22Z

Yes, 2 PRs please, both against master, and we will backport the deprecation one to 6.x: if the deprecation PR was only merged into 6.x and then the removal PR stalled for some reason then we'd have a regression.

Anticipating future feedback (because I forgot last time I did something similar): there should be a test that the deprecation warning is emitted on the incorrect spelling.

olcbean · 2017-11-17T15:21:41Z

Thanks @DaveCTurner
I just deprecated the levenstein

DaveCTurner · 2017-11-18T10:44:11Z

core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java

        distanceVal = distanceVal.toLowerCase(Locale.US);
        if ("internal".equals(distanceVal)) {
            return DirectSpellChecker.INTERNAL_LEVENSHTEIN;
        } else if ("damerau_levenshtein".equals(distanceVal) || "damerauLevenshtein".equals(distanceVal)) {
            return new LuceneLevenshteinDistance();
        } else if ("levenstein".equals(distanceVal)) {
+            DEPRECATION_LOGGER.deprecated("Deprecated distance [levenstein] used, replaced by [levenshtein]");
+            return new LevensteinDistance();
+        } else if ("levenshtein".equals(distanceVal)) {
            return new LevensteinDistance();
            // TODO Jaro and Winkler are 2 people - so apply same naming logic


It seems a shame to leave this TODO here. Would you care to fix that too?

No scope creep please. 😄

DaveCTurner · 2017-11-18T10:48:18Z

Looking good. I added a request for some scope creep :)

@elasticmachine please test this.

jasontedor

I left a comment.

jasontedor · 2017-11-18T12:23:26Z

core/src/test/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorTests.java

@@ -65,6 +67,11 @@ public void testEqualsAndHashcode() throws IOException {
        }
    }

+    public void testLevensteinDeprecation() {
+        assertThat(DirectCandidateGeneratorBuilder.resolveDistance("levenstein"), equalTo(new LevensteinDistance()));


I might be missing something but I do not see a test that resolveDistance("levenshtein") does the right thing and does not produce a deprecation warning?

Good catch!

olcbean · 2017-11-20T07:37:37Z

@DaveCTurner I will clean the Jaro and Winkler TODO once this PR is merged (I want to reuse the test I introduced in this PR)

jasontedor · 2017-11-20T17:52:20Z

test this please

* master: (41 commits) [Test] Fix AggregationsTests#testFromXContentWithRandomFields [DOC] Fix mathematical representation on interval (range) (elastic#27450) Update version check for CCS optional remote clusters Bump BWC version to 6.1.0 for elastic#27469 Adapt rest test BWC version after backport Fix dynamic mapping update generation. (elastic#27467) Use the primary_term field to identify parent documents (elastic#27469) Move composite aggregation to core (elastic#27474) Fix test BWC version after backport Protect shard splitting from illegal target shards (elastic#27468) Cross Cluster Search: make remote clusters optional (elastic#27182) [Docs] Fix broken bulleted lists (elastic#27470) Move resync request serialization assertion Fix resync request serialization Fix issue where pages aren't released (elastic#27459) Add YAML REST tests for filters bucket agg (elastic#27128) Remove tcp profile from low level nio channel (elastic#27441) [TEST] Fix `GeoShapeQueryTests#testPointsOnly` failure Transition transport apis to use void listeners (elastic#27440) AwaitsFix GeoShapeQueryTests#testPointsOnly elastic#27454 ...

jasontedor · 2017-11-21T20:12:00Z

retest this please

jasontedor

LGTM.

jasontedor · 2017-11-23T02:32:06Z

@DaveCTurner If this looks good to you, would you mind pulling this in? I'm happy to walk you through the process, I think this would be your first time pulling a community PR in?

Support both spellings thoughout 6.x, reporting the incorrect one as deprecated.

DaveCTurner · 2017-11-23T12:59:26Z

LGTM, thanks @olcbean!

Backported to 6.x as 19afeaf but not to 6.1 as that branch is now feature-frozen.

* es/master: (38 commits) Backport wait_for_initialiazing_shards to cluster health API Carry over version map size to prevent excessive resizing (#27516) Fix scroll query with a sort that is a prefix of the index sort (#27498) Delete shard store files before restoring a snapshot (#27476) Replace `delimited_payload_filter` by `delimited_payload` (#26625) CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512) Fix merging of _meta field (#27352) Remove unused method (#27508) unmuted test, this has been fixed by #27397 Consolidate version numbering semantics (#27397) Add wait_for_no_initializing_shards to cluster health API (#27489) [TEST] use routing partition size based on the max routing shards of the second split Adjust CombinedDeletionPolicy for multiple commits (#27456) Update composite-aggregation.asciidoc Deprecate `levenstein` in favor of `levenshtein` (#27409) Automatically prepare indices for splitting (#27451) Validate `op_type` for `_create` (#27483) Minor ShapeBuilder cleanup muted test Decouple nio constructs from the tcp transport (#27484) ...

* es/6.x: (30 commits) Add wait_for_no_initializing_shards to cluster health API (#27489) Carry over version map size to prevent excessive resizing (#27516) Fix scroll query with a sort that is a prefix of the index sort (#27498) Delete shard store files before restoring a snapshot (#27476) CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512) Fix merging of _meta field (#27352) test: do not run percolator query builder bwc test against 5.x versions Remove unused method (#27508) Consolidate version numbering semantics (#27397) Adjust CombinedDeletionPolicy for multiple commits (#27456) Minor ShapeBuilder cleanup [GEO] Deprecate ShapeBuilders and decouple geojson parse logic Improve docs for split API in 6.1/6.x (#27504) test: use correct pre 6.0.0-alpha1 format Update composite-aggregation.asciidoc Deprecate `levenstein` in favor of `levenshtein` (#27409) Decouple nio constructs from the tcp transport (#27484) Bump version from 6.1 to 6.2 Fix whitespace in Security.java Tighten which classes can exit ...

#27409 deprecated the incorrectly-spelled `levenstein` in favour of `levenshtein`. #27526 deprecated the inconsistent `jarowinkler` in favour of `jaro_winkler`. These changes were merged into 6.2, and this change removes them entirely in 7.0.

replacing levenstein with levensHtein

f660b39

DaveCTurner added >breaking >bug v7.0.0 :Search Relevance/Suggesters "Did you mean" and suggestions as you type labels Nov 17, 2017

olcbean force-pushed the levenshtein branch from a455dea to 820d507 Compare November 17, 2017 15:14

deprecate levestein

0d89c26

olcbean force-pushed the levenshtein branch from 820d507 to 0d89c26 Compare November 17, 2017 15:17

olcbean changed the title ~~Replacing levenstein with levensHtein~~ Deprecating levenstein in favor of levensHtein Nov 17, 2017

DaveCTurner reviewed Nov 18, 2017

View reviewed changes

DaveCTurner added v6.1.0 and removed >breaking labels Nov 18, 2017

jasontedor reviewed Nov 18, 2017

View reviewed changes

added tests

3f281a7

jasontedor approved these changes Nov 23, 2017

View reviewed changes

jasontedor added v6.2.0 and removed v6.1.0 labels Nov 23, 2017

DaveCTurner merged commit fd564b1 into elastic:master Nov 23, 2017

DaveCTurner pushed a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 23, 2017

Deprecate levenstein in favor of levenshtein (elastic#27409)

19afeaf

Support both spellings thoughout 6.x, reporting the incorrect one as deprecated.

olcbean mentioned this pull request Nov 25, 2017

deprecating jarowinkler in favor of jaro_winkler #27526

Merged

clintongormley added the >deprecation label Nov 27, 2017

codebrain mentioned this pull request May 23, 2018

[6.2] Deprecating levenstein in favor of levensHtein elastic/elasticsearch-net#3251

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Deprecating levenstein in favor of levensHtein #27409

Deprecating levenstein in favor of levensHtein #27409

Uh oh!

Conversation

olcbean commented Nov 16, 2017

Uh oh!

elasticmachine commented Nov 16, 2017

Uh oh!

DaveCTurner commented Nov 16, 2017

Uh oh!

olcbean commented Nov 16, 2017

Uh oh!

rjernst commented Nov 16, 2017

Uh oh!

DaveCTurner commented Nov 17, 2017

Uh oh!

olcbean commented Nov 17, 2017

Uh oh!

DaveCTurner Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Nov 18, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

olcbean Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

olcbean commented Nov 20, 2017

Uh oh!

jasontedor commented Nov 20, 2017

Uh oh!

jasontedor commented Nov 21, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Nov 23, 2017

Uh oh!

DaveCTurner commented Nov 23, 2017

Uh oh!

Uh oh!

Deprecating `levenstein` in favor of `levensHtein` #27409

Deprecating `levenstein` in favor of `levensHtein` #27409