Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
a3f4338 to
e3c1eb8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e3c1eb8 to
6b9173e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
quick question! do these changes require cc: @coszio |
Yes. Please feel free to create a new test file there. |
7429017 to
c40273e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
timvisee
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I must say, I'm quite impressed.
Right now we return an array of shard keys (either being a string or a number). I wonder if we ever want to extend this to also report the number of shards that belong to a key. With the current design that is not (easily) possible. At the same time I do like the simplicity it currently has.
Of course, this information is already indirectly available through GET /collections/something/cluster.
Let me think about it for a bit.
src/tonic/api/collections_api.rs
Outdated
| let shard_keys: CollectionShardKeys = do_get_collection_shard_keys( | ||
| self.dispatcher.toc(&access, &pass), | ||
| access, | ||
| request.into_inner().collection_name.as_str(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Shard keys are only used when custom sharding is enabled.
I wonder if it makes sense to just return an error if custom sharding is not enabled.
There was a problem hiding this comment.
I don't think we should error.
The endpoint is /collections/{collection_name}/shards, and right now the response we want is
{
"shard_keys": [
{ "key": "my_shard_key" }
]
}
Since semantics refer to shards in general, this same endpoint can be extended to add another field called "shards". Something like
{
"shard_keys": [
{ "key": "my_shard_key", "shard_ids": [1] }
],
"shards": [
{ "id": 1 }
]
}
Or just "shards" in the case of ShardingMethod::Auto
{
"shards": [
{ "id": 1 }
]
}
There was a problem hiding this comment.
I hadn’t considered errors in my last changes, as @coszio point makes sense to me. I’ll defer to you guys’ call.
|
Having quickly discussed this internally. Lets use a dedicated type for each item in the list we return. That allows us to extend it in the future. I expect this to be similar to the collections listing ( In code:
So, for the shard key listing I'd expect to use a wrapping struct like this: #[derive(Debug, Serialize, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub struct ShardKeyDescription {
pub shard_key: ShardKey,
} |
coszio
left a comment
There was a problem hiding this comment.
Good work! My review mostly covers nits, but requesting changes due to #7615 (comment)
src/tonic/api/collections_api.rs
Outdated
| let shard_keys: CollectionShardKeys = do_get_collection_shard_keys( | ||
| self.dispatcher.toc(&access, &pass), | ||
| access, | ||
| request.into_inner().collection_name.as_str(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
I don't think we should error.
The endpoint is /collections/{collection_name}/shards, and right now the response we want is
{
"shard_keys": [
{ "key": "my_shard_key" }
]
}
Since semantics refer to shards in general, this same endpoint can be extended to add another field called "shards". Something like
{
"shard_keys": [
{ "key": "my_shard_key", "shard_ids": [1] }
],
"shards": [
{ "id": 1 }
]
}
Or just "shards" in the case of ShardingMethod::Auto
{
"shards": [
{ "id": 1 }
]
}
c40273e to
bb33a83
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
coszio
left a comment
There was a problem hiding this comment.
Thanks a lot for taking the time to address this!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/api/build.rs (1)
118-189: Add collection_name validation for ListShardKeysRequest
ListShardKeysRequestcurrently only appears inextra_derives, so it gets aValidateimpl but no actual constraints oncollection_name, unlike other collection-level requests. This creates an inconsistent validation surface for the new RPC.Recommend adding a field-level validation entry mirroring existing
collection_namerules:fn configure_validation(builder: Builder) -> Builder { builder // prost_wkt_types needed for serde support .extern_path(".google.protobuf.Timestamp", "::prost_wkt_types::Timestamp") // Service: collections.proto .validates(&[ ("GetCollectionInfoRequest.collection_name", "length(min = 1, max = 255), custom(function = \"common::validation::validate_collection_name_legacy\")"), ("CollectionExistsRequest.collection_name", "length(min = 1, max = 255), custom(function = \"common::validation::validate_collection_name_legacy\")"), ("CreateCollection.collection_name", "length(min = 1, max = 255), custom(function = \"common::validation::validate_collection_name\")"), ... ("ListCollectionAliasesRequest.collection_name", "length(min = 1, max = 255), custom(function = \"common::validation::validate_collection_name_legacy\")"), + ("ListShardKeysRequest.collection_name", "length(min = 1, max = 255), custom(function = \"common::validation::validate_collection_name_legacy\")"), ("HnswConfigDiff.ef_construct", "range(min = 4)"), ... ], &[ "ListCollectionsRequest", "ListAliasesRequest", "CollectionClusterInfoRequest", ... - "Replica", - "ListShardKeysRequest", + "Replica", + "ListShardKeysRequest", ])(Keeping
"ListShardKeysRequest"inextra_derivesis harmless due to dedup, but optional once the field rule is present.)
♻️ Duplicate comments (2)
lib/collection/src/shards/shard_holder/shard_mapping.rs (1)
69-72: Shard-key iterator API matches existing mapping helpers
iter_shard_keyscleanly exposesHashMap::keys()and mirrorsiter_shard_ids, giving callers an efficient way to enumerate shard keys without cloning.lib/api/src/grpc/qdrant.rs (1)
1983-1991: Add collection_name validation to ListShardKeysRequest for consistency
ListShardKeysRequestderivesvalidator::Validatebutcollection_namelacks the length +validate_collection_name_legacyconstraints used on other collection-name requests (e.g.GetCollectionInfoRequest,CollectionExistsRequest). This meansvalidate(request.get_ref())?in the tonic handler does not enforce any constraints on this field, unlike the rest of the API surface.Mirror the same validation attributes on
collection_namein the corresponding.protodefinition and regenerate this file, instead of editing it directly. Based on learnings,lib/api/src/grpc/qdrant.rsis prost-generated from the proto specs.
🧹 Nitpick comments (4)
tests/openapi/helpers/collection_setup.py (1)
136-136: Consider conditionally including sharding_method.Currently,
"sharding_method": sharding_methodwill include the field with anullvalue whensharding_method=None. While this may work if the backend treatsnullas absent, it would be cleaner to only include the field when it has a value:body = { "vectors": { "size": 4, "distance": "Dot", "on_disk": on_disk_vectors, }, "sparse_vectors": { "sparse-text": {}, }, "on_disk_payload": on_disk_payload, "wal_config": { "wal_capacity_mb": wal_capacity, } } if sharding_method is not None: body["sharding_method"] = sharding_methodtests/openapi/test_shard_key.py (1)
53-54: Improve assertion precision.The current assertions check if the values appear in any field of the dictionary items, which is imprecise. Consider checking the specific
"key"field:- assert any("test_key" in item.values() for item in shard_keys) - assert any(100 in item.values() for item in shard_keys) + assert any(item.get("key") == "test_key" for item in shard_keys) + assert any(item.get("key") == 100 for item in shard_keys)This makes the test more robust and clearer in intent.
docs/redoc/master/openapi.json (1)
16009-16019: Prefer required, non-null list; add example.Return an empty array when sharding is auto instead of null. This matches other list responses (e.g., CollectionsResponse.collections) and simplifies clients.
Apply:
- "ShardKeysResponse": { + "ShardKeysResponse": { "type": "object", + "required": ["shard_keys"], "properties": { "shard_keys": { - "description": "The existing shard keys. Only available when sharding method is `custom`", + "description": "Existing shard keys. Empty array when sharding method is `auto`.", "type": "array", "items": { "$ref": "#/components/schemas/ShardKeyDescription" - }, - "nullable": true + } } - } + }, + "example": { + "shard_keys": [ + { "shard_key": "region_1" }, + { "shard_key": 12 } + ] + } },lib/api/src/grpc/qdrant.rs (1)
2007-2022: Consider renaming ShardKeyDescription.key to shard_key for API clarityThe wrapper type is great for future extensibility, but the field name
keyis quite generic and slightly inconsistent with other shard-related types (LocalShardInfo.shard_key,ShardKeySelector.shard_keys). Renaming this field toshard_keyin the proto (and regenerating) would make the API more self-descriptive and consistent with existing naming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/redoc/master/openapi.json(2 hunks)lib/api/build.rs(1 hunks)lib/api/src/grpc/conversions.rs(2 hunks)lib/api/src/grpc/proto/collections.proto(1 hunks)lib/api/src/grpc/proto/collections_service.proto(1 hunks)lib/api/src/grpc/qdrant.rs(5 hunks)lib/api/src/rest/models.rs(2 hunks)lib/collection/src/shards/shard_holder/shard_mapping.rs(1 hunks)openapi/openapi-shards.ytt.yaml(1 hunks)src/actix/api/shards_api.rs(2 hunks)src/common/collections.rs(2 hunks)src/schema_generator.rs(2 hunks)src/tonic/api/collections_api.rs(2 hunks)tests/consensus_tests/auth_tests/test_jwt_access.py(2 hunks)tests/openapi/helpers/collection_setup.py(3 hunks)tests/openapi/test_shard_key.py(1 hunks)tests/openapi_consistency_check.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicitSomeType::from(x)over implicitx.into()in Rust
In new code, don't usetransmute_from_u8,transmute_to_u8,transmute_from_u8_to_slice,transmute_from_u8_to_mut_slice,transmute_to_u8_slice- usebytemuckorzerocopycrates instead
Prefer explicit exhaustive matches over catch-all_arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using: _over using..in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)
Files:
lib/collection/src/shards/shard_holder/shard_mapping.rssrc/common/collections.rssrc/schema_generator.rslib/api/src/grpc/conversions.rslib/api/src/rest/models.rslib/api/build.rssrc/tonic/api/collections_api.rssrc/actix/api/shards_api.rslib/api/src/grpc/qdrant.rs
🧠 Learnings (9)
📚 Learning: 2025-03-20T13:19:35.328Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 6209
File: lib/collection/src/shards/shard_holder/shard_mapping.rs:119-122
Timestamp: 2025-03-20T13:19:35.328Z
Learning: The `to_map()` method in `ShardKeyMappingWrapper` is a temporary solution that will be replaced in a future version (likely in version 1.15 based on the TODO comment). Performance benchmarking for this method is not necessary as it's not used frequently and will be replaced with an implementation that doesn't require cloning.
Applied to files:
lib/collection/src/shards/shard_holder/shard_mapping.rs
📚 Learning: 2025-09-08T08:47:43.795Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7188
File: lib/collection/src/update_handler.rs:810-813
Timestamp: 2025-09-08T08:47:43.795Z
Learning: In the Qdrant codebase, CollectionId implements traits that allow &CollectionId to convert Into<String>, so passing &collection_name to functions requiring Into<String> works correctly without needing .clone() or .to_string().
Applied to files:
src/common/collections.rs
📚 Learning: 2025-09-16T19:14:17.614Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.
Applied to files:
src/schema_generator.rslib/api/src/grpc/conversions.rslib/api/src/grpc/qdrant.rs
📚 Learning: 2025-08-10T18:31:56.855Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/point_ops.rs:501-528
Timestamp: 2025-08-10T18:31:56.855Z
Learning: In Qdrant, batch operations validate that `ids`, `vectors`, and `payloads` (if present) have matching lengths at the REST API level in `lib/api/src/rest/validate.rs` through the `Validate` trait implementation for `Batch`. This validation happens before data is converted to internal structures like `PointInsertOperationsInternal`, so methods operating on these internal structures can safely assume the lengths match.
Applied to files:
src/schema_generator.rs
📚 Learning: 2025-08-10T18:30:02.986Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Applied to files:
src/schema_generator.rs
📚 Learning: 2025-08-10T18:26:12.443Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:13645-13652
Timestamp: 2025-08-10T18:26:12.443Z
Learning: In qdrant, docs/redoc/master/openapi.json is auto-generated from the REST schemas. To change field docs, edit lib/api/src/rest/schema.rs (e.g., add doc comments or #[schemars(description = ...)]). Specifically, UpdateVectors.update_filter lacked a description and should state: "If specified, only update vectors for points that match this filter; points not matching the filter are left unchanged."
Applied to files:
lib/api/src/rest/models.rs
📚 Learning: 2025-08-10T18:25:16.206Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:10626-10634
Timestamp: 2025-08-10T18:25:16.206Z
Learning: In Qdrant, docs/redoc/master/openapi.json is autogenerated from the Rust REST schema definitions (e.g., lib/api/src/rest/schema.rs). To change OpenAPI descriptions, update the Rust struct field doc comments/attributes rather than editing the JSON directly.
Applied to files:
lib/api/src/rest/models.rs
📚 Learning: 2025-08-15T15:56:36.821Z
Learnt from: coszio
Repo: qdrant/qdrant PR: 7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.
Applied to files:
lib/api/build.rslib/api/src/grpc/qdrant.rs
📚 Learning: 2025-10-16T13:00:46.809Z
Learnt from: coszio
Repo: qdrant/qdrant PR: 7368
File: lib/shard/src/query/mod.rs:313-341
Timestamp: 2025-10-16T13:00:46.809Z
Learning: In the Qdrant codebase, for the `Feedback` query variant in `lib/shard/src/query/mod.rs`, validation for sparse vectors should happen down the line rather than early in the `query_enum_from_grpc_raw_query` function. The `Nearest` variant is treated as a special case where early validation with better error messages is acceptable.
Applied to files:
lib/api/src/grpc/qdrant.rs
🧬 Code graph analysis (4)
src/common/collections.rs (1)
lib/collection/src/shards/shard_holder/mod.rs (1)
new(85-122)
src/tonic/api/collections_api.rs (2)
lib/api/src/grpc/conversions.rs (18)
request(2752-2756)request(2757-2761)from(132-147)from(151-162)from(166-177)from(181-188)from(192-204)from(208-224)from(228-240)from(244-249)from(253-286)from(290-295)from(299-311)from(315-327)from(331-342)from(346-357)from(361-372)from(376-402)src/tonic/auth.rs (1)
extract_access(111-115)
tests/consensus_tests/auth_tests/test_jwt_access.py (1)
lib/storage/src/rbac/ops_checks.rs (13)
check_access(78-78)check_access(109-111)check_access(123-126)check_access(138-140)check_access(152-154)check_access(166-168)check_access(180-188)check_access(200-203)check_access(215-217)check_access(229-237)check_access(263-265)check_access(277-279)check_access(300-302)
lib/api/src/grpc/qdrant.rs (1)
src/actix/api/shards_api.rs (1)
list_shard_keys(18-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration-tests-consensus
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: e2e-tests
- GitHub Check: integration-tests
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: Build Qdrant Edge Python bindings
🔇 Additional comments (18)
tests/openapi_consistency_check.sh (1)
38-49: OpenAPI API-count bump looks consistent with new endpointUpdating
EXPECTED_NUMBER_OF_APISto 70 aligns with adding the new shard-keys endpoint and associated schemas; the script’s checks and messaging remain correct.lib/api/src/grpc/proto/collections_service.proto (1)
61-64: ListShardKeys RPC wiring is consistent with existing shard-key RPCsThe new
ListShardKeysmethod, docs, and placement next to create/delete shard-key operations are coherent and follow existing service conventions.src/schema_generator.rs (1)
3-4: Schema generator extension for ShardKeysResponse is correctImporting
ShardKeysResponseand adding it as fieldboinAllDefinitionsmatches the pattern used for other REST models, ensuring the new response type is included in generated schemas.Also applies to: 41-100
lib/api/src/grpc/conversions.rs (1)
51-60: gRPC ListShardKeysResponse conversion is consistent and efficientThe new
(Instant, ShardKeysResponse) -> ListShardKeysResponseconversion correctly:
- Flattens the REST shard-key collection into gRPC
ShardKeyDescriptionitems.- Reuses
convert_shard_key_to_grpcfor explicit type conversion.- Sets
timefromtiming.elapsed().as_secs_f64(), matching other list-style responses.Imports added for
ListShardKeysResponse,ShardKeyDescription, andShardKeysResponseare minimal and appropriate.Also applies to: 65-66, 131-148
lib/api/src/grpc/proto/collections.proto (1)
793-812: Shard-keys proto messages follow existing collection patterns
ListShardKeysRequest,ShardKeyDescription, andListShardKeysResponsemirror established collection APIs: a request keyed bycollection_name, a dedicated item wrapper, and a response containing a list plus processing time. The design is consistent and future‑proof.openapi/openapi-shards.ytt.yaml (1)
31-43: OpenAPI GET /collections/{collection_name}/shards definition is well-formedThe new
list_shard_keysoperation is correctly attached under the shards path, reuses the existingcollection_namepath parameter pattern, and returns the newShardKeysResponseschema, matching the rest of the shard-key API surface.src/tonic/api/collections_api.rs (1)
251-271: LGTM! Implementation follows established patterns.The handler correctly implements the shard key listing logic following the same pattern as other read-only collection operations in this file (e.g.,
list,list_collection_aliases). The use ofnew_unchecked_verification_passis appropriate for read operations.tests/consensus_tests/auth_tests/test_jwt_access.py (2)
251-257: LGTM! Access configuration is appropriate.The access flags (True, True, True) correctly allow read, collection read/write, and manage levels to list shard keys, which is appropriate for a read-only metadata operation. This aligns with similar operations like
list_collection_aliasesandget_collection_cluster_info.
1204-1210: LGTM! Test follows established patterns.The test correctly validates access control for the new
list_shard_keysendpoint using the same pattern as other endpoint tests in this file.tests/openapi/helpers/collection_setup.py (1)
160-162: LGTM! Early return is appropriate for custom sharding.Skipping point insertion when
sharding_method == "custom"is correct because custom sharding requires explicit shard key creation before points can be inserted. This allows test code to create shard keys as needed.src/common/collections.rs (1)
93-115: LGTM! Implementation correctly handles both sharding methods.The function appropriately returns
NoneforShardingMethod::Auto(where shard keys don't apply) and iterates the shard key mapping forShardingMethod::Custom. The access check and error handling are consistent with similar functions in this file.lib/api/src/rest/models.rs (2)
165-169: LGTM! Good use of wrapper type for extensibility.The
ShardKeyDescriptionstruct wraps the shard key, allowing for future extension with additional fields (e.g., shard IDs, metadata) without breaking the API, as discussed in the PR comments.
171-177: LGTM! Response structure is well-designed.The
Option<Vec<ShardKeyDescription>>correctly represents that shard keys are only present for custom sharding. The doc comment clearly explains this, andskip_serializing_ifensures cleaner JSON output.src/actix/api/shards_api.rs (1)
17-32: LGTM! Handler follows Actix patterns correctly.The implementation properly uses the
helpers::timewrapper andnew_unchecked_verification_passfor this read-only operation, consistent with other handlers in the codebase.docs/redoc/master/openapi.json (1)
99-172: LGTM: REST shape aligns with existing patterns.GET List shard keys endpoint looks consistent (tags, operationId, envelope).
lib/api/src/grpc/qdrant.rs (3)
2934-2959: Client wiring for ListShardKeys RPC looks correctThe new
CollectionsClient::list_shard_keysmethod follows the same pattern as existing RPCs (ready check, codec, path/qdrant.Collections/ListShardKeys, method metadata, compression settings). No issues spotted here.
3073-3080: Collections service trait extension is consistentThe added
list_shard_keysmethod on theCollectionstrait uses the expected request/response types and matches the client/server routing. Interface shape is consistent with other collection RPCs.
3768-3813: Server routing for /qdrant.Collections/ListShardKeys is correctly hooked upThe new match arm registers
/qdrant.Collections/ListShardKeys, wires it throughListShardKeysSvc<T>, and applies the same compression and message-size config as other methods. This matches the client path and trait signature.
|
We've just released Qdrant 1.16.2 which includes these changes. Note that we only update our API spec and clients in 1.17.0, but the implemented call is usable now. |
|
@timvisee i've submitted a PR to the client would appreciate a review when you have time! |
|
We've released Qdrant 1.17.0 which includes this feature. Documentation: https://qdrant.tech/documentation/guides/distributed_deployment/#user-defined-sharding |
related to #4002