fix: support FieldCondition.is_empty/is_null in local mode - #1308
fix: support FieldCondition.is_empty/is_null in local mode#1308sainikhiljuluri wants to merge 2 commits into
Conversation
`FieldCondition` carries `is_empty` and `is_null` as the shorthand syntax for
`IsEmptyCondition` / `IsNullCondition`, and both are wired up for REST and gRPC.
Local mode's `check_condition()` never inspected them, so a `FieldCondition`
carrying `is_empty=`/`is_null=` matched none of the `if` branches and fell
through to the trailing `return False`.
The condition was therefore False for every point, silently: `must` matched
nothing and `must_not` matched everything, with no warning and no
`NotImplementedError`. That affects scroll, count, query_points, facet and
delete(filter=...).
Behaviour was established by running the queries against qdrant/qdrant:dev
rather than by reading core, and is pinned by a congruence test:
- a value is empty when it is null or an empty array; a key holding no value
counts as empty but not null
- a value is null when it is null or an array containing a null
- for a key resolving to several values, any one of them satisfying the
condition is a match, so one point can satisfy both `is_empty=True` and
`is_empty=False`
A condition that also carries `values_count` is left to the existing
`values_count` branch, so its behaviour is unchanged.
Note that on a field without a payload index the server does not treat
`is_null` and `IsNullCondition` as interchangeable, even though the generated
models describe them as alternative syntax: the verbose condition tests the
values a key resolves to, so an array holding a null is not itself null, while
the shorthand looks inside it. Local mode does not model payload indexes, so it
mirrors the unindexed behaviour here, as the surrounding branches already do.
Values are extracted with `flat=False`, like the neighbouring
`IsEmptyCondition`/`IsNullCondition` branches. It is load-bearing:
`{"field": []}` flattens to `None`, which would otherwise collapse the
empty-array case into the no-value case.
✅ 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 Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds local evaluation for Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
qdrant_client/local/payload_filters.py (1)
246-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the is_empty/is_null evaluation into a helper function.
This block mixes precedence gating, missing-key handling, and per-value evaluation inside
check_condition. Extract it into a dedicated helper that returnsbool | None(Nonemeaning "fall through to match/range/values_count handling"). This keepscheck_conditionflatter and easier to follow.♻️ Proposed refactor
+def evaluate_is_empty_is_null( + condition: models.FieldCondition, payload: dict[str, Any] +) -> bool | None: + """Evaluate `is_empty` / `is_null` for a FieldCondition. Returns `None` when the + caller should fall through to match/range/values_count handling instead.""" + if condition.values_count is not None: + return None + if condition.is_empty is None and condition.is_null is None: + return None + raw_values = value_by_key(payload, condition.key, flat=False) + if not raw_values: + # nothing stored under the key: the server counts that as empty, not null + if condition.is_empty is not None: + return condition.is_empty + return not condition.is_null + if condition.is_empty is not None: + return any(check_is_empty_value(condition.is_empty, v) for v in raw_values) + return any(check_is_null_value(condition.is_null, v) for v in raw_values) + + elif isinstance(condition, models.FieldCondition): - if condition.values_count is None and ( - condition.is_empty is not None or condition.is_null is not None - ): - # values_count keeps its own branch below, this must not shadow it - raw_values = value_by_key(payload, condition.key, flat=False) - if not raw_values: - # nothing stored under the key: the server counts that as empty, not null - if condition.is_empty is not None: - return condition.is_empty - return not condition.is_null - if condition.is_empty is not None: - return any(check_is_empty_value(condition.is_empty, v) for v in raw_values) - return any(check_is_null_value(condition.is_null, v) for v in raw_values) + empty_or_null = evaluate_is_empty_is_null(condition, payload) + if empty_or_null is not None: + return empty_or_null values = value_by_key(payload, condition.key)🤖 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 `@qdrant_client/local/payload_filters.py` around lines 246 - 258, Extract the is_empty/is_null branch from check_condition into a dedicated helper returning bool | None, with None when condition.values_count is set or neither predicate applies. Move the existing missing-key behavior and per-value checks unchanged into that helper, then have check_condition delegate to it and return immediately only for non-None results.Source: Linters/SAST tools
tests/congruence_tests/test_is_empty_is_null.py (1)
75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exercising more than
scrollin the congruence loop.The PR objective states this bug affected
scroll,count,query,facet, anddelete, but this test only comparesscrollresults. All these operations share the samecheck_filter/check_conditionlogic, soscrollcoverage substantially validates the fix. Add at least one more operation (for examplecount) to the comparison loop to directly confirm parity for the other operations named in the PR objective.🤖 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/congruence_tests/test_is_empty_is_null.py` around lines 75 - 85, Extend the congruence loop in the test around compare_client_results to exercise at least one additional filter operation, such as count, alongside scroll. Reuse the same local/remote comparison pattern and each filter so parity is directly validated for another operation sharing the filter-checking logic.
🤖 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 `@qdrant_client/local/payload_filters.py`:
- Around line 246-258: Extract the is_empty/is_null branch from check_condition
into a dedicated helper returning bool | None, with None when
condition.values_count is set or neither predicate applies. Move the existing
missing-key behavior and per-value checks unchanged into that helper, then have
check_condition delegate to it and return immediately only for non-None results.
In `@tests/congruence_tests/test_is_empty_is_null.py`:
- Around line 75-85: Extend the congruence loop in the test around
compare_client_results to exercise at least one additional filter operation,
such as count, alongside scroll. Reuse the same local/remote comparison pattern
and each filter so parity is directly validated for another operation sharing
the filter-checking logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d44ce851-e244-4c59-814d-28495a49bc40
📒 Files selected for processing (3)
qdrant_client/local/payload_filters.pyqdrant_client/local/tests/test_payload_filters.pytests/congruence_tests/test_is_empty_is_null.py
The conditions reach every filtered operation, so compare a second one that shares the filter path rather than only scroll.
|
Thanks for the review. Took one of the two: Compare more than Extract the |
1 participant
All Submissions:
devbranch. Did you create your branch fromdev?Changes to Core Features:
The problem
FieldConditioncarriesis_emptyandis_null— the shorthand syntax forIsEmptyCondition/IsNullCondition, described in the generated models as"alternative syntax for
is_empty: 'field_name'". In local mode they are ignoredsilently, and the failure is total rather than partial:
So
mustmatches nothing andmust_notmatches everything — with no warning andno
NotImplementedError. It reachesscroll,count,query_points,facetand
delete(filter=...), which means adeleteguarded bymust_not=[...]canclear a whole local collection while doing almost nothing against a server.
Root cause
check_condition()'sFieldConditionbranch only ever inspectedmatch,range,geo_bounding_box,geo_radius,values_countandgeo_polygon. Acondition carrying
is_empty=/is_null=matches none of thoseifs, falls outof the
elifbody and hits the trailingreturn Falseat the end of thefunction — evaluating to
Falsefor every point.The fix
Two small helpers plus a branch in
check_condition:nullor an empty array; a key holding novalue counts as empty but not null
nullor an array containing anulla[].b), any one of them satisfying thecondition is a match — so one point can satisfy both
is_empty=Trueandis_empty=FalseA condition that also carries
values_countis left to the existingvalues_countbranch, so nothing about its behaviour changes.Values are read with
flat=False, like the two neighbouringIsEmptyConditionand
IsNullConditionbranches. That is load-bearing rather than stylistic:{"field": []}flattens toNone, which would otherwise collapse theempty-array case into the no-value case.
How this was tested
The semantics above were established by running the queries against a real
qdrant/qdrant:dev(1.18.3-dev) container, not by reading core — an earlierrevision of this PR was written from the Rust source and got the
values_countinteraction wrong in a way only the live server revealed.
tests/congruence_tests/test_is_empty_is_null.py(new) — 16 points and 19filters diffed between local mode and a live server, covering a flat key, a
nested key and an array-traversing key, both directions of each condition,
must_not, and both verbose conditions. This follows the same pattern asfix: fix geo bounding box filters on edges #1190 / fix: fix local mode values count #1191 / fix: check_match() raises TypeError when MatchText applied to non-string field #1224 / Fix local mode filters cross-matching booleans and integers #1259.
qdrant_client/local/tests/test_payload_filters.py— 4 new unit tests(72 → 76 collected in that directory).
qdrant/qdrant:dev: 284 passed, matching aclean
devbaseline.ruff-format --line-length=99clean;mypyclean onqdrant_client/local.Risk and compatibility
No public API, signature, default or return-shape change — this is local-mode
internals, and the async local client picks it up unchanged. The only behaviour
that changes is for conditions that are currently broken. In principle someone
could be relying on
must_not=[FieldCondition(is_empty=True)]matchingeverything, but that is the bug rather than a contract.
Two things worth your judgement
1. Payload indexes. Local mode does not model them (it warns as much), so
where the server's answer depends on whether a field is indexed, local mode can
only mirror one path. This PR mirrors the unindexed payload-scan path, which is
what the surrounding local-mode branches already do and what the congruence
fixtures exercise.
That matters in one visible place: on an unindexed field the server does not
treat
is_nullandIsNullConditionas interchangeable — the verbose conditiontests the values a key resolves to, so an array holding a null is not itself
null, while the shorthand looks inside it:
With a keyword index on the field, both match.
is_emptyandIsEmptyConditionagree either way. Since the models call the two formsalternative syntax, this looks like it may be a core inconsistency rather than
intended — happy to flip the client whenever core settles it, and the congruence
test means you would hear about it either way.
2. Deliberately out of scope.
check_conditionevaluatesmatch/range/geo before
values_count, whereas the server prefersvalues_count. Thatpredates this PR and is unchanged by it, so I have not touched it. Glad to send a
separate PR if you want it fixed.
Found by comparing local mode against server behaviour rather than from a
reported issue, so there is no linked issue. AI-assisted: an agent wrote the
patch and tests, and every behavioural claim here was verified against a running
Qdrant container.