feat(sqlite-vec): enable keyword search for sqlite-vec#1439
Conversation
|
Hi @varshaprasad96! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
964f979 to
cdbf7f2
Compare
|
I've signed the CLA, it may take sometime to reflect. |
cdbf7f2 to
76c569e
Compare
76c569e to
f7eae78
Compare
franciscojavierarceo
left a comment
There was a problem hiding this comment.
Great work Varsha! Could you also update the docs in this PR as well?
|
|
||
| async def query(self, embedding: NDArray, k: int, score_threshold: float) -> QueryChunksResponse: | ||
| async def query( | ||
| self, embedding: NDArray, k: int, score_threshold: float, query_str: None, search_mode: None |
There was a problem hiding this comment.
can we instead introduce a new type of provider like "keyword_search_io"? Presumably there can be other full text search providers like elasticsearch or postgres ts_query that can be added in addition to sqlite-fts5.
There was a problem hiding this comment.
@raghotham maybe we should discuss this in an RFC?
I personally think that vector, keyword, and standard primary key lookup are all retrieval strategies for a database. I would worry making three separate providers may introduce too much of an abstraction.
That is largely how we've implemented things in @feast-dev (the sqlite example: feast-dev/feast#5082), which I had pointed @varshaprasad96 to prior to doing the implementation. Of course, that may not be how we want to design these things here, so definitely open to your thoughts here. I just wanted to be transparent with how I saw things.
|
@varshaprasad96 we should definitely draft an RFC here. Looking at LangChain, they tend to refer to things slightly differently and I kind of like their semantics. They refer to text search as lexical search to be more generic (ie.., BM25 , TF-IDF) and their Hybrid retrieval is called "Ensemble" retrieval. I think highlighting how other popular frameworks name things is helpful for us choosing something that's explicit, elegant, and intuitive for users. |
|
@franciscojavierarceo @raghotham Thanks for the inputs! As Francisco mentioned, the goal of this approach was to provide users with the flexibility to choose between different retrieval strategies—Vector-based, Keyword-based, or Hybrid—within the RAG pipeline. The level of support for these approaches varies across different databases; for example, ChromaDB does not fully support FTS. Given this, we felt it made more sense to integrate these options into the RAG pipeline rather than introduce a separate provider.
Makes sense, we will get a RFC ready and open it up for review. |
|
/hold - this is blocked by #1530. There will also be a RFC open to finalise the design before merging the PR. |
|
@varshaprasad96 any update on this? I see #1530 has merged, do we have a RFC somewhere? Thanks! |
|
@leseb Yes. I'm at a conference (kubecon) this week, but it's very much in radar. RFC is ready, and I'll open it for review soon with an implementation PR that can be referenced as a PoC. |
|
#1944 -> RFC for review. |
f7eae78 to
4f4aa02
Compare
be5b0fe to
e07fd65
Compare
|
Thanks for working on this and apologies I could not get to this earlier. It will allow for explicit |
|
Just a status update - I'm following up on this, and will push up the changes by eod today / early tomorrow. |
e07fd65 to
23eadc8
Compare
franciscojavierarceo
left a comment
There was a problem hiding this comment.
can you update the small nits I have?
otherwise lgtm!
franciscojavierarceo
left a comment
There was a problem hiding this comment.
- You'll need to rebase my latest changes to
RagQueryConfig - Can you add the new parameter in
RagQueryConfigto the docstring? - Can you also regenerate the API documentation using
uv run --with ".[dev]" ./docs/openapi_generator/run_openapi_generator.sh?
That plus some nits and then I think this lgtm!
0cc6501 to
c2dd17a
Compare
franciscojavierarceo
left a comment
There was a problem hiding this comment.
thank you for this, lgtm!
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
f74de61 to
25b8f96
Compare
hardikjshah
left a comment
There was a problem hiding this comment.
small nits but not blocking, lets land this.
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
25b8f96 to
6a2a083
Compare
What does this PR do?
This PR introduces support for keyword based FTS5 search with BM25 relevance scoring. It makes changes to the existing EmbeddingIndex base class in order to support a search_mode and query_str parameter, that can be used for keyword based search implementations.
Test Plan
run
Output:
For reference, with the implementation, the fts table looks like below:
Query results:
Result 1: Sentence 5 from document 0
Result 2: Sentence 5 from document 1
Result 3: Sentence 5 from document 2