- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2k
 
feat(chroma): improve handling and testing of complex metadata values #3332
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
Conversation
- Convert non-primitive metadata values to JSON strings in ChromaApi.AddEmbeddingsRequest for compatibility - Add tests to verify metadata conversion and complex metadata handling in Chroma vector store integration - Ensure OpenAiChatModel always returns annotations as a list of maps in metadata - Add test dependency on spring-ai-advisors-vector-store for advisor-related tests Signed-off-by: Christian Tzolov <[email protected]>
| assertThat(processed.get("strVal")).isInstanceOf(String.class); | ||
| assertThat(processed.get("doubleVal")).isInstanceOf(Number.class).isEqualTo(3.14); | ||
| assertThat(processed.get("listVal")).isInstanceOf(String.class).isEqualTo("[1,2,3]"); | ||
| assertThat(processed.get("mapVal")).isInstanceOf(String.class); | 
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 we also verify that the value of mapVal is what we expect it to be?
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.
good point. It is done
…N content in tests Signed-off-by: Christian Tzolov <[email protected]>
| .uri("/api/v2/tenants/{tenant_name}/databases/{database_name}/collections/{collection_name}/upsert", | ||
| tenantName, databaseName, collectionId) | ||
| .headers(this::httpHeaders) | ||
| // .headers(this::httpHeaders) | 
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.
Why is this commented out?
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 a mistake. Thanks for catching it
Signed-off-by: Christian Tzolov <[email protected]>
| 
           Squashed, rebased and merged as 7466cb9  | 
    
| 
           Also, back-ported into 1.0.x via aba837a  | 
    
BACK-PORT to 1.0.1 as well!