Conversation
📝 WalkthroughWalkthroughThis change updates the strict mode verification logic for queries in the collection module. The Implementations of Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/collection/src/operations/verification/query.rs (2)
91-94: RedundantBox::pinaround already-pinned future
prefetch.check_strict_mode(...)returns aPin<Box<…>>(per the trait).
Wrapping it again withBox::pinallocates a second box and needlessly complicates the type:- Box::pin(prefetch.check_strict_mode(collection, strict_mode_config)).await?; + prefetch.check_strict_mode(collection, strict_mode_config).await?;Same issue appears in the
CollectionPrefetchimplementation.
72-77: Error message could expose little context – include the vector nameThe current message:
Fullscan forbidden on '{using}'is helpful but omits why (index disabled). Consider:
return Err(CollectionError::strict_mode( format!("Fullscan forbidden on '{using}' – vector indexing is disabled (m = 0 or payload_m = 0)"), "Enable vector indexing or use a prefetch query before rescoring", ));This makes troubleshooting easier, especially when multiple vectors are configured.
tests/openapi/test_strictmode.py (1)
1813-1864: Test is brittle – relies on exact error string & skips status-code checkThe assertions check the full, hard-coded error message. Any wording change will break the test although behaviour is still correct.
- assert not response.ok - assert "Fullscan forbidden on 'dense-multi'. Help: Enable vector indexing or use a prefetch query before rescoring" in response.json()['status']['error'] + assert response.status_code == 400 # or the exact code your API returns + assert "Fullscan forbidden" in response.json()['status']['error'] + assert "dense-multi" in response.json()['status']['error']Using substrings and explicit status codes makes the test less fragile while still validating the contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/collection/src/operations/verification/query.rs(5 hunks)tests/openapi/test_strictmode.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/openapi/test_strictmode.py (2)
tests/openapi/conftest.py (1)
collection_name(50-51)tests/openapi/helpers/helpers.py (1)
request_with_validation(41-116)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consensus-compose
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: test-consistency
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: integration-tests
- GitHub Check: storage-compat-test
- GitHub Check: lint
🔇 Additional comments (1)
lib/collection/src/operations/verification/query.rs (1)
48-61:Details
❌ Incorrect review comment
Full-scan guard ignores ID-based & multivector queries
check_fullscanshort-circuits unless the variant isQuery::Vector(_).
However, full scans are equally possible when the query is:
- a point-ID query that falls back to an un-indexed scan, or
- a multivector / hybrid variant.
The new test uses an ID (
"query": 2) and therefore relies on other logic to catch the violation. For defence-in-depth (and future-proofing), consider extending the match arm or adding a separate check for those variants.
We need to inspect the full
check_fullscanimplementation to see whichQueryvariants it handles. Let’s dump the entire function body:
🏁 Script executed:
#!/bin/bash rg -n "fn check_fullscan" -A200 lib/collection/src/operations/verification/query.rsLength of output: 6041
🏁 Script executed:
#!/bin/bash # Locate the Query enum definition to inspect its variants rg -n "enum Query" -A30 lib/collection/src/operations/verification/query.rsLength of output: 77
🏁 Script executed:
#!/bin/bash # Display the top of the file to locate the Query enum definition and its variants sed -n '1,200p' lib/collection/src/operations/verification/query.rsLength of output: 6642
🏁 Script executed:
#!/bin/bash # Locate and display the Query enum definition with its variants file_path=$(fd --full-path "collection_query.rs" lib/collection/src/operations/universal_query) echo "Using file: $file_path" sed -n '1,200p' "$file_path"Length of output: 7602
Ignore full-scan guard suggestion for ID-based or multivector queries
The
Query::Vector(_)arm already handles both pure vectors and ID-based lookups, sinceVectorInputInternalcan be anId. There is no separate “ID” or “hybrid” variant to match, and otherQueryvariants (e.g.Fusion,Formula,OrderBy,Sample) do not perform vector full-scans. No change required.Likely an incorrect or invalid review comment.
| strict_mode_config: &StrictModeConfig, | ||
| ) -> CollectionResult<()> { | ||
| // Check only applies on `unindexed_filtering_retrieve` | ||
| if strict_mode_config.unindexed_filtering_retrieve == Some(false) { |
There was a problem hiding this comment.
Nit but maybe we can early return here to reduce amount of indents.
There was a problem hiding this comment.
I prefer keeping it this to make the scope clearer when add new checks in the custom checker.
| // Check only applies on `unindexed_filtering_retrieve` | ||
| if strict_mode_config.unindexed_filtering_retrieve == Some(false) { | ||
| if let Query::Vector(_) = &self { | ||
| let config = collection.collection_config.read().await; |
There was a problem hiding this comment.
Maybe a bit too cautious but are you sure that this block call is not too slow for getting called in every request?
I don't know if there is a proper workaround though.
There was a problem hiding this comment.
In my experience the async lock on the collection configuration is not an issue.
| collection: &Collection, | ||
| strict_mode_config: &StrictModeConfig, | ||
| ) -> CollectionResult<()> { | ||
| // Check only applies on `unindexed_filtering_retrieve` |
There was a problem hiding this comment.
Shouldn't it be search_allow_exact instead?
There was a problem hiding this comment.
Good catch!
You are right, got confused with my ongoing work on group by filtering 🤦
| ) -> CollectionResult<()> { | ||
| // Check only applies on `unindexed_filtering_retrieve` | ||
| if strict_mode_config.unindexed_filtering_retrieve == Some(false) { | ||
| if let Query::Vector(_) = &self { |
There was a problem hiding this comment.
Let's use match here, explicitly handling each case
| .and_then(|param| param.hnsw_config.as_ref()); | ||
|
|
||
| let vector_hnsw_m = vector_hnsw_config.and_then(|hnsw| hnsw.m); | ||
| let vector_hnsw_payload_m = vector_hnsw_config.and_then(|hnsw| hnsw.payload_m); |
There was a problem hiding this comment.
Payload_m is only an excuse if there is a filter by tenant/principal
There was a problem hiding this comment.
Got it, removing it from the check and will tackle multitenancy in a different PR.
| // check for unindexed fields in formula | ||
| query.check_strict_mode(collection, strict_mode_config)? | ||
| query | ||
| .check_strict_mode(collection, strict_mode_config) |
There was a problem hiding this comment.
Function naming in this case becomes confusing.
check_fullscan is followed by check_strict_mode. Isn't check_fullscan part of strict mode?
There was a problem hiding this comment.
check_strict_mode on the Query does not have the knowledge whether there are prefetches.
This information is necessary to know if it is a rescoring use case.
For CollectionPrefetch it should always be checked.
But for CollectionQueryRequest only if there are no prefetches.
This is why the calls are not unified.
agourlay
force-pushed
the
strict-mode-fullscan
branch
from
8e63b77 to
6962b12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/collection/src/operations/verification/query.rs (1)
39-85: Well-structured implementation of fullscan prevention in strict mode.This new method correctly implements the strict mode check for fullscan queries when
search_allow_exactis set to false. The method:
- Only applies the check when appropriate (line 47)
- Uses pattern matching for different query types (line 48)
- Properly handles sparse vectors which are always indexed (lines 53-62)
- Correctly identifies disabled vector indexing via the HNSW
mparameter (lines 64-73)- Provides a helpful error message with a clear solution (lines 74-79)
The TODO comment on line 72 could be more descriptive about what the payload_m check will entail and its purpose in the multi-tenancy context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/collection/src/operations/verification/query.rs(5 hunks)tests/openapi/test_strictmode.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/openapi/test_strictmode.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/collection/src/operations/verification/query.rs (6)
lib/collection/src/operations/verification/discovery.rs (1)
check_strict_mode(30-42)lib/collection/src/operations/verification/search.rs (1)
check_strict_mode(53-65)lib/collection/src/operations/verification/mod.rs (1)
check_strict_mode(150-162)src/common/update.rs (1)
check_strict_mode(125-172)lib/collection/src/collection/collection_ops.rs (1)
strict_mode_config(277-283)lib/collection/src/operations/types.rs (1)
strict_mode(1120-1123)
🔇 Additional comments (4)
lib/collection/src/operations/verification/query.rs (4)
11-37: The conversion to async is correct and well-implemented.The method has been properly converted from synchronous to asynchronous. This change is essential for allowing the collection configuration to be read asynchronously later in the
check_fullscanmethod. The existing logic for checking unindexed expressions remains intact.
96-112: Correct implementation of strict mode checks for CollectionQueryRequest.The implementation properly:
- Awaits the async
check_strict_modefor prefetches- Only applies the fullscan check when not in a rescoring scenario (empty prefetch)
- Correctly calls both checks on the query object
This approach aligns with the PR objective of allowing rescoring use cases to proceed despite strict mode restrictions.
150-157: Appropriate strict mode checks for CollectionPrefetch.For prefetch queries, the implementation correctly:
- Always checks if the prefetch can perform a fullscan (since prefetches are the first query in a rescoring pipeline)
- Properly awaits the async checks
- Maintains the existing check for unindexed fields in the formula
191-200: Consistent implementation for CollectionQueryGroupsRequest.The implementation correctly:
- Only applies the fullscan check when not rescoring (empty prefetch)
- Properly awaits the async checks
- Maintains the check for unindexed fields in the formula
This implementation is consistent with the approach used for CollectionQueryRequest, which is good for code maintainability and understanding.
agourlay
force-pushed
the
strict-mode-fullscan
branch
from
6962b12 to
8e4f6ca
Compare
|
More review is necessary here 🙏 |
| .and_then(|param| param.hnsw_config.as_ref()); | ||
|
|
||
| let vector_hnsw_m = vector_hnsw_config.and_then(|hnsw| hnsw.m); | ||
| // TODO(strict-mode) check also payload_m if if there is a filter by tenant/principal |
There was a problem hiding this comment.
Just FYI: we can't do releases without this todo implemented. Otherwise it is too restrictive
3 participants
This PR implements fully the strict mode for
search_allow_exactto make sure no search query can be issued against a disabled HNSW index.The check takes into account the presence of prefetches to still allow the rescoring use case anyway.