-
-
Notifications
You must be signed in to change notification settings - Fork 124
[Store] Allow id to be int|string|Uuid for VectorDocument and TextDocument
#867
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
base: main
Are you sure you want to change the base?
Conversation
|
that should also be changed in |
|
I think this is it |
chr-hertel
left a comment
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.
One issue we have now is for users not using the Uid component but receiving Uuid instances even if they used string before.
I think the best solution here is to just drop the dependency to the component. no feature of the component itself - besides the type-declaration - actually needs a Uuid and it was a wrong assumption of mine in the beginning based on starting with Pinecone as store 🙈
|
Good thinking. I'd need only to remove the Uuid auto-casting from the constructor, right? |
|
I removed the Uuid auto-casting and only left the type declaration |
id to be int|string|Uuid for VectorDocument and TextDocument
|
Can we simplify something in the |
wouldn't know what, don't think so. but i think a valid follow up would be to kill UUID dependency in store component |
chr-hertel
left a comment
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.
Can you please rebase this once and have another look at MongoDb, Weaviate, Typesense, Milvus, MariaDb, and Meilisearch?
I think your changes in bridge implementations for removing the Uuid::fromString(...) make sense, but is not done everywhere.
Thanks already @MolloKhan!
I gave this a second thought, and I think @OskarStark idea is the way to go (#857 (comment))
If the ID coming from the store is in Uuid format the user will likely cast it themself
If you are ok with my changes, I'll proceed to update all the places where
VectorDocumentsare instantiated