feat: Add retrieve_online_documents_v2 support to Qdrant online store#6484
feat: Add retrieve_online_documents_v2 support to Qdrant online store#6484patelchaitany wants to merge 2 commits into
Conversation
| response = client.query_points( | ||
| collection_name=collection_name, | ||
| prefetch=prefetches, | ||
| query=models.FusionQuery(fusion=models.Fusion.RRF), | ||
| limit=top_k, | ||
| with_payload=True, | ||
| ) | ||
| points = response.points | ||
| elif embedding is not None: | ||
| response = client.query_points( | ||
| collection_name=collection_name, | ||
| query=embedding, | ||
| query_filter=vector_feature_filter, | ||
| limit=top_k, | ||
| with_payload=True, | ||
| using=dense_using, | ||
| ) | ||
| points = response.points | ||
| else: | ||
| assert query_string is not None | ||
| response = client.query_points( | ||
| collection_name=collection_name, | ||
| query=models.Document( | ||
| text=query_string, | ||
| model=online_store_config.sparse_embedding_model, | ||
| ), | ||
| using=online_store_config.sparse_vector_name, | ||
| limit=top_k, | ||
| with_payload=True, | ||
| ) | ||
| points = response.points | ||
|
|
||
| for point in points: | ||
| payload = point.payload or {} | ||
| entity_key_bin = _entity_key_bytes_from_payload(payload.get("entity_key")) | ||
| if entity_key_bin is None: | ||
| continue | ||
| if entity_key_bin not in hit_order: | ||
| hit_order.append(entity_key_bin) | ||
| hit_payload_by_entity[entity_key_bin] = payload | ||
| if point.score is not None: | ||
| score = float(point.score) | ||
| if embedding is not None: | ||
| dense_score_by_entity[entity_key_bin] = score | ||
| if query_string is not None: | ||
| sparse_score_by_entity[entity_key_bin] = score |
There was a problem hiding this comment.
🔴 Hybrid RRF fusion returns fewer unique entities than top_k and overwrites best scores
In hybrid search mode (both embedding and query_string provided), Qdrant's RRF fusion operates on points, not entities. Because Feast stores one Qdrant point per feature (embedding point and text_field point have different point IDs), the same entity can appear twice in the fused results — once from the dense prefetch (the "embedding" point) and once from the sparse prefetch (the "text_field" point). Since limit=top_k constrains the total number of points returned, not unique entities, the effective number of unique entities can be as low as ceil(top_k / 2) in the worst case.
Additionally, the score-tracking loop unconditionally overwrites scores when the same entity reappears (line 610–612). Results are ordered by descending score, so later duplicate points have lower scores, meaning the entity ends up with a degraded score.
Score overwrite and dedup issue
For entity A appearing twice in fusion results:
- First point (embedding, score=0.9): added to
hit_order, score 0.9 stored in bothdense_score_by_entityandsparse_score_by_entity - Second point (text_field, score=0.7): NOT added to
hit_order(dedup check passes), but score 0.7 overwrites 0.9 in both dicts
Result: entity A gets score 0.7 instead of 0.9.
The fix should (a) request a larger limit (e.g., top_k * 2) from the fusion query, (b) keep the first (best) score per entity rather than overwriting, and (c) truncate hit_order to top_k unique entities.
Prompt for agents
In retrieve_online_documents_v2, the hybrid search path at line 567-612 has two related issues:
1. The fusion query uses limit=top_k, but because Feast stores one Qdrant point per feature (embedding point and text_field point have different IDs), the same entity can consume two slots. This means fewer unique entities are returned than the user requested. Fix: increase the limit in the fusion query_points call to top_k * 2 (or more), and then truncate hit_order to top_k after deduplication.
2. In the score-processing loop (lines 599-612), when the same entity appears multiple times, the score dicts (dense_score_by_entity and sparse_score_by_entity) are unconditionally overwritten. Since results are ordered by descending score, later appearances have lower scores, degrading the entity's score. Fix: use setdefault or a conditional to only keep the first (highest) score per entity, e.g.:
dense_score_by_entity.setdefault(entity_key_bin, score)
sparse_score_by_entity.setdefault(entity_key_bin, score)
Also consider capping hit_order after the loop: hit_order = hit_order[:top_k]
Files: sdk/python/feast/infra/online_stores/qdrant_online_store/qdrant.py, function retrieve_online_documents_v2
Was this helpful? React with 👍 or 👎 to provide feedback.
| client.set_sparse_model(config.sparse_embedding_model) | ||
| sparse_vectors: List[models.SparseVector] = [] | ||
| for embedding in client._sparse_embed_documents(texts): |
There was a problem hiding this comment.
🚩 _encode_sparse_texts uses private API _sparse_embed_documents
_encode_sparse_texts calls client._sparse_embed_documents(texts) (line 155) — a private/internal method of the Qdrant client (note the leading underscore). While this appears to be the only way to access client-side sparse embedding, this API is not part of the public contract and could break with future qdrant-client upgrades without notice. Additionally, client.set_sparse_model(config.sparse_embedding_model) is called on every invocation rather than once during client initialization. This is a maintainability concern rather than a current bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
ad6ef63 to
6b06175
Compare
There was a problem hiding this comment.
@patelchaitany I think alpha-vector-database.md matrix table is inconsistent with the note text.
| {% endhint %} | ||
|
|
||
| **Note**: Milvus and SQLite implement the v2 `retrieve_online_documents_v2` method in the SDK. This will be the longer-term solution so that Data Scientists can easily enable vector similarity search by just flipping a flag. | ||
| **Note**: Milvus, SQLite, PostgreSQL, Elasticsearch, MongoDB, and Qdrant implement the v2 `retrieve_online_documents_v2` method in the SDK. This will be the longer-term solution so that Data Scientists can easily enable vector similarity search by just flipping a flag. |
There was a problem hiding this comment.
The note at line 33 was updated to include Qdrant, but the table at line 17 still shows [] for V2 Support. Need to update the table row to [x] for V2 Support (and Online Read).
There was a problem hiding this comment.
Done, Thanks for Pointing out
6b06175 to
8c0d332
Compare
| sparse_vectors: List[models.SparseVector] = [] | ||
| for embedding in client._sparse_embed_documents(texts): |
There was a problem hiding this comment.
@patelchaitany _encode_sparse_texts has a likely bug and uses deprecated internals
client._sparse_embed_documents(texts) at line 155 is called without passing embedding_model_name, so it defaults to "BAAI/bge-small-en" (a dense model). The official usage in qdrant-client's own add() method explicitly passes self.sparse_embedding_model_name. This means the sparse encoding at write time would attempt to use a dense model name to initialize a sparse model, which will either error or produce wrong results.
Additionally, _sparse_embed_documents, _FASTEMBED_INSTALLED, and the surrounding fastembed APIs were deprecated in qdrant-client. The modern approach is to use models.Document objects and let the client handle embedding internally.
There was a problem hiding this comment.
Suggested fix: if you must use this private API, pass the model name explicitly:
client._sparse_embed_documents(texts, embedding_model_name=config.sparse_embedding_model)
Or better yet, consider using the non-deprecated models.Document approach for sparse vector creation (as the retrieval path already does at line 559).
0f17e7a to
10a982c
Compare
10a982c to
71fed4e
Compare
There was a problem hiding this comment.
Thanks for addressing the earlier feedback, @patelchaitany. The _encode_sparse_texts → models.Document rewrite is a big improvement, the alpha-vector-database.md table is now consistent, _parse_timestamp handles both ISO formats, and the v1 path properly decodes base64 entity keys. All 14 unit tests pass locally.
What's good
models.Documentfor sparse vectors — The write path now uses the public Qdrant API (models.Document) instead of the private_sparse_embed_documents, consistent with the retrieval path. Much more maintainable.- Robust timestamp parsing —
_parse_timestampnow handles both%Y-%m-%dT%H:%M:%S.%fand%Y-%m-%dT%H:%M:%Swith a clear error for unsupported formats. - v1 fix —
retrieve_online_documentsnow uses_entity_key_bytes_from_payloadand_parse_timestamp, fixing silent bugs with raw entity key strings and brittle timestamp parsing. - New test coverage —
test_online_write_batch_uses_document_for_sparse_vectors,test_retrieve_online_documents_v1_decodes_base64_entity_key, andtest_parse_timestamp_*add solid coverage for the new code paths.
Remaining issues (see inline comments)
- RRF fused scores mislabeled as dense/sparse — In hybrid mode, the scores from RRF fusion are not the original cosine/BM25 scores but fused rank scores. Assigning them to
distanceandtext_rankis semantically misleading. distance_metricparam validated but never applied — The parameter has no effect on the Qdrant query; the collection uses the metric configured at creation time.- Missing docstring on
retrieve_online_documents_v2— Other online store implementations include one. - No integration test for hybrid search — Only dense and error-case integration tests exist.
| if feature_name == vector_field_name: | ||
| dense_score_by_entity.setdefault(entity_key_bin, score) | ||
| elif feature_name == text_feature_name: | ||
| sparse_score_by_entity.setdefault(entity_key_bin, score) |
There was a problem hiding this comment.
RRF fused scores are mislabeled as dense/sparse scores
In hybrid mode, Qdrant's RRF fusion query returns a single fused rank score per point — not the original dense cosine similarity or sparse BM25 relevance. This loop assigns the fused score to dense_score_by_entity or sparse_score_by_entity based on feature_name, but the score from an RRF-fused result set doesn't represent the raw similarity for that specific vector type.
Downstream, distance and text_rank are surfaced to users who'll naturally interpret them as cosine similarity and keyword relevance. This could lead to incorrect ranking decisions in application code.
Options:
- Return only a single
distancefield in hybrid mode representing the fused RRF score, and document it clearly - Run dense and sparse queries independently (like qdrant-client's own
query()method does — seeqdrant_fastembed.pyL680-703), fuse manually withreciprocal_rank_fusion(), and keep the individual scores
For non-hybrid paths (dense-only or text-only), the current behavior is correct.
|
|
||
| metric = (distance_metric or online_store_config.similarity or "cosine").lower() | ||
| if metric not in DISTANCE_MAPPING: | ||
| raise ValueError(f"Unsupported distance metric: {metric}") |
There was a problem hiding this comment.
distance_metric is validated but never actually used in the query
This line validates the metric against DISTANCE_MAPPING, but it's never passed to client.query_points(). Qdrant uses whatever distance metric was configured when the collection was created (in create_collection). If a user passes distance_metric="l2" but the collection uses cosine, they'll silently get cosine results.
Suggestions:
- Log a warning if the requested metric differs from the configured
online_store_config.similarity - Or document in the docstring that this parameter is for validation only and the collection's configured metric is always used
| Optional[EntityKeyProto], | ||
| Optional[Dict[str, ValueProto]], | ||
| ] | ||
| ]: |
There was a problem hiding this comment.
Missing docstring — All other online store implementations (Milvus, MongoDB, PostgreSQL, SQLite) include a docstring on retrieve_online_documents_v2 documenting the parameters, return format, and any backend-specific behavior (e.g., the one-point-per-feature join, hybrid search semantics). Please add one here for consistency.
| collection_name=collection_name, | ||
| prefetch=prefetches, | ||
| query=models.FusionQuery(fusion=models.Fusion.RRF), | ||
| limit=top_k * 2, |
There was a problem hiding this comment.
Nit: top_k * 2 works when only embedding + text_field points exist per entity, but if a feature view has more features stored as points, the same entity could consume more than 2 slots in the fused results. A comment documenting the assumption (2 point types participate in fusion: the embedding point and the text_field point) would help future maintainers.
| ) | ||
| finally: | ||
| shutil.rmtree(repo_dir, ignore_errors=True) | ||
|
|
There was a problem hiding this comment.
Missing hybrid search integration test — The integration tests cover dense search and the error case for text_search_enabled, but there's no end-to-end test exercising hybrid search (embedding + query_string together with text_search_enabled: true). This is the most complex code path and the one most likely to break. Even if it requires fastembed as a dependency, a guarded test (with pytest.importorskip("fastembed")) would add strong confidence.
- Add docstring to retrieve_online_documents_v2 documenting all three search modes, parameters, and return format - Fix RRF fused scores: in hybrid mode, return a single fused RRF rank score as `distance` instead of mislabeling it as dense/sparse scores; `text_rank` is no longer populated in hybrid mode since individual sub-scores are unavailable after fusion - Log a warning when distance_metric differs from the collection's configured similarity, since the parameter has no effect on the query - Expand comment explaining the top_k * 2 assumption (exactly 2 point types participate in fusion) - Add hybrid search integration test guarded by fastembed availability - Update unit test assertions to match new hybrid score semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
- Add docstring to retrieve_online_documents_v2 documenting all three search modes, parameters, and return format - Fix RRF fused scores: in hybrid mode, return a single fused RRF rank score as `distance` instead of mislabeling it as dense/sparse scores; `text_rank` is no longer populated in hybrid mode since individual sub-scores are unavailable after fusion - Log a warning when distance_metric differs from the collection's configured similarity, since the parameter has no effect on the query - Expand comment explaining the top_k * 2 assumption (exactly 2 point types participate in fusion) - Add hybrid search integration test guarded by fastembed availability - Update unit test assertions to match new hybrid score semantics Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
b09df8e to
b2d3020
Compare
Signed-off-by: Chaitany patel <patelchaitany93@gmail.com>
- Add docstring to retrieve_online_documents_v2 documenting all three search modes, parameters, and return format - Fix RRF fused scores: in hybrid mode, return a single fused RRF rank score as `distance` instead of mislabeling it as dense/sparse scores; `text_rank` is no longer populated in hybrid mode since individual sub-scores are unavailable after fusion - Log a warning when distance_metric differs from the collection's configured similarity, since the parameter has no effect on the query - Expand comment explaining the top_k * 2 assumption (exactly 2 point types participate in fusion) - Add hybrid search integration test guarded by fastembed availability - Update unit test assertions to match new hybrid score semantics Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
b2d3020 to
8367c78
Compare
What this PR does / why we need it:
This PR adds retrieve_online_documents_v2 for the Qdrant online store so vector search returns all requested features per document, not just the embedding hit. It implements dense embedding search (with a join across Qdrant’s one-point-per-feature layout), plus optional hybrid search when text_search_enabled: true. v1 is unchanged; hybrid is off by default so existing setups don’t need migration. Files touched: qdrant.py, qdrant.md, alpha-vector-database.md, test_qdrant_online_retrieval.py, test_qdrant_retrieve_online_documents_v2.py, and manual_qdrant_v2.py.
Which issue(s) this PR fixes:
#6445
Checks
git commit -s)Testing Strategy
Misc