[Django] #27448: GIS distance queries use deprecated ST_distance_sphere
Django
-------------------------------------+-------------------------------------
Reporter: Christian | Owner: nobody
von Roques |
Type: Bug | Status: new
Component: GIS | Version: master
Severity: Normal | Keywords: GIS distance
Triage Stage: | deprecated ST_distance_sphere
Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When using `django.contrib.gis` with PostGIS 2.3.0, distance queries will
create SQL using the `ST_distance_sphere` function, which is deprecated
since PostGIS 2.2.0 (see http://trac.osgeo.org/postgis/ticket/2748 ). The
old function still works, but now issues a deprecation-warning. The
problem is that `ST_distance_sphere` has become more than 40× slower than
its replacement `ST_DistanceSphere` due to the added deprecation warning.
My `city_cities` table has 143260 rows. The following query executes in
about 7.5 seconds:
{{{#!sql
SELECT min(ST_distance_sphere(ST_Point(0,0), location)) FROM cities_city;
}}}
Whereas the same query with `ST_DistanceSphere` instead of
`ST_distance_sphere` completes in less than 170ms.
I've prepared a patch to stop Django from using the deprecated functions.
https://github.com/roques/django/tree/postgis22_deprecated The problem
with this patch is that the non-deprecated functions only became available
with PostGIS 2.2.0 and thus the patch would raise the minimum required
PostGIS version from 2.1.0.
As I can see the situation, we have a choice between the following three
scenarios:
1. Raise the required version of PostGIS from 2.1.0 to 2.2.0 and use the
non-deprecated functions.
2. Keep using the deprecated functions and live with all distance queries
being often unusably slow.
3. Complicate the code to detect the PostGIS version and use
`ST_DistanceSphere` when available or `ST_distance_sphere` otherwise.
I would prefer option 1, but people stuck with PostGIS <2.2.0 would then
be unable to make distance queries with newer releases of Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Roques |
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I think we should favor the third option. It's already a pattern we use
for the MySQL GIS function names and it doesn't really make the code more
complex.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:1>
Django
-------------------------------------+-------------------------------------
Roques |
Type: | Status: assigned
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => mjumbewu
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:2>
Django
-------------------------------------+-------------------------------------
Roques |
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Definitely option 3. We won't raise the minimum PostGIS version in Django
1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:3>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mjumbe Poe):
I've started adding some tests at
https://github.com/mjumbewu/django/tree/postgis22_deprecated. I tried
handling the pre/post PostGIS 2.2 condition in there as well. It's not the
most elegant solution, and it's not easy to test, since the PostGIS
version is cached after it's first calculated (so I can't just
{{{override_settings}}}). I had to test it by just running the tests twice
against different postgis versions (which is why there are test in there
commented out). Can anyone else think of better tests for this?
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:4>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
In `PostGISDistanceOperator`, you should be able to check
`connection.ops.spatial_version`. Then for testing, you should be able to
mock `spatial_version` with something like:
{{{
with
mock.patch('django.contrib.gis.db.backends.postgis.operations.PostGISOperations.spatial_version',
new_callable=mock.PropertyMock) as mock_spatial_version:
mock_spatial_version.return_value = '2.1.0'
... your test
}}}
I wouldn't care about fixing the deprecated GeoQuerySet API (where mocking
would be more difficult).
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:5>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Christian von Roques):
Now that I spent some more time with the code, option 3 was not that hard
to implement.
More tests are still needed. However, mocking `spatial_version` won't do,
as my changes only call it once in `PostGISOperations.__init__`. However,
changing `settings.POSTGIS_VERSION` before instantiating
`PostGISOperations` should work.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:6>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:7>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Christian von Roques):
I've added a test to the test-suite to check the PostGIS version dependent
behaviour.
However, my first concise implementation evaluates `spatial_version` in
`PostGISOperations.__init__`, which in turn evaluates an SQL-query, which
confuses quite a few of Django's tests (e.g.
`backends.tests.PostgreSQLTests.test_nodb_connection`). To appease the
test-suite I modified my implementation to defer evaluation of
`spatial_version` using `@cached_property`, which works, but is quite a
bit larger. — My branch
https://github.com/roques/django/tree/postgis22_deprecated contains both,
the nice and the deferred/lazy implementation.
What is the policy for cases like this?
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:8>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Could you please first fix the test which is still using the deprecated
syntax. That is, move the test in `DistanceFunctionsTests` and replace:
`AustraliaCity.objects.exclude(id=hillsdale.id).distance(hillsdale.point,
spheroid=True)`
by
`AustraliaCity.objects.exclude(id=hillsdale.id).annotate(distance=Distance('point',
hillsdale.point, spheroid=True))`
The fix might mostly (exclusively?) concern
`django/contrib/gis/db/models/functions.py`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:9>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Christian von Roques):
I've moved and adapted @mjumbewu's testcase as indicated.
Will it be OK to squash his changeset with mine in the end, losing
attribution for his work?
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:10>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Yes, the usual procedure is to write "Thanks <name> for the initial
patch." in the commit message.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:11>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
And then please make your branch a pull request so as we can comment on
it.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:12>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Christian von Roques):
[https://github.com/django/django/pull/7539 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:13>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Thanks! As stated on the PR and in comment:5, I don't think we should
update the deprecated `GeoQuerySet`-related names. Let's concentrate on
database function names (as in
https://docs.djangoproject.com/en/1.10/ref/contrib/gis/functions/).
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:14>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Accepted
deprecated ST_distance_sphere |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Christian von Roques):
I've reverted all fixes related to `GeoQuerySet`.
Please take a look at the code, as I'm unsure about some of the stylistic
choices I made.
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:15>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: GIS distance | Triage Stage: Ready for
deprecated ST_distance_sphere | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:16>
Django
-------------------------------------+-------------------------------------
Roques | Poe
Cleanup/optimization |
Component: GIS | Version: master
Severity: Normal | Resolution: fixed
Keywords: GIS distance | Triage Stage: Ready for
deprecated ST_distance_sphere | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cbae4d31847d75d889815bfe7c04af035f45e28d" cbae4d31]:
{{{
#!CommitTicketReference repository=""
revision="cbae4d31847d75d889815bfe7c04af035f45e28d"
Fixed #27448 -- Switched use of functions deprecated in PostGIS 2.2.
Thanks Claude Paroz and Tim Graham for reviews, and
Mjumbe Wawatu Poe for the initial regression test.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27448#comment:17>