fix: change offset calculation order to work with prefix matches #1
Conversation
This file contains 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
If a match was found exactly at the beginning of a haystack (see the new supplied test), the scanner would panic when trying to subtract `pat_len` from `pos`, as `pat_len` would always be larger than `pos` in this case.
tvlbot
pushed a commit
to tvlfyi/tvix
that referenced
this pull request
Feb 4, 2023
Our fork fixes a small bug (jneem/wu-manber#1) but it's not clear whether upstream will accept patches, so for now lets point this directly at our fork. Change-Id: Iccdcedae3e9a8b783241431787c952561d032694 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8031 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
tazjin
added a commit
to tvlfyi/tvix
that referenced
this pull request
Feb 28, 2023
Our fork fixes a small bug (jneem/wu-manber#1) but it's not clear whether upstream will accept patches, so for now lets point this directly at our fork. Change-Id: Iccdcedae3e9a8b783241431787c952561d032694 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8031 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
|
I'm closing this PR as this repo is unmaintained, and we are starting to land other changes in the Feel free to pull our commits in the future if you are interested in the improvements! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
If a match was found exactly at the beginning of a haystack (see the new supplied test), the scanner would panic when trying to subtract
pat_lenfrompos, aspat_lenwould always be larger thanposin this case.In case you're wondering "Why is someone randomly sending a PR to this 7 year old, seemingly unmaintained repo?": We are working on a Rust implementation of Nix, called Tvix. In a part of this project we have a potentially huge number of needles which are all equal length, very random strings (hashes of other data), and need to find their occurences in a set of other matches. The set of needles frequently changes and we need to do this scanning hundreds/thousands of times during a single run.
Especially the fact that the set changes quickly makes automaton-based solutions fairly inefficient, as we're doing expensive construction and throwing them away all the time. We tried the famous
aho-corasickRust implementation - it increased runtime by 5x, and ended up being most of what the program did (apart from disk IO). After reading some of the literature, I found that the Wu-Manber algorithm is likely a good fit for our problem space, and your crate was the only one implementing it already (I wasn't quite convinced enough to start a from-scratch implementation, though). It is significantly better performing for us already, but we had to squash this bug :)Thanks for publishing this!