Repository navigation
fix: do not drop falsy shard keys in convert_points_update_operation - #1279
Conversation
All eight branches of `RestToGrpc.convert_points_update_operation` gate the
shard key on truthiness:
shard_key_selector = (
cls.convert_shard_key_selector(model.upsert.shard_key)
if model.upsert.shard_key
else None
)
`ShardKey` is `Union[StrictInt, StrictStr]`, so `0` and `""` are valid keys
that evaluate to False. They get silently replaced with `None`, and the
operation is routed by the default hashing rules instead of to the requested
shard. No error is raised on either side, so the write just lands somewhere
else.
Before, converting each operation type with `shard_key=0`:
LOST UpsertOperation shard_key_selector present=False
LOST DeleteOperation shard_key_selector present=False
LOST SetPayloadOperation shard_key_selector present=False
LOST OverwritePayloadOperation shard_key_selector present=False
LOST DeletePayloadOperation shard_key_selector present=False
LOST ClearPayloadOperation shard_key_selector present=False
LOST UpdateVectorsOperation shard_key_selector present=False
LOST DeleteVectorsOperation shard_key_selector present=False
8/8 operations silently dropped the shard key
control, shard_key=1 -> [number: 1]
After:
OK UpsertOperation shard_key_selector present=True keys=[number: 0]
... (all eight)
0/8 operations silently dropped the shard key
This is on the write path: `QdrantRemote.batch_update_points` calls
`RestToGrpc.convert_update_operation` for every operation when the client is
built with `prefer_grpc=True`, so a custom-sharded collection keyed on
integers loses shard 0 on every batch update.
The same conversion elsewhere in the file already uses the `is not None`
form, for example `convert_query_request`:
shard_key_selector=(
cls.convert_shard_key_selector(model.shard_key)
if model.shard_key is not None
else None
),
so this just brings the update path in line with it. The field is
`Optional[ShardKeySelector]` with a `None` default, and `None` is the only
value that should mean "not set".
Tested with `pytest tests/conversions/test_validate_conversions.py`
(22 passed). The new test fails on all eight operations without the
conversion.py change.
✅ 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 (2)
📝 WalkthroughWalkthroughUpdated point update conversion to check whether Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Hey @ErenAta16 Thanks for pointing it out! I'll review the PR |
|
looks good to me! let's wait for the green ci |
What's broken
All eight branches of
RestToGrpc.convert_points_update_operationgate the shard key on truthiness:ShardKey = Union[StrictInt, StrictStr], so0and""are valid shard keys that evaluate toFalse. They are silently replaced withNone, and the operation gets routed by the default rules instead of to the requested shard. Nothing raises, so the write just lands somewhere else.Repro against
dev, converting each operation type withshard_key=0:With this patch:
Why it matters
This is on the write path.
QdrantRemote.batch_update_points(and the async variant) callRestToGrpc.convert_update_operationfor every operation when the client is constructed withprefer_grpc=True. A custom-sharded collection keyed on integers loses shard0on every batch update, and there is no signal that it happened.The fix
Switch the eight guards to
is not None. The field is declaredOptional[ShardKeySelector]with aNonedefault, soNoneis the only value that should mean "not set". This is the idiom the rest of the file already uses, for example inconvert_query_request:so this only brings the update path in line with the read path. No behaviour changes for
Noneor for any truthy key.Testing
test_convert_points_update_operation_falsy_shard_keybuilds each of the eight REST operation types withshard_key=0andshard_key="", converts to gRPC, and asserts the selector survives and round-trips back to the original value.Reverting only the
conversion.pyhunk makes the new test fail on the first operation and leaves the other 21 tests passing, so nothing else in the file depended on the old behaviour.ruff format --line-length=99reports no changes.