Repository navigation
Preserve proto3 field presence for falsy values in REST/gRPC conversion - #1260
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated REST↔gRPC conversions to distinguish unset optional fields from explicit values, preserving falsy Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
Ran all three of these against master rather than reading the diff, and they check out. Then went looking for the same pattern elsewhere in the file and found eight more instances the PR does not touch, on the write path.
Verification of what is here.
######## master ########
scored: HasField(shard_key)=False HasField(order_value)=False
record: HasField(shard_key)=False HasField(order_value)=False
unset IntegerIndexParams -> range=False lookup=False is_principal=True
SparseVectorParams() -> index=SparseIndexParams(full_scan_threshold=None, on_disk=None, datatype=None)
######## this branch ########
scored: HasField(shard_key)=True HasField(order_value)=True
record: HasField(shard_key)=True HasField(order_value)=True
unset IntegerIndexParams -> range=None lookup=None is_principal=True
SparseVectorParams() -> index=None
The round trip argument is the cleanest way to see why 2 and 3 matter. grpc -> rest -> grpc on master:
IntegerIndexParams(is_principal=True) roundtrip equal: False
IntegerIndexParams() roundtrip equal: False
SparseVectorParams() roundtrip equal: False
on this branch:
IntegerIndexParams(is_principal=True) roundtrip equal: True
IntegerIndexParams(range=True, lookup=False) roundtrip equal: True
IntegerIndexParams() roundtrip equal: True
SparseVectorParams() roundtrip equal: True
SparseVectorParams(index=...on_disk=True) roundtrip equal: True
So these are not cosmetic, master genuinely cannot round trip an unset range/lookup or an absent sparse index. HasField("range") not raising also confirms both are declared optional in the proto, which was the thing I most expected to be wrong.
The HasField("index") is not None line is a nice find, HasField returns a plain bool so that guard was unconditionally true and every SparseVectorParams without an index came back carrying a fabricated all-None SparseIndexParams.
All three new tests fail on master with only conversion.py reverted, so they are not just encoding the new behavior:
FAILED test_convert_point_falsy_shard_key_and_order_value
FAILED test_convert_integer_index_params_optional_flags
FAILED test_convert_sparse_vector_params_without_index
Full tests/conversions/test_validate_conversions.py is 24 passed on the branch. Worth noting the fix also covers shard_key="", since ShardKey = Union[StrictInt, StrictStr] and the empty string is equally falsy. I checked, it survives on the branch and is dropped on master. Might be worth an assertion, it is free.
The gap. RestToGrpc.convert_points_update_operation has the identical truthiness guard in eight places:
4598: if model.upsert.shard_key
4622: if model.delete.shard_key
4643: if model.set_payload.shard_key
4667: if model.overwrite_payload.shard_key
4691: if model.delete_payload.shard_key
4705: if model.clear_payload.shard_key
4719: if model.update_vectors.shard_key
4748: if model.delete_vectors.shard_key
Still reproduces on this branch:
UpsertOperation shard_key=0 survives? False shard_key=7 survives? True
DeleteOperation shard_key=0 survives? False shard_key=7 survives? True
ClearPayloadOperation shard_key=0 survives? False shard_key=7 survives? True
This one bothers me more than the two the PR does fix. convert_scored_point and convert_record are response-shaped, so a dropped shard_key mostly costs the caller some metadata. convert_points_update_operation feeds batch_update_points, so an integer shard key of 0 silently loses its routing and the write lands wherever the server puts an unspecified selector. Silent misrouting on a write is a worse failure mode than a missing field on a read.
The correct idiom is already in the file, about 470 lines above, in convert_query_request:
shard_key_selector=(
cls.convert_shard_key_selector(model.shard_key) if model.shard_key is not None else None
),so those eight are just stragglers that never got updated. Same one-token change you already made twice. (ShardKeySelector also admits List[ShardKey], so an empty list is falsy at those sites too, though that input is degenerate enough that I would not worry about it.)
I would rather see them in this PR than in a follow-up, since the PR is already "audit the falsy guards in conversion.py" and leaving half the audit undone makes the next person assume it was finished. Everything already in the diff looks right to me.
|
Thanks for the detailed verification and for tracing the same pattern through the write path. I reproduced all eight |
|
Hey @winklemad |
Three spots in conversions/conversion.py lost information for legitimately
falsy values when converting between the REST and gRPC models:
- ScoredPoint/Record: a shard_key of 0 or an order_value of 0/0.0 was
dropped by a truthiness check (`if model.shard_key`), so it never made it
into the gRPC message. Check `is not None` instead.
- IntegerIndexParams: `range` and `lookup` are optional bools but were read
without a HasField guard, so an unset field turned into False instead of
None, silently overriding the server-side default.
- SparseVectorParams: the index guard was `HasField("index") is not None`,
which is always true since HasField returns a bool, so an empty
SparseIndexParams was fabricated even when no index was configured.
Added regression tests covering all three.
a3a3159 to
8464339
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conversions/fixtures.py`:
- Around line 518-533: Update the order-value fixture variants in the conversion
fixtures so falsy-value cases explicitly use grpc.OrderValue(int=0), and ensure
the float variants provide an explicit zero-valued order value rather than
omitting the field. Apply this consistently to both ScoredPoint and
RetrievedPoint fixtures.
- Line 633: Update the integer-index test data to use integer_index_params_2 in
payload_schema_integer_default and include integer_index_params_2 in the
IntegerIndexParams fixture list. Preserve the existing fixtures while ensuring
the unset-presence case for the optional range and lookup fields is exercised.
- Around line 534-556: Update the four fixtures retrieved_point,
retrieved_point_order_value_int, retrieved_point_order_value_float, and
retrieved_point_multivector to construct grpc.RetrievedPoint instead of
grpc.ScoredPoint. Apply the same type correction to the corresponding fixtures
around lines 1766-1772, preserving their existing fields and values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10b84b77-36f1-4157-b2b6-c4145f1b42a9
📒 Files selected for processing (1)
tests/conversions/fixtures.py
| scored_point_shard_key_order_value_falsy_value = grpc.ScoredPoint( | ||
| id=point_id, | ||
| payload=payload, | ||
| score=0.99, | ||
| vectors=single_vector_output, | ||
| version=12, | ||
| shard_key=grpc.ShardKey(number=0), | ||
| order_value=grpc.OrderValue(int=3), | ||
| ) | ||
| retrieved_point_shard_key_order_value_falsy_value = grpc.RetrievedPoint( | ||
| id=point_id, | ||
| payload=payload, | ||
| vectors=single_vector_output, | ||
| shard_key=grpc.ShardKey(number=0), | ||
| order_value=grpc.OrderValue(int=3), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant fixture references.
sed -n '500,560p' tests/conversions/fixtures.py
# Search for order_value fixture expectations/usages.
rg -n "order_value|OrderValue|retrieved_point_order_value|scored_point_order_value" tests/conversions fixtures tests -S || true
# Lightweight check for the declared values and whether comments are present.
python3 - <<'PY'
from pathlib import Path
p = Path('tests/conversions/fixtures.py')
lines = p.read_text().splitlines()
for i in range(510, 560):
if i < len(lines) and ('retrieved_point_order_value' in lines[i-1] or 'scored_point_order_value' in lines[i-1] or 'OrderValue' in lines[i-1]):
print(f"{i}: {lines[i-1]}")
PYRepository: qdrant/qdrant-client
Length of output: 6680
Use actual order-value inputs in the order-value fixture variants.
The *_order_value_falsy_value fixtures pass grpc.OrderValue(int=3), so they do not exercise 0. The *_order_value_float fixtures omit the order_value field entirely, so they do not exercise integer-false ordering. Add explicit zero-valued fixtures for both int and float where coverage is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conversions/fixtures.py` around lines 518 - 533, Update the order-value
fixture variants in the conversion fixtures so falsy-value cases explicitly use
grpc.OrderValue(int=0), and ensure the float variants provide an explicit
zero-valued order value rather than omitting the field. Apply this consistently
to both ScoredPoint and RetrievedPoint fixtures.
| integer_index_params_1 = grpc.IntegerIndexParams( | ||
| lookup=True, range=True, on_disk=True, is_principal=True, enable_hnsw=True | ||
| ) | ||
| integer_index_params_2 = grpc.IntegerIndexParams() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate fixture file =="
fd -a 'fixtures.py$' . | sed 's#^\./##'
echo "== inspect relevant fixture lines =="
sed -n '600,715p' tests/conversions/fixtures.py
echo "== search integer_index_params registry usages =="
rg -n 'integer_index_params_[0-9]|payload_schema_integer_default|IntegerIndexParams|integer.*lookup|integer.*range' tests/conversions/fixtures.py tests/conversions -S || trueRepository: qdrant/qdrant-client
Length of output: 6289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== proto files around IntegerIndexParams =="
fd -a '\.proto$' . | sed 's#^\./##' | head -50
rg -n 'message IntegerIndexParams|optional bool lookup|bool lookup|optional bool range|bool range' . -S --glob '*.proto'
echo "== fixture exports involving integer schema and registry =="
sed -n '740,810p' tests/conversions/fixtures.py
sed -n '1770,1810p' tests/conversions/fixtures.py
echo "== python/pytest availability =="
python3 - <<'PY'
print('python available')
import grpc
print('grpc available, version', getattr(grpc, '__version__','?'))
PY || true
python3 - <<'PY'
try:
from qdrant_client.http import api
print('qdrant_client.http.api available')
except Exception as e:
print('qdrant_client.http.api unavailable:', type(e).__name__, str(e))
PY || trueRepository: qdrant/qdrant-client
Length of output: 5599
Wire the empty integer-index fixture into the test data.
integer_index_params_2 is defined as the empty/default fixture, but payload_schema_integer_default still uses integer_index_params_0, and the IntegerIndexParams fixture list omits _2. Since range and lookup are separate optional proto fields, the unset-presence case is not covered unless this fixture is referenced in both places.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conversions/fixtures.py` at line 633, Update the integer-index test
data to use integer_index_params_2 in payload_schema_integer_default and include
integer_index_params_2 in the IntegerIndexParams fixture list. Preserve the
existing fixtures while ensuring the unset-presence case for the optional range
and lookup fields is exercised.
…on (#1260) Co-authored-by: George Panchuk <[email protected]>
No linked issue — found by reading the REST↔gRPC conversion code.
Summary
Three conversions in
qdrant_client/conversions/conversion.pylose information for values that are legitimately falsy (0,0.0,False, or an unset optional). In each case a falsy-but-present value ends up indistinguishable from "missing", which changes the meaning of the request/response.Bugs
1.
shard_key/order_valueof0dropped (REST → gRPC)convert_scored_pointandconvert_recordguarded these with a plain truthiness check:An integer shard key of
0or an order value of0/0.0is a valid value, butif model.shard_keyisFalsefor it, so the field never made it into the gRPC message. Switched tois not None.2.
IntegerIndexParams.range/.lookupunset →Falseinstead ofNone(gRPC → REST)Both are
optional boolfields, so reading them without aHasFieldguard returns the proto defaultFalsewhen they were never set — turning "unspecified" into an explicitFalseand silently overriding the server-side default. Every sibling field in the same struct (is_principal,on_disk,enable_hnsw) already guards withHasField;range/lookupnow match.3.
SparseVectorParams.indexfabricated when unset (gRPC → REST)HasField(...)returns abool, andbool is not Noneis alwaysTrue, so the guard never short-circuits. When no index was set,model.indexis a default-constructed empty message and an emptySparseIndexParams(full_scan_threshold=None, on_disk=None, datatype=None)was produced instead ofNone. Dropped the deadis not None.Tests
Added three focused tests in
tests/conversions/test_validate_conversions.py:test_convert_point_falsy_shard_key_and_order_value—0shard key / order value survive bothScoredPointandRecord, andNonestill maps to an unset field.test_convert_integer_index_params_optional_flags— unsetrange/lookup→None, explicitTrue/Falsepreserved.test_convert_sparse_vector_params_without_index— no index →None, set index still converts.Full
tests/conversions/test_validate_conversions.pypasses (24 tests), andruff format --line-length 99(0.4.3) is clean.