INTPYTHON-835 Add support for GIS lookups #448
Conversation
| "GIS Union not supported.": { | ||
| "gis_tests.geoapp.tests.GeoLookupTest.test_gis_lookups_with_complex_expressions", | ||
| }, |
There was a problem hiding this comment.
Note to self that we should add @skipUnlessDBFeature("supports_Union_function") and send the patch upstream. I'll hold off in case any more similar changes are identified by this PR.
There was a problem hiding this comment.
There are a couple other places where tests not marked as requiring a feature use it. Maybe the authors didn't expect a backend to implement only the slice of operations that we do?
There was a problem hiding this comment.
Correct. The skipping is generally just best effort... what's needed for the built-in backends.
|
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. |
timgraham
left a comment
timgraham
left a comment
There was a problem hiding this comment.
The documentation to update is docs/ref/contrib/gis.rst.
timgraham
changed the title
timgraham
left a comment
timgraham
left a comment
There was a problem hiding this comment.
Did you forget the documentation updates? :-)
tests/gis_tests_/tests.py
Outdated
| self.assertEqual(qs.count(), 1) | ||
| self.assertEqual(houston, qs.first()) |
There was a problem hiding this comment.
prefer self.assertCountEqual(qs, [houston])
QuerySet.count() adds an extra query and makes a failure more difficult to debug since the first thing you need to do is run the test to find which extra objects appeared.
Also, in Django tests, the expected values usually appear as the second argument.
This comment was marked as resolved.
This comment was marked as resolved.
| def gis_lookup(self, compiler, connection, as_expr=False): # noqa: ARG001 | ||
| raise NotSupportedError(f"MongoDB does not support the {self.lookup_name} lookup.") | ||
|
|
||
| def gis_lookup(self, compiler, connection, as_expr=False): |
There was a problem hiding this comment.
Can we use the geo operator like $geoWithin and other inside an $expr?
As long as I know we can't, so we should raise an error if the as_expr is True I have fear that someone tries to wrap a gis_lookup inside a Case and the query fails
There was a problem hiding this comment.
Please add a test for the exception and document all limitations of these lookups (no subqueries, no expressions).
This comment was marked as resolved.
This comment was marked as resolved.
NoahStapp
requested review from
WaVEV and
timgraham
and removed request for
timgraham
| models.Union, | ||
| ) | ||
|
|
||
| operators = { |
There was a problem hiding this comment.
You can simply use:
| operators = { | |
| gis_operators = { |
It's a property on databases where some logic based on database version is needed to determine what operators are available.
tests/gis_tests_/tests.py
Outdated
| houston = City.objects.get(name="Houston") | ||
| dallas = City.objects.get(name="Dallas") # Roughly ~363 km from Houston | ||
| qs = City.objects.filter(point__distance_lte=(houston.point, 362826)) | ||
| self.assertCountEqual(list(qs), [houston, dallas]) |
There was a problem hiding this comment.
list() isn't needed with assertCountEqual()
tests/gis_tests_/tests.py
Outdated
| qs = City.objects.filter(point__dwithin=(houston.point, 0.2)) | ||
| self.assertEqual(qs.count(), 5) |
There was a problem hiding this comment.
Same comment about asserting the results and not using count applies to other tests. In this case, you could use City.objects.exclude() to produce fewer results.
tests/gis_tests_/tests.py
Outdated
| def test_within(self): | ||
| zipcode = Zipcode.objects.get(code="77002") | ||
| qs = City.objects.filter(point__within=zipcode.poly).values_list("name", flat=True) | ||
| self.assertEqual(list(qs), ["Houston"]) |
There was a problem hiding this comment.
self.assertCountEqual(qs, ["Houston"]) also works if you want be consistent with the other tests.
| return self.name, [] | ||
|
|
||
|
|
||
| class Within(Operator): |
There was a problem hiding this comment.
Please alphabetize the subclasses.
3 participants
No description provided.