-
Notifications
You must be signed in to change notification settings - Fork 222
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
feat: added voyager in backend #1846
base: main
Are you sure you want to change the base?
Conversation
Hello @darshi1337 , We need for you to sign the DCO agreement by signing off your commits |
Thanks for the contribution, since it seems to be a work in progress (no tests, print statements, etc...) I will put it as a Draft. |
Ohh forgot to ask something |
Signed-off-by: darshi1337 <priyadarshi.annupam.mec22@itbhu.ac.in>
f31713a
to
679b8fc
Compare
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.
I do not see any obvious missing thing. Just to clarify what is possible and not possible with the backend.
if columns_str: | ||
columns_str = ', ' + columns_str | ||
|
||
query = f'CREATE TABLE IF NOT EXISTS docs (doc_id INTEGER PRIMARY KEY, data BLOB{columns_str})' |
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.
does this make that column data will not be filterable at all?
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.
The SQL query provided creates a table named docs
with a data
column defined as BLOB
(Binary Large Object). The BLOB
data type is generally used for storing binary data, and it does not inherently support filtering or indexing.
If the intention is to make the data
column filterable, I might want to consider using a different data type based on the nature of the data being stored. For example, if the data
column contains text-based information, changing the data type to TEXT
could be more appropriate.
Here's an example modification to the query:
if columns_str:
columns_str = ', ' + columns_str
query = f'CREATE TABLE IF NOT EXISTS docs (doc_id INTEGER PRIMARY KEY, data TEXT{columns_str})'
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 is what is done in HNSWDocumentIndex I believe
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 like something similar
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.
it would be good indeed to have it
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1846 +/- ##
==========================================
- Coverage 85.06% 84.01% -1.06%
==========================================
Files 136 137 +1
Lines 9260 9376 +116
==========================================
Hits 7877 7877
- Misses 1383 1499 +116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if columns_str: | ||
columns_str = ', ' + columns_str | ||
|
||
query = f'CREATE TABLE IF NOT EXISTS docs (doc_id INTEGER PRIMARY KEY, data BLOB{columns_str})' |
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 is what is done in HNSWDocumentIndex I believe
Hello @darshi1337 , is there any progress to be expected here? |
See issue #1824.