[Django] #25708: cannot annotate with geometry value
Django
-------------------------------+--------------------
Reporter: sir-sigurd | Owner: nobody
Type: Uncategorized | Status: new
Component: GIS | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
{{{
In [4]: Test.objects.annotate(p=Value(Point(1,1),
output_field=GeometryField()))
Out[4]:
---------------------------------------------------------------------------
ProgrammingError Traceback (most recent call
last)
/home/sergey/tmp/django-venv/local/lib/python2.7/site-
packages/IPython/core/formatters.pyc in __call__(self, obj)
693 type_pprinters=self.type_printers,
694 deferred_pprinters=self.deferred_printers)
--> 695 printer.pretty(obj)
696 printer.flush()
697 return stream.getvalue()
/home/sergey/tmp/django-venv/local/lib/python2.7/site-
packages/IPython/lib/pretty.pyc in pretty(self, obj)
399 if callable(meth):
400 return meth(obj, self, cycle)
--> 401 return _default_pprint(obj, self, cycle)
402 finally:
403 self.end_group()
/home/sergey/tmp/django-venv/local/lib/python2.7/site-
packages/IPython/lib/pretty.pyc in _default_pprint(obj, p, cycle)
519 if _safe_getattr(klass, '__repr__', None) not in
_baseclass_reprs:
520 # A user-provided repr. Find newlines and replace them
with p.break_()
--> 521 _repr_pprint(obj, p, cycle)
522 return
523 p.begin_group(1, '<')
/home/sergey/tmp/django-venv/local/lib/python2.7/site-
packages/IPython/lib/pretty.pyc in _repr_pprint(obj, p, cycle)
701 """A pprint that just redirects to the normal repr
function."""
702 # Find newlines and replace them with p.break_()
--> 703 output = repr(obj)
704 for idx,output_line in enumerate(output.splitlines()):
705 if idx:
/home/sergey/dev/django/django/db/models/query.pyc in __repr__(self)
232
233 def __repr__(self):
--> 234 data = list(self[:REPR_OUTPUT_SIZE + 1])
235 if len(data) > REPR_OUTPUT_SIZE:
236 data[-1] = "...(remaining elements truncated)..."
/home/sergey/dev/django/django/db/models/query.pyc in __iter__(self)
256 - Responsible for turning the rows into model
objects.
257 """
--> 258 self._fetch_all()
259 return iter(self._result_cache)
260
/home/sergey/dev/django/django/db/models/query.pyc in _fetch_all(self)
1072 def _fetch_all(self):
1073 if self._result_cache is None:
-> 1074 self._result_cache = list(self.iterator())
1075 if self._prefetch_related_lookups and not
self._prefetch_done:
1076 self._prefetch_related_objects()
/home/sergey/dev/django/django/db/models/query.pyc in __iter__(self)
50 # Execute the query. This will also fill compiler.select,
klass_info,
51 # and annotations.
---> 52 results = compiler.execute_sql()
53 select, klass_info, annotation_col_map = (compiler.select,
compiler.klass_info,
54
compiler.annotation_col_map)
/home/sergey/dev/django/django/db/models/sql/compiler.pyc in
execute_sql(self, result_type)
837 cursor = self.connection.cursor()
838 try:
--> 839 cursor.execute(sql, params)
840 except Exception:
841 cursor.close()
/home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql,
params)
77 start = time()
78 try:
---> 79 return super(CursorDebugWrapper, self).execute(sql,
params)
80 finally:
81 stop = time()
/home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql,
params)
62 return self.cursor.execute(sql)
63 else:
---> 64 return self.cursor.execute(sql, params)
65
66 def executemany(self, sql, param_list):
/home/sergey/dev/django/django/db/utils.pyc in __exit__(self, exc_type,
exc_value, traceback)
90 if dj_exc_type not in (DataError, IntegrityError):
91 self.wrapper.errors_occurred = True
---> 92 six.reraise(dj_exc_type, dj_exc_value, traceback)
93
94 def __call__(self, func):
/home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql,
params)
62 return self.cursor.execute(sql)
63 else:
---> 64 return self.cursor.execute(sql, params)
65
66 def executemany(self, sql, param_list):
ProgrammingError: can't adapt type 'Point'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25708>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Uncategorized | Status: assigned
Component: GIS | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sir-sigurd):
* status: new => assigned
* needs_better_patch: => 0
* owner: nobody => sir-sigurd
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:1>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Uncategorized | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sir-sigurd):
* has_patch: 0 => 1
Comment:
PR -- https://github.com/django/django/pull/5614
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:2>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Component: GIS | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:3>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I have created an alternate pull request:
https://github.com/django/django/pull/5666
About the second test case of the origin pull request, I wouldn't include
it as I think the use case is already covered by the Transform function.
What do you think about that approach?
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:4>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sir-sigurd):
I'm against of having user to wrap geometries with `GeomValue` because of
[https://wiki.python.org/moin/TOOWTDI TOOWTDI] principle and this approach
gives the second way of wrapping values and the first is still broken.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:5>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
With your approach, you are also forced to specify an output_field for
geometry values, which is not very intuitive either. So the difference is
basically recommending either `Value(geom, GeometryField(srid=...))` or
`GeomValue(geom)`. Of course, if we find a solution to make `Value(geom)`
just work, I'd favored that solution.
Otherwise, we'll simply have to ask other devs about their preferences.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:6>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sir-sigurd):
One does not simply annotate without specifying `output_field`:
{{{
Test.objects.annotate(a=Value(1))
}}}
raises
{{{
FieldError: Cannot resolve expression type, unknown output_field
}}}
, so I see no reason why geometry values should be handled differently.
There is another difference in our approaches: I'm trying to reuse the
code for preparing SQL that existed before `GeomValue` was added, because
I believe that `GeometryField` and DB backends are sufficient for this and
there is no need for another entity.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:7>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: jarshwah (added)
Comment:
Josh, care to lend your expertise on the two approaches here?
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:8>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
This is really a matter of taste.
I personally like the look of GeomValue better, and DurationValue sets
precedence for specialised `Value` subclasses, even though it's an
internal implementation detail.
Maybe take it to the mailing list and see if the gis community has an
opinion.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:9>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sir-sigurd):
Replying to [comment:9 jarshwah]:
I don't quite understand your answer.
Are you speaking about using `GeomValue` internally only or providing it
to users?
Is `Value(Point(1,1), output_field=GeometryField())` valid? I can't get a
definite answer from the
[https://docs.djangoproject.com/en/1.9/ref/models/expressions/#value-
expressions docs], but IMO if `SomeField()` accepts `some_value` then
`Value(some_value, output_field=SomeField())` should be valid.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:10>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
I'm suggesting that `GeomValue` could be made public.
> but IMO if SomeField() accepts some_value then Value(some_value,
output_field=SomeField()) should be valid.
Not so. The first argument to `Value` needs to be convertible by the
backend driver to a param. `Value(Point(1, 1), output_field=X)` generates
`"%s", (Point(1, 1), )`. The `output_field` is only really responsible for
converting the value returned from the database into a useable model
field, it doesn't take responsibility for serialising the value to be sent
to the database. It *could* be used for serialisation but it doesn't for
`Value`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:11>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sir-sigurd):
Replying to [comment:11 jarshwah]:
> The `output_field` is only really responsible for converting the value
returned from the database into a useable model field, it doesn't take
responsibility for serialising the value to be sent to the database.
This doesn't seem to be true, `output_field.get_db_prev_value()` is used
in `Value.as_sql()`
[https://github.com/django/django/blob/93452a70e8a62c7408eeded444f5088d4a26212d/django/db/models/expressions.py#L573
(GitHub)]:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
def as_sql(self, compiler, connection):
connection.ops.check_expression_support(self)
val = self.value
# check _output_field to avoid triggering an exception
if self._output_field is not None:
if self.for_save:
val = self.output_field.get_db_prep_save(val,
connection=connection)
else:
val = self.output_field.get_db_prep_value(val,
connection=connection)
if val is None:
# cx_Oracle does not always convert None to the appropriate
# NULL type (like in case expressions using numbers), so we
# use a literal SQL NULL
return 'NULL', []
return '%s', [val]
}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:12>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
Sorry, you're right. That was a relatively recent change to support case
expressions and Value for .update().
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:13>
Django
Reporter: sir-sigurd | Owner: sir-sigurd
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
After looking at this a bit closer, I see good arguments for both
solutions. For lack of a better indicator, I'll mark "patch needs
improvement" until there's a discussion to find consensus on how to move
this forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:14>
Django
Reporter: Sergey Fedoseev | Owner: Sergey
| Fedoseev
Type: Bug | Status: assigned
Component: GIS | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* needs_better_patch: 1 => 0
Comment:
To unblock the unfortunate current situation, I'd suggest to go with
Sergey's solution, as my suggested alternate approach is obviously not
superior.
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:15>
Django
Reporter: Sergey Fedoseev | Owner: Sergey
| Fedoseev
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:16>
Django
Reporter: Sergey Fedoseev | Owner: Sergey
| Fedoseev
Component: GIS | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f909fa84bedb51778a175aadfe4cfe7a91fe06cd" f909fa8]:
{{{
#!CommitTicketReference repository=""
revision="f909fa84bedb51778a175aadfe4cfe7a91fe06cd"
Fixed #25708 -- Fixed annotations with geometry values.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25708#comment:17>