Allow query_points to accept Sequence[float]#1150
Allow query_points to accept Sequence[float]#1150veeceey wants to merge 4 commits intoqdrant:masterfrom
Conversation
- Update type hints to accept Sequence[float] instead of list[float] - Allows tuples, lists, and other sequence types as query vectors - Matches the deprecated search() method API for better compatibility - Update both sync (QdrantClient) and async (AsyncQdrantClient) clients - Update _resolve_query to handle tuple conversion - Add comprehensive tests for tuple, list, and numpy array inputs Fixes qdrant#1074
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughType annotations for numeric query inputs were widened from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All checks passing, ready for maintainer review |
|
Thanks for the review feedback, CodeRabbit! Here's my plan to address the suggestions:
Will push updates shortly. |
…rmatting changes - Add async test for query_points_groups with tuple input - Add generic Sequence fallback in _resolve_query for both sync and async - Remove out-of-scope formatting changes from async_client_base.py, keeping only the Sequence[float] type hint updates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed the updates addressing review feedback:
All 4 tests pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_query_points_sequence.py (1)
118-192: Consider adding a test for the genericSequencefallback.The PR adds a generic
Sequencefallback in_resolve_query(converting anySequence[float]to list), but no test exercises that path. A test using e.g.collections.dequewould confirm the fallback works end-to-end:from collections import deque def test_query_points_with_deque(): """Test that query_points accepts deque (generic Sequence[float])""" client = QdrantClient(":memory:") collection_name = "test_query_deque" client.create_collection( collection_name=collection_name, vectors_config=models.VectorParams(size=10, distance=models.Distance.COSINE), ) client.upsert( collection_name=collection_name, points=[ models.PointStruct(id=i, vector=[float(i)] * 10, payload={"value": i}) for i in range(5) ], ) query_deque = deque([1.0] * 10) result = client.query_points( collection_name=collection_name, query=query_deque, limit=5, ) assert len(result.points) == 5 client.delete_collection(collection_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_query_points_sequence.py` around lines 118 - 192, Add a test that exercises the generic Sequence fallback in _resolve_query by passing a collections.deque as the query vector to query_points (and its async counterpart); create a collection with VectorParams(size=10), upsert a few PointStructs, call client.query_points (and await for AsyncQdrantClient) with query=deque([1.0]*10) and assert the returned points length matches the limit, then delete the collection—this will verify the Sequence-to-list conversion path used by _resolve_query for both QdrantClient.query_points and AsyncQdrantClient.query_points.qdrant_client/qdrant_fastembed.py (1)
854-857:isinstance(query, Sequence)matchesbytes,bytearray, andrange— consider adding defensive checks.
stris correctly caught as aPointIdfirst (line 842 checksisinstance(query, get_args(types.PointId)), which includesstr). However, the fallback at line 854 will accept anySequence, includingbytes,bytearray, andrange, which would be converted to lists without validation. This relies entirely on upstream type hints for safety.Optional: Add an element-type check before conversion:
Suggested refinement
if isinstance(query, Sequence): - # Generic Sequence fallback - convert any Sequence[float] to list - return models.NearestQuery(nearest=list(query)) + # Generic Sequence fallback - convert any Sequence of numbers to list. + # str is already handled as PointId above; guard against bytes/bytearray/range. + if len(query) > 0 and not isinstance(query[0], (int, float)): + raise ValueError(f"Unsupported query type: {type(query)}") + return models.NearestQuery(nearest=list(query))Note:
async_qdrant_fastembed.py(line 792) has identical code that should be updated in parallel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/qdrant_fastembed.py` around lines 854 - 857, The Sequence fallback in the nearest-query conversion accepts bytes/bytearray/range and blindly converts to list; update the branch that returns models.NearestQuery(nearest=list(query)) to first guard against unwanted Sequence types (e.g., if isinstance(query, (bytes, bytearray, range)): raise TypeError(...) or skip) and/or validate element types (e.g., ensure all items are int/float or castable to float via a quick all(isinstance(x, (int, float)) for x in query) check) before converting; apply the same defensive check/change to the identical code in async_qdrant_fastembed.py so models.NearestQuery is only created from valid numeric sequences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@qdrant_client/qdrant_fastembed.py`:
- Around line 854-857: The Sequence fallback in the nearest-query conversion
accepts bytes/bytearray/range and blindly converts to list; update the branch
that returns models.NearestQuery(nearest=list(query)) to first guard against
unwanted Sequence types (e.g., if isinstance(query, (bytes, bytearray, range)):
raise TypeError(...) or skip) and/or validate element types (e.g., ensure all
items are int/float or castable to float via a quick all(isinstance(x, (int,
float)) for x in query) check) before converting; apply the same defensive
check/change to the identical code in async_qdrant_fastembed.py so
models.NearestQuery is only created from valid numeric sequences.
In `@tests/test_query_points_sequence.py`:
- Around line 118-192: Add a test that exercises the generic Sequence fallback
in _resolve_query by passing a collections.deque as the query vector to
query_points (and its async counterpart); create a collection with
VectorParams(size=10), upsert a few PointStructs, call client.query_points (and
await for AsyncQdrantClient) with query=deque([1.0]*10) and assert the returned
points length matches the limit, then delete the collection—this will verify the
Sequence-to-list conversion path used by _resolve_query for both
QdrantClient.query_points and AsyncQdrantClient.query_points.
- Reject bytes, bytearray, and range before the generic Sequence fallback in both sync and async _resolve_query methods - Add sync and async tests using collections.deque to exercise the generic Sequence[float] fallback path
|
Addressed the latest review nitpicks:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qdrant_client/qdrant_fastembed.py (1)
854-861: UseTypeErrorfor unsupportedquerytype arguments.These branches reject based on argument type, not invalid values. Current
ValueErrorusage diverges from Python semantics whereTypeErrorshould signal type contract violations. This makes it harder for callers to distinguish between type errors and value validation errors.Also update the docstring from "Raises: ValueError" to "Raises: TypeError" to match the implementation. Keep the autogenerated async copy in sync as well.
♻️ Proposed fix
- if isinstance(query, (bytes, bytearray, range)): - raise ValueError(f"Unsupported query type: {type(query)}") + if isinstance(query, (bytes, bytearray, range)): + raise TypeError(f"Unsupported query type: {type(query)}") @@ - raise ValueError(f"Unsupported query type: {type(query)}") + raise TypeError(f"Unsupported query type: {type(query)}")Update docstring in both
_resolve_querymethods:- ValueError: if query is not of supported type + TypeError: if query is not of supported type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/qdrant_fastembed.py` around lines 854 - 861, Replace ValueError with TypeError in the _resolve_query implementation(s): change the two raise ValueError(...) lines inside _resolve_query to raise TypeError(...) because these branches reject by argument type, and update the method docstring "Raises: ValueError" to "Raises: TypeError" accordingly; do the same for the autogenerated async copy of _resolve_query so both synchronous and async variants use TypeError in raises and docstrings and remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_query_points_sequence.py`:
- Around line 195-265: Extend the existing tests
(test_query_points_with_deque_sync and test_query_points_with_deque_async) to
also assert the new reject path by passing invalid Sequence types (bytes,
bytearray, range) into client.query_points and verifying they raise the expected
exception (TypeError/ValueError) for both QdrantClient and AsyncQdrantClient;
locate the call sites to client.query_points in those functions and add
assertions that calling query_points with query=b"bytes..." ,
query=bytearray(...), and query=range(...) raises the same error the library
uses for unsupported sequence types so the guard branches are covered.
---
Nitpick comments:
In `@qdrant_client/qdrant_fastembed.py`:
- Around line 854-861: Replace ValueError with TypeError in the _resolve_query
implementation(s): change the two raise ValueError(...) lines inside
_resolve_query to raise TypeError(...) because these branches reject by argument
type, and update the method docstring "Raises: ValueError" to "Raises:
TypeError" accordingly; do the same for the autogenerated async copy of
_resolve_query so both synchronous and async variants use TypeError in raises
and docstrings and remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbe8786f-4858-459b-88cc-d94767d4377c
📒 Files selected for processing (3)
qdrant_client/async_qdrant_fastembed.pyqdrant_client/qdrant_fastembed.pytests/test_query_points_sequence.py
|
hey, just checking in on this one - anything else needed? |
1 participant
Summary
query_pointsandquery_points_groupsto acceptSequence[float]instead oflist[float]search()method API for better compatibilityQdrantClient) and async (AsyncQdrantClient) clientsMotivation
The deprecated
search()method acceptedSequence[float]for query vectors, butquery_points()only acceptedlist[float]. This created an API inconsistency when migrating fromsearch()toquery_points().With this change, users can now pass tuples and other sequence types:
Changes
client_base.pyandasync_client_base.py_resolve_querymethod inqdrant_fastembed.pyandasync_qdrant_fastembed.pyto handle tuple conversionTest Plan
Fixes