Separate search between frontend and backend #39
Closed
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
…ds to avoid name clash.
…implemented to support search.
We can't use this yet as we need to create traits too, so we'll do RLFMI next.
Still need to refactor to simplify.
…l SearchIndex trait.
…we don't need to hide documentation either.
…tests into frontend and backend tests.
…c documentation anyway.
|
I should have looked better at github as I missed your own PR that was already there! I am okay with going further with that one, but I think some naming ideas could be taken from this one. |
|
I've withdrawn this PR in favor of yours. |
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.
This is a large PR unfortunately. Here's what happened:
the existing FMIndex and RLFMIndex structs become backend structs, implementing a backend trait.
define new FMIndex and RLFMIndex frontend structs that don't know anything about backend traits. This unfortunately requires quite a bit of code duplication. The alternative is to try to write a general frontend code on top of the backend trait. But this would require the backend trait to be public, meaning we have to do the whole seal thing again. I chose to make the backend completely hidden.
Also define new traits for the frontend in
frontend.rs. Those traits are not required to useFMIndexorRLFMIndexunless you want to use them in a generic way.Something I'm not super happy with: defined a new frontend traits
SearchIndexWithLocateandSearchWithLocate. This allows us to hide the marker traitsHasLocationentirely, but it makes it difficult to see in the documentation when thelocatefunctionality is available. I still hope we can find a better way to support this behavior.Made those tests that could work against the frontend API work against that, keeping those that test internals in the backend code.
right now
lenandis_emptyandsizearen't yet part of the frontend search index, but we could do that, what do you think?So in summary we have some wiring code that feels redundant in the frontend implementation but the benefit is that we can completely hide how it's all implemented.
The one thing exposed that I think we could consider not exposing as well is the suffix array. Right now we use this information to let the
locatemethod exist in type-driven way, but I'd prefer it if we could hide that a suffix array is involved entirely. I'm not sure what that looks like yet. (and we'd have to do the same for the doc array)