Repository navigation
Fix local mode filters cross-matching booleans and integers - #1259
Conversation
Python treats bool as a subclass of int (True == 1, False == 0), but Qdrant keeps booleans and integers as distinct payload value types. Local mode compared them with a plain `==` / `in` / `isinstance(value, (int, float))`, so: - MatchValue(value=1) matched a payload of True, and MatchValue(value=True) matched a payload of 1 (same for 0 / False) - MatchAny / MatchExcept cross-matched the same way - Range matched booleans as if they were 0 / 1 The server never cross-matches these (its ValueVariants keeps Integer and Bool distinct, and booleans are not numeric for range conditions). Add a type-aware equality helper used by the value-match conditions, and exclude booleans from range checks. Adds an in-memory regression test.
✅ 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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLocal payload filtering now rejects booleans in range checks and uses type-aware comparisons for exact, Any, and Except matches. Congruence tests create payloads containing boolean, integer, and float values, then compare local and remote scroll results across multiple filter conditions and client modes. Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_in_memory.py (1)
325-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a
MatchExceptassertion.
check_matchnow routesMatchExceptthroughvalues_match, but this test only exercisesMatchValue,MatchAny, andRange. AMatchExceptcase (e.g.except_=[1]should excluden=1but keepn=True) would lock in the boolean/integer separation for that changed branch as well.💚 Proposed additional assertion
assert matched_ids(models.FieldCondition(key="n", match=models.MatchAny(any=[1, 0]))) == [1, 3] + assert matched_ids( + models.FieldCondition(key="n", match=models.MatchExcept(**{"except": [1]})) + ) == [2, 3, 4] assert matched_ids(models.FieldCondition(key="n", range=models.Range(gte=1, lte=1))) == [1]🤖 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/test_in_memory.py` around lines 325 - 330, Add a MatchExcept assertion in the existing in-memory matching test near the other FieldCondition cases, verifying that except_=[1] excludes the integer value 1 while retaining the boolean value True, thereby covering the values_match path and boolean/integer distinction.
🤖 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.
Nitpick comments:
In `@tests/test_in_memory.py`:
- Around line 325-330: Add a MatchExcept assertion in the existing in-memory
matching test near the other FieldCondition cases, verifying that except_=[1]
excludes the integer value 1 while retaining the boolean value True, thereby
covering the values_match path and boolean/integer distinction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a00b34e5-632d-42d6-beb0-f0a75901dbfa
📒 Files selected for processing (2)
qdrant_client/local/payload_filters.pytests/test_in_memory.py
MatchExcept also routes through values_match, so assert that except=[1] keeps the True payload (bool is not the integer 1).
eeshsaxena
left a comment
eeshsaxena
left a comment
There was a problem hiding this comment.
This looks correct and the root cause is right - bool subclassing int (so True == 1 and True in [1]) is exactly what makes the old value == condition.value cross-match. I reproduced all the affected paths on main:
check_match(MatchValue(value=1), True) -> True # should be False
check_match(MatchValue(value=True), 1) -> True # should be False
check_match(MatchAny(any=[1, 2]), True) -> True # should be False
check_range(Range(gt=0), True) -> True # should be False
and confirmed the real matches still pass (MatchValue(1) vs 1, MatchValue(True) vs True).
A couple of things I checked that are worth noting for confidence:
- Array payloads are covered.
check_conditionflattens list-valued payloads and callscheck_match(condition.match, v)per element (line 199), so routing everything throughvalues_matchalso fixes the array case ([True]no longer matchingMatchValue(1)), not just scalar payloads. - Range via the interface wrapper is covered too, since
check_range_interfacedelegates to the patchedcheck_range. MatchExceptflips correctly:not any(values_match(...))means aTruepayload is no longer wrongly excluded by anexcept_=[1].
One small suggestion: the added test covers MatchValue, but since the same values_match helper now backs MatchAny/MatchExcept and the separate check_range guard, a couple of extra asserts would lock in those paths against regressions - e.g. MatchAny(any=[1]) vs True, MatchExcept(except_=[1]) vs True (should keep the point), and a Range vs True. Entirely optional.
Nice, well-scoped fix. Deferring to the maintainers.
Lock the single-value MatchAny path and the check_range bool guard against regressions, in addition to the existing combined-condition coverage.
|
Thanks @eeshsaxena — appreciate the careful review and reproducing the paths. Added the isolated asserts in a270fb9: |
|
Hey @winklemad Thanks for pointing it out and fixing it! Thank you @eeshsaxena for the review 👍 I'll take a look at it One problem is that we need to compare local mode implementation against the remote mode. I'll address it |
* Fix local mode filters cross-matching booleans and integers Python treats bool as a subclass of int (True == 1, False == 0), but Qdrant keeps booleans and integers as distinct payload value types. Local mode compared them with a plain `==` / `in` / `isinstance(value, (int, float))`, so: - MatchValue(value=1) matched a payload of True, and MatchValue(value=True) matched a payload of 1 (same for 0 / False) - MatchAny / MatchExcept cross-matched the same way - Range matched booleans as if they were 0 / 1 The server never cross-matches these (its ValueVariants keeps Integer and Bool distinct, and booleans are not numeric for range conditions). Add a type-aware equality helper used by the value-match conditions, and exclude booleans from range checks. Adds an in-memory regression test. * Cover MatchExcept in the bool/int cross-match test MatchExcept also routes through values_match, so assert that except=[1] keeps the True payload (bool is not the integer 1). * Add isolated MatchAny and range asserts to the bool/int cross-match test Lock the single-value MatchAny path and the check_range bool guard against regressions, in addition to the existing combined-condition coverage. * fix: handle floats in cross-match local mode filters, add congruence tests --------- Co-authored-by: George Panchuk <george.panchuk@qdrant.tech>
Description
Local mode (
QdrantClient(":memory:")) cross-matches booleans and integers in payload filters, because Python treatsboolas a subclass ofint(True == 1,False == 0) while Qdrant keeps them as distinct payload value types.With
id=1 -> {"n": 1}(int) andid=2 -> {"n": True}(bool):MatchAny,MatchExceptand group-by are affected the same way. The server never cross-matches: itsValueVariantsenum keepsIntegerandBooldistinct, and booleans are not part of numeric range conditions.Fix
values_match(a, b)that returnsFalsewhen exactly one of the two is abool, and use it inMatchValue/MatchAny/MatchExcept.check_range(an explicitisinstance(value, bool)short-circuits to no match).Both live in the shared
qdrant_client/local/payload_filters.py, so the fix covers sync and async local mode (the async engine reuses the same collection).Test
Adds
test_bool_and_int_filters_do_not_cross_matchintests/test_in_memory.pycoveringMatchValue(1 / True / 0 / False),MatchAny, andRange— each now returns only the correctly-typed points, matching the server. Verified againstdev; the fulltests/test_in_memory.pysuite passes andruff format --line-length 99(0.4.3) is clean.