Add VectorStore.upsert for pre-computed embeddings - #7002
sobychacko wants to merge 3 commits into
Conversation
tzolov
left a comment
tzolov
left a comment
There was a problem hiding this comment.
Thanks @sobychacko,
I was wondering if we need to put some guards in cases where CONTENT_REF is empty or set on non upsert() execution path? Are those supported invariants? if not maybe a waring message would be useful.
Also Seems like Redis drops the CONTENT_REF? It returns only metadata declared in metadataFields at build time.
| /** | ||
| * VectorStore upsert operation. | ||
| */ | ||
| UPSERT("upsert"), |
There was a problem hiding this comment.
Please update the observability/index.adoc's (low cardinality keys and Labels tables) for the new Operation.UPSERT and the emitted db.operation.name=upsert.
| if (errResponse.isPresent()) { | ||
| String message = MessageFormat.format("Could not upsert document: {0}", errResponse.get()); | ||
| logger.error(message); | ||
| throw new RuntimeException(message); |
There was a problem hiding this comment.
Other vector stores seem to throw IllegalStateException here. We can be consistent throwing the same even though RuntimeException is still good.
|
|
||
| this.qdrantClient.upsertAsync(this.collectionName, points).get(); | ||
| } | ||
| catch (InterruptedException | ExecutionException e) { |
There was a problem hiding this comment.
Should we call Thread.currentThread().interrupt() when handling InterruptedException?
|
@tzolov Thanks for the review. I will address them. See below. Guards / invariants: you're right, I never documented the contract. CONTENT_REF on On a blank value: Would documenting the invariants plus a TCK case that round-trips a CONTENT_REF row be enough? There's no behavioral test for it today. Happy to add guards if you'd rather. Redis: The read only asks for the fields declared in
.metadataFields(MetadataField.tag(DocumentMetadata.CONTENT_REF.value()))I'll put that in the docs and the Redis IT so it runs in CI, plus a warning when a document carries undeclared metadata so it stops being silent. @ilayaperumalg Thanks for the review. I will address them. |
Fixes spring-projects#6989 User supplied embeddings write path: an EmbeddedDocument record pairing a Document with a caller-supplied vector, and a default VectorStore.upsert that throws so stores opt in. Wired into AbstractObservationVectorStore (UPSERT observation + doUpsert), with replace-by-id implementations for pgvector, Redis, Elasticsearch, and Qdrant. Adds a shared AbstractVectorStoreUpsertTests TCK, the DocumentMetadata.CONTENT_REF key for non-text content, and reference docs. See also spring-projects#3540 and spring-projects#3541 Signed-off-by: Soby Chacko <soby.chacko@broadcom.com>
- List upsert in the observability tables - Document the CONTENT_REF rules and add a TCK case that round-trips a reference row - Declare content_ref in the Redis test and warn on undeclared metadata, which Redis drops on read - Throw IllegalStateException from the Redis and Qdrant upsert paths - Restore the interrupt flag when a Qdrant upsert is interrupted Signed-off-by: Soby Chacko <soby.chacko@broadcom.com>
- Reject media documents, as add does, since every store requires text on the row - Write Redis metadata before the reserved fields so it cannot overwrite the vector - Check vector dimensions only when the expected width is known - Use ids the stores accept in the docs examples - Document what the caller owns when supplying vectors Signed-off-by: Soby Chacko <soby.chacko@broadcom.com>
1a0aa34 to
5a74983
Compare
3 participants
Fixes #6989
User supplied embeddings write path: an EmbeddedDocument record pairing a Document with a caller-supplied vector, and a default VectorStore.upsert that throws so stores opt in. Wired into AbstractObservationVectorStore (UPSERT observation + doUpsert), with replace-by-id implementations for pgvector, Redis, Elasticsearch, and Qdrant. Adds a shared AbstractVectorStoreUpsertTests TCK, the DocumentMetadata.CONTENT_REF key for non-text content, and reference docs.
See also #3540 and #3541