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 Sergey Fedoseev, 9 years ago

Owner: changed from nobody to Sergey Fedoseev
Status: new → assigned

comment:2 by Sergey Fedoseev, 9 years ago

Has patch: set

PR -- ​https://github.com/django/django/pull/5614

comment:3 by Tim Graham, 9 years ago

Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Bug

comment:4 by Claude Paroz, 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 Sergey Fedoseev, 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 Claude Paroz, 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 Sergey Fedoseev, 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.

comment:8 by Tim Graham, 9 years ago

Cc: Josh Smeaton added

Josh, care to lend your expertise on the two approaches here?

comment:9 by Josh Smeaton, 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.

in reply to:  9 comment:10 by Sergey Fedoseev, 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.

comment:11 by Josh Smeaton, 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.

in reply to:  11 comment:12 by Sergey Fedoseev, 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]
Last edited 8 years ago by Sergey Fedoseev (previous) (diff)

comment:13 by Josh Smeaton, 9 years ago

Sorry, you're right. That was a relatively recent change to support case expressions and Value for .update().

comment:14 by Tim Graham, 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 Claude Paroz, 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 Tim Graham, 8 years ago

Triage Stage: Accepted → Ready for checkin

comment:17 by Tim Graham <timograham@…>, 8 years ago

Resolution: → fixed
Status: assigned → closed

In f909fa8:

Fixed #25708 -- Fixed annotations with geometry values.

Note: See TracTickets for help on using tickets.
Back to Top