feat: FST-based Curated Dictionary Spellchecking#258
Merged
Conversation
Closed
Collaborator
Author
|
Made it more correct (better utf-8 edit-distance support using the levenshtein-automata crate mentioned in #138) and a tiny bit faster by storing the DFA builders thread-locally 💪
|
elijah-potter
requested changes
Nov 4, 2024
Collaborator
There was a problem hiding this comment.
Grant, this is some really great work. There are a couple of test cases and organizational things that need to be looked at, but overall it's looking very good, particularly for the WASM target.
Let me know if you have any questions about my comments.
elijah-potter
force-pushed
the
fst-dictionary
branch
from
c4645ac to
d802b68
Compare
elijah-potter
requested changes
Nov 14, 2024
Collaborator
elijah-potter
left a comment
elijah-potter
left a comment
There was a problem hiding this comment.
I've got some more requests for changes. Still looks good. Let me figure out the performance problem.
It was at this moment I recognized the height of my folly.
- Removed words_iter() - Removed words_with_len_iter() - Added fuzzy_match() - Added fuzzy_match_str()
…xcept fuzzy search
…parsing The dictionary parsing is relatively isolated, and the parsing is needed for the harper-core build script.
… numbers from dictionary
…zzy find operations
…o pub(super) - get_word - get_word_metadata These two functions are needed by `FstDictionary`, but aren't something we need to expose publicly.
grantlemons
force-pushed
the
fst-dictionary
branch
from
1586d7b to
69cbae1
Compare
This reverts commit 0d5c600. Using `Lrc` in this position makes it much harder to support choosing `concurrency` support.
Has the added bonus of doubling test execution speed
elijah-potter
approved these changes
Nov 19, 2024
Collaborator
Author
|
Update: For correctness reasons it is now only 80% faster. |
grantlemons
commented
Nov 23, 2024
| // Let common words bubble up, but do not prioritize them over all else. | ||
| found.sort_by_key(|fmr| fmr.edit_distance + if fmr.metadata.common { 0 } else { 1 }); | ||
| // Make commonality relevant | ||
| found.sort_by_key(|fmr| if fmr.metadata.common { 0 } else { 1 }); |
Collaborator
Author
There was a problem hiding this comment.
Why did you remove the edit distance portion?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created a new dictionary
FstDictionarythat uses a finite-state transducer (FST)-based map underneath. This precomputed map is built based on the dictionary file at compile time, and allows for extremely fast edit-distance calculations used in the spellchecking logic.FstDictionaryis ideal for the curated dictionary, but is immutable, so the currentFullDictionaryimplementation is still used for user and file dictionaries.Using this new dictionary results in much faster spellchecking (about 10x faster when not yet cached per benchmark below).
One downside to this PR is that it moves the hunspell parsing stuff out into a new crate
harper-dictionary-parsing, which makes some of the imports a little weird. For instance,harper-corenow importsSpanfromharper-dictionary-parsing.Also, there is a noted bug in thefstlibrary with its handling of japanese characters, but for the time being this should not pose an issue as multi-language support is nowhere near being implemented. (#138, fst/#38)