-
Notifications
You must be signed in to change notification settings - Fork 728
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
[FEATURE] Metadata UUID Support #1211
Conversation
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/filter/comparison/IsEqualTo.java
Show resolved
Hide resolved
...ain4j-core/src/main/java/dev/langchain4j/store/embedding/filter/comparison/IsNotEqualTo.java
Show resolved
Hide resolved
.../main/java/dev/langchain4j/store/embedding/inmemory/GsonInMemoryEmbeddingStoreJsonCodec.java
Outdated
Show resolved
Hide resolved
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.
@humcqc thank you! Do tests pass for all EmbeddingStore
s that support storing metadata?
...chain4j-core/src/main/java/dev/langchain4j/store/embedding/filter/MetadataFilterBuilder.java
Show resolved
Hide resolved
I've just tested the ones having the filter test : |
@langchain4j @humcqc Any chance we can get this approved and merged? With PgVectorEmbeddingStore I'd like to add a metadata column of type UUID which points to the parent table PK of the embedding table which has a type UUID. |
@langchain4j It should be ok to merge. @sjivan by that time you can still use deprecated method :
|
@humcqc thanks for getting the PR ready to merge. The workaround of the deprecated method does not work with PgVectorEmbeddingStore metadata because if the field in question is declared as "foo uuid", when the retreiver reads the column "foo" it introspects the type via JDBC and when it sees its of type UUID, it fails with an unssupported type exception. Hopefully this is no longer the case after change. |
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.
@humcqc thanks a lot!
Please update Weaviate ITs, "uuid" field needs to be added to metadataKeys
, otherwise it fails
...n4j-core/src/main/java/dev/langchain4j/store/embedding/filter/comparison/UUIDComparator.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
...hain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithFilteringIT.java
Outdated
Show resolved
Hide resolved
@langchain4j Done. |
@langchain4j I've seen that some modules are in java 17 |
@humcqc yes, if there is a good reason for that |
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.
@humcqc good job, thank you!
[FEATURE] Metadata UUID Support #1164