TEXT-188 Speed up LevenshteinDistance with threshold by exiting early#174
TEXT-188 Speed up LevenshteinDistance with threshold by exiting early#174vesterstroem wants to merge 2 commits intoapache:masterfrom
Conversation
kinow
left a comment
kinow
left a comment
There was a problem hiding this comment.
@vesterstroem CI passed on GitHub actions. I should be able to review it this weekend. Just want to sit down with ☕ and the IDE to debug and confirm I understand what changed, and also that it is being covered by our tests (quite sure it's).
Thanks for your pull request!
Bruno
| } | ||
| // if the lower bound is greater than the threshold, then exit early | ||
| if (lowerBound > threshold) { | ||
| return -1; |
There was a problem hiding this comment.
@vesterstroem it looks to me like this nullifies the if (p[n] <= threshold) { further down.
I was looking at the coverage report, and it seems to be that the coverage decreased because that return -1 never happens.
If so, I think we can remove it and simply return p[n];. WDYT?
There was a problem hiding this comment.
The improvement introduces the need for an extra test, which I forgot to add.
The assertion:
assertThat(new LevenshteinDistance(1).apply("abc", "acb")).isEqualTo(-1);
should be inserted into e.g. testGetLevenshteinDistance_StringStringInt of LevenshteinDistanceTest.
That will give coverage to the uncovered line - and it will fail, if we just always return p[n].
Should I make a new pull request or update the existing one?
There was a problem hiding this comment.
Better update this one. We can squash/edit commits later if necessary 👍 After you push a new commit I think the CI jobs will run again, and coveralls will confirm the code is being tested now.
Thanks!
|
Infra is restarting our JIRA. So will merge it later today @vesterstroem . Thanks again for your contribution and for adding the test 👍 |
|
Merged, with changelog added for 1.9.1. Thanks @vesterstroem |
You're welcome :-) |
3 participants
For details, see https://issues.apache.org/jira/browse/TEXT-188