Skip to content

String distance algorithms cleanup#27640

Merged
DaveCTurner merged 7 commits intoelastic:masterfrom
olcbean:distance_alg_cleanup
Dec 11, 2017
Merged

String distance algorithms cleanup#27640
DaveCTurner merged 7 commits intoelastic:masterfrom
olcbean:distance_alg_cleanup

Conversation

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

elasticmachine commented Dec 4, 2017

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

@olcbean olcbean Dec 4, 2017

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

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

@DaveCTurner DaveCTurner Dec 4, 2017

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

@olcbean olcbean Dec 5, 2017

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

DaveCTurner commented Dec 4, 2017

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

@DaveCTurner DaveCTurner Dec 5, 2017

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

@jasontedor jasontedor Dec 5, 2017

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

@olcbean olcbean Dec 5, 2017

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

DaveCTurner commented Dec 5, 2017

@elasticmachine please test this

@DaveCTurner
Copy link
Copy Markdown
Member

DaveCTurner commented Dec 5, 2017

LGTM. Kicked off a CI run.

@DaveCTurner
Copy link
Copy Markdown
Member

DaveCTurner commented Dec 7, 2017

@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
Copy Markdown
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
Copy Markdown
Member

DaveCTurner commented Dec 8, 2017

@elasticmachine please test this

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

DaveCTurner commented Dec 11, 2017

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