-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
integrations: LanceDB update #13416
integrations: LanceDB update #13416
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@ravi03071991 review pls |
...ns/vector_stores/llama-index-vector-stores-lancedb/llama_index/vector_stores/lancedb/base.py
Outdated
Show resolved
Hide resolved
) | ||
|
||
if table: | ||
assert isinstance(table, lancedb.db.LanceTable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a complimentary error msg here
if query_type == "vector": | ||
_query = query.query_embedding | ||
else: | ||
self._table.create_fts_index(self.text_key, replace=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create index every time query is called, let's keep track of index if that's created in an instance variable and not recreate index every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is also done because if the user adds more data, a full reindex is needed from our(lance) end, creating it every time is also not the best way but we would have to handle this in the future.
Resolving it as per your comment as of now thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm..actually just set the flag to False every time table.add is called? just use fts_index_created or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I actually just did that haha, basically setting the instance variable back to default and on query time we are checking the variable status
self._table.create_fts_index(self.text_key, replace=True) | ||
if query_type == "hybrid": | ||
_query = (query.query_embedding, query.query_str) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif query_type="fts" and else: raise ValueError("invalid query_type")
|
||
def __init__( | ||
self, | ||
uri: Optional[str], | ||
table_name: str = "vectors", | ||
connection: Optional[Any] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will changing the order of args affect backwards compat? If so, let's try to add the new args towards the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was supposed to be a minor breaking change to synch with other integrations, thanks ill change it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk sounds good
Feature update :
from_table
- creates index from existing tableUpdated demo notebook ( tested locally ) and tested via make commands.