[Store] Allow id to be int|string|Uuid for VectorDocument and TextDocument
#867
Conversation
MolloKhan
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
added
Feature
|
that should also be changed in |
|
I think this is it |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
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 |
OskarStark
changed the title
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
chr-hertel
left a comment
There was a problem hiding this comment.
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!
|
friendly ping @MolloKhan |
|
Sorry for the delay! I just got back home from holidays (it was such a long ride 😄) - I searched for all places where we cast strings into |
MolloKhan
force-pushed
the
vectordocument-support-types
branch
from
0b8cd10 to
59de316
Compare
MolloKhan
force-pushed
the
vectordocument-support-types
branch
from
c8bb9ae to
e9a9c7c
Compare
|
I rebased and fixed the conflicts, but I don't understand why the tests are failing (they do pass locally). It seems it is using an old version of the code base or something. |
|
Loos like the store bridges don't use the working branch version of the store component 🤔 see #1224 |
This PR was merged into the main branch. Discussion ---------- Test store bridge pipeline | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Docs? | no | Issues | | License | MIT might help with #867 not sure `@OskarStark` if there side effects, but in my understanding we need this? Commits ------- 913d44c Link vendors in bridge pipelines
|
I merged #1224, can you please rebase? Thanks |
4 participants
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