Opened 9 years ago
Closed 8 years ago
#25708 closed Bug (fixed)
cannot annotate with geometry value
Reported by: | Sergey Fedoseev | Owned by: | Sergey Fedoseev |
---|---|---|---|
Component: | GIS | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Josh Smeaton | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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'
Change History (17)
comment:1 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 9 years ago
Has patch: | set |
---|
comment:3 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:4 by , 9 years ago
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?
comment:5 by , 9 years ago
I'm against of having user to wrap geometries with GeomValue
because of TOOWTDI principle and this approach gives the second way of wrapping values and the first is still broken.
comment:6 by , 9 years ago
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.
comment:7 by , 9 years ago
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.
follow-up: 10 comment:9 by , 9 years ago
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.
comment:10 by , 9 years ago
Replying to 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 docs, but IMO if SomeField()
accepts some_value
then Value(some_value, output_field=SomeField())
should be valid.
follow-up: 12 comment:11 by , 9 years ago
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
.
comment:12 by , 9 years ago
Replying to 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_prep_value()
is used in Value.as_sql()
(GitHub):
Code highlighting:
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]
comment:13 by , 9 years ago
Sorry, you're right. That was a relatively recent change to support case expressions and Value for .update().
comment:14 by , 9 years ago
Patch needs improvement: | set |
---|
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.
comment:15 by , 8 years ago
Patch needs improvement: | unset |
---|
To unblock the unfortunate current situation, I'd suggest to go with Sergey's solution, as my suggested alternate approach is obviously not superior.
comment:16 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR -- https://github.com/django/django/pull/5614