fix: fix geo bounding box filters on edges#1190
Merged
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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)
qdrant_client/local/payload_filters.py (1)
69-75: Document that bbox checks are intentionally open-interval.This behavior change is subtle (edge points are excluded). A short inline note will reduce future regressions/confusion.
💡 Suggested clarification
- # handle anti-meridian crossing case + # Handle anti-meridian crossing case. + # Boundary checks are intentionally strict (open interval) to match + # Rust server behavior: points exactly on bbox edges are excluded. if condition.top_left.lon > condition.bottom_right.lon: longitude_condition = lon > condition.top_left.lon or lon < condition.bottom_right.lon else: longitude_condition = condition.top_left.lon < lon < condition.bottom_right.lon latitude_condition = condition.top_left.lat > lat > condition.bottom_right.lat🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/local/payload_filters.py` around lines 69 - 75, The bbox latitude/longitude checks in the bbox evaluation (variables longitude_condition and latitude_condition within the bbox handling logic referencing condition.top_left, condition.bottom_right, lon, and lat) intentionally use open-interval comparisons (e.g., < and > rather than <= and >=) to exclude points exactly on the bounding box edges; add a concise inline comment above the longitude_condition/latitude_condition assignments stating that edge points are excluded by design (open-interval) to avoid future confusion/regressions and to document the anti-meridian handling branch as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@qdrant_client/local/payload_filters.py`:
- Around line 69-75: The bbox latitude/longitude checks in the bbox evaluation
(variables longitude_condition and latitude_condition within the bbox handling
logic referencing condition.top_left, condition.bottom_right, lon, and lat)
intentionally use open-interval comparisons (e.g., < and > rather than <= and
>=) to exclude points exactly on the bounding box edges; add a concise inline
comment above the longitude_condition/latitude_condition assignments stating
that edge points are excluded by design (open-interval) to avoid future
confusion/regressions and to document the anti-meridian handling branch as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d796648-0a68-4509-8bf4-edbf57f158eb
📒 Files selected for processing (1)
qdrant_client/local/payload_filters.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/congruence_tests/test_geo_polygon_filter_query.py (2)
103-145: Missing anti-meridian edge-case coverage for the changed logic branch.This test covers the non-crossing bbox path only. Since edge handling was also changed for anti-meridian logic, add a companion test where
top_left.lon > bottom_right.lonand include edge/corner points there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/congruence_tests/test_geo_polygon_filter_query.py` around lines 103 - 145, Add a companion test to cover the anti-meridian case where top_left.lon > bottom_right.lon: create a new points list (using models.PointStruct and payload.location with lat/lon) that places points on top/bottom edges, left/right edges, both corners, one strictly inside and one strictly outside for a bbox that crosses the anti-meridian (e.g., top_left=models.GeoPoint(lon=170.0, lat=90.0) and bottom_right=models.GeoPoint(lon=-170.0, lat=69.4)), upload them with local_client.upload_points and remote_client.upload_points (same as the existing test), construct the crossing bbox with models.GeoBoundingBox, and assert results analogous to the non-crossing test so edge/corner handling for anti-meridian is verified.
148-157: Add an explicit semantic assertion for edge exclusion.Right now this only proves local/remote parity. It doesn’t prove the intended behavior (“edges excluded”) if both sides regress the same way. Add a direct ID assertion (expected only inside point
id=7).Proposed patch
compare_client_results( local_client, remote_client, lambda client: client.scroll( collection_name=collection_name, scroll_filter=geo_filter, limit=100, with_payload=True, ), ) + + # semantic guard: only the strictly-inside point should match + remote_records, _ = remote_client.scroll( + collection_name=collection_name, + scroll_filter=geo_filter, + limit=100, + with_payload=True, + ) + assert {point.id for point in remote_records} == {7}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/congruence_tests/test_geo_polygon_filter_query.py` around lines 148 - 157, Add a direct semantic assertion that the polygon-edge point (id 7) is excluded from the geo-filter results in addition to the parity check: after the compare_client_results(...) invocation (which calls client.scroll with collection_name, geo_filter, limit and with_payload), collect the returned item IDs from the scroll response(s) and assert that 7 is not present (e.g., assert 7 not in ids) so the test verifies edge exclusion explicitly rather than relying only on local/remote parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/congruence_tests/test_geo_polygon_filter_query.py`:
- Around line 103-145: Add a companion test to cover the anti-meridian case
where top_left.lon > bottom_right.lon: create a new points list (using
models.PointStruct and payload.location with lat/lon) that places points on
top/bottom edges, left/right edges, both corners, one strictly inside and one
strictly outside for a bbox that crosses the anti-meridian (e.g.,
top_left=models.GeoPoint(lon=170.0, lat=90.0) and
bottom_right=models.GeoPoint(lon=-170.0, lat=69.4)), upload them with
local_client.upload_points and remote_client.upload_points (same as the existing
test), construct the crossing bbox with models.GeoBoundingBox, and assert
results analogous to the non-crossing test so edge/corner handling for
anti-meridian is verified.
- Around line 148-157: Add a direct semantic assertion that the polygon-edge
point (id 7) is excluded from the geo-filter results in addition to the parity
check: after the compare_client_results(...) invocation (which calls
client.scroll with collection_name, geo_filter, limit and with_payload), collect
the returned item IDs from the scroll response(s) and assert that 7 is not
present (e.g., assert 7 not in ids) so the test verifies edge exclusion
explicitly rather than relying only on local/remote parity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3089bc16-3dc8-4e6a-a3c4-3d793fb67589
📒 Files selected for processing (1)
tests/congruence_tests/test_geo_polygon_filter_query.py
tbung
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#1188