Skip to content

String distance algorithms cleanup #27640

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
Merged
merged 7 commits into from
Dec 11, 2017
Merged

String distance algorithms cleanup #27640

merged 7 commits into from
Dec 11, 2017

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Dec 4, 2017

Remove deprecated levenstein and jarowinkler string distance algs

Remove damerauLevenshtein : distanceVal is explicitly converted to lowercase, thus it can never match damerauLevenshtein

Use the ROOT locale for to lowercase

@elasticmachine
Copy link
Collaborator

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?

olcbean
olcbean commented Dec 4, 2017
View reviewed changes
core/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java
if ("internal".equals(distanceVal)) {
return DirectSpellChecker.INTERNAL_LEVENSHTEIN;
} else if ("damerau_levenshtein".equals(distanceVal) || "damerauLevenshtein".equals(distanceVal)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

distanceVal is explicitly converted to lowercase, thus it can never match damerauLevenshtein

@DaveCTurner DaveCTurner added :Search Relevance/Suggesters "Did you mean" and suggestions as you type v7.0.0 labels Dec 4, 2017
DaveCTurner
DaveCTurner requested changes Dec 4, 2017
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I asked for one wording change, otherwise I'm happy with this.

docs/reference/migration/migrate_7_0/search.asciidoc Outdated

==== Term Suggesters supported distance algorithms

The following suggestion distance algorithms have been removed:
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds a little more dramatic than it needs to: we haven't removed the algorithms, just the deprecated and non-preferred spellings. Could you reword more gently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @DaveCTurner! Can you have another look?

@DaveCTurner
Copy link
Contributor

@elasticmachine please test this.

@olcbean olcbean force-pushed the distance_alg_cleanup branch from 995b400 to 6cfb15c Compare December 5, 2017 10:58
DaveCTurner
DaveCTurner reviewed Dec 5, 2017
View reviewed changes
docs/reference/migration/migrate_7_0/search.asciidoc Outdated
@@ -43,7 +43,8 @@ Scroll queries are not meant to be cached.

==== Term Suggesters supported distance algorithms

The following suggestion distance algorithms have been removed:
For better readability and consistency the following string distance algorithms
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

The following string distance algorithms were renamed in 6.0 and their old names were deprecated. The deprecated names have now been removed.

Copy link
Member

Choose a reason for hiding this comment

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

How about:

The following string distance algorithms were given additional names in 6.2 and their existing names were deprecated. The deprecated names have now been removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions!

@DaveCTurner
Copy link
Contributor

@elasticmachine please test this

@DaveCTurner
Copy link
Contributor

LGTM. Kicked off a CI run.

@DaveCTurner
Copy link
Contributor

@olcbean I think your branch is based on a commit in master which doesn't pass CI, as the failure here isn't obviously related to your changes and looks similar to some other failures from a few days ago. Could you merge in the latest master and we'll retry?

@olcbean olcbean force-pushed the distance_alg_cleanup branch from 52fd037 to 9f3a1b4 Compare December 7, 2017 13:41
@olcbean
Copy link
Contributor Author

olcbean commented Dec 7, 2017

@DaveCTurner thanks! I just rebased and pushed. Can you trigger another build?
( this issue has been fixed in 72800bb )

@DaveCTurner
Copy link
Contributor

@elasticmachine please test this

@DaveCTurner DaveCTurner merged commit 25c606c into elastic:master Dec 11, 2017
@DaveCTurner
Copy link
Contributor

Thanks for your work on this, @olcbean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor left review comments

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
>non-issue :Search Relevance/Suggesters "Did you mean" and suggestions as you type v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@olcbean @elasticmachine @DaveCTurner @jasontedor @colings86 @jpountz