-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Allow multiple metadata keys on RedisVectorStoreFilterType #5015 #5028
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thanks for the PR! I think this could be nice but can you please look into making it backwards compatible? |
Hi @jacoblee93, Thank you for your time looking into this. There are two 'incompatibilities' in my original PR:
Originally: metadata = { a:"A", b: "B", c:"C" };
redis: JSON.stringnify(metadata); Now: metadata = { a:"A", b: "B", c:"C" };
metadataSchema = {
a: SchemaFieldTypes.TEXT,
b: SchemaFieldTypes.TEXT,
c: SchemaFieldTypes.TEXT,
};
redis: { a:"A", b: "B", c:"C" }; Suggestion: metadata = { a:"A", b: "B", c:"C" };
metadataSchema = {
a: SchemaFieldTypes.TEXT,
b: SchemaFieldTypes.TEXT,
c: SchemaFieldTypes.TEXT,
metadataKey: SchemaFieldTypes.TEXT,
};
redis: { metadataKey: JSON.stringnify(metadata), a:"A", b: "B", c:"C" };
Originally: filter = [ "A", "B", "C"];
filterString = '@metadataKey{A|B|C}' Suggestion: filter = [ "@a:{A}", "b:{B}", "@c:{C}"];
filterString = '@a:{A} | b:{B} | @c:{C}' Note that the old filter array would result in a filter string that does not make sense anymore: filter = [ "a", "b", "c"];
filterString = 'a|b|c' This is why I made it with breaking changes... At least the developer would know that things are different now and would have to change the code to get the correct behavior. Although we can keep things backwards compatible in the interfaces, we would still have a very different behavior at runtime. Please, let me know your thoughs and how can we make it work. Thank you. |
Also, I have looked into other VectorStores implementations, such as PgVector It is also storing the metadata as a JSON object in a single column It would produce similar limitations, as we would not be able to filter by metadata fields in a reasonable way... |
@mauriciocirelli this is a nice PR, with the addition of the RediSearch schema definition you get the flexibility of the full power of the search engine, but that means (as you pointed out above #5028 (comment) ) that For the backwards compatibility, off the top of my head, if the 'metadataKey' is present use that as the backwards compatibility trigger and do as it was done before. If the |
Hi @bsbodden Regarding the backwards compatibility, I think that's easy and reasonable. In my opinion, it is not reasonable that Langchain should do such things for any kind of integration. There are helper libs for that and the developer should know how to build a Redis query (or use redis-om for example) as much as they have to know how to build Postgres queries (or use external ORM libs) or any other 3rd party VectorStore queries they want to integrate to langchain. That's just my opinion. If you are all okay with that, I can update the PR with these changes. |
Fixes #5015