-
Notifications
You must be signed in to change notification settings - Fork 31
INTPYTHON-835 Add support for spatial lookups #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All tests (including some simple ones I added due to many of the Django ones requiring features we don't support) passing, marking as ready for an initial review pass. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| if self.name == "distance_gt" or self.name == "distance_gte": | ||
| cmd = { | ||
| field: { | ||
| "$not": { | ||
| "$geoWithin": { | ||
| "$centerSphere": [ | ||
| value["coordinates"], | ||
| distance / 6378100, # radius of earth in meters | ||
| ], | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be moved to DistanceGT and DistanceGTE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more specific? Having it here lets us reduce the code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see a refactor like this: [Incidentally, is gt vs. gte accounted for elsewhere?)
diff --git a/django_mongodb_backend/gis/operators.py b/django_mongodb_backend/gis/operators.py
index cc24473..3824de5 100644
--- a/django_mongodb_backend/gis/operators.py
+++ b/django_mongodb_backend/gis/operators.py
@@ -52,22 +52,28 @@ class DistanceBase(Operator):
def as_mql(self, field, value, params=None):
distance = params[0].m if hasattr(params[0], "m") else params[0]
- if self.name == "distance_gt" or self.name == "distance_gte":
- cmd = {
- field: {
- "$not": {
- "$geoWithin": {
- "$centerSphere": [
- value["coordinates"],
- distance / 6378100, # radius of earth in meters
- ],
- }
- }
+ return self.get_mql(field, value, distance):
+
+ def get_mql(self, field, value, distance):
+ return {
+ field: {
+ "$geoWithin": {
+ "$centerSphere": [
+ value["coordinates"],
+ distance / 6378100, # radius of earth in meters
+ ],
}
}
+ return self.get_mql(field, value, distance):
+
+ def get_mql(self, field, value, distance):
+ return {
+ field: {
+ "$geoWithin": {
+ "$centerSphere": [
+ value["coordinates"],
+ distance / 6378100, # radius of earth in meters
+ ],
}
}
- else:
- cmd = {
- field: {
+ }
+
+
+class DistanceGT(DistanceBase):
+ name = "distance_gt"
+
+ def get_mql(self, field, value, distance):
+ return {
+ field: {
+ "$not": {
"$geoWithin": {
"$centerSphere": [
value["coordinates"],
@@ -76,14 +82,9 @@ class DistanceBase(Operator):
}
}
}
- return cmd
-
-
-class DistanceGT(DistanceBase):
- name = "distance_gt"
-
+ }
-class DistanceGTE(DistanceBase):
+class DistanceGTE(DistanceGT):
name = "distance_gte"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The geospatial functionality supported by the server doesn't allow for a finer-grained gt vs gte filter beyond adding a tiny offset to the given distance. We could add that, but then we are effectively modifying user queries invisibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that only one of gt/gte is really supported then, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put that way yes, we don't support strict LT or GT. Removing those two operators would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like only one of LT or GT should be "equal to". Since the query condition is negated, both can't include the "equal to" boundary, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We'll have to arbitrarily pick one to be equal to and one to be strictly less/greater. Since greater than is the negation, that will be the strict (no equal to) operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI (if correct) says "The $geoWithin operator treats its border as inclusive" so I think it would be lte and gt.
dc2f504 to
924523e
Compare
175f3cd to
4264937
Compare
63e7346 to
0ba8409
Compare
82dacce to
5bc08fa
Compare
5bc08fa to
3d03b3c
Compare
| value = value[0] | ||
| if isinstance(value, Distance): | ||
| raise ValueError( | ||
| "Only numeric values of radian units are allowed on geodetic distance queries." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think saying "geodetic" isn't necessary because MongoDB doesn't support non-geodetic. Also, unlike the SQL implementation, this method isn't used for distance queries, rather it should say "on dwithin queries", I believe.
016b505 to
3059abc
Compare
PR docs: https://django-mongodb-backend--448.org.readthedocs.build/en/448/