Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#25171 closed Bug (duplicate)

Can't update queryset after Count annotation

Reported by: Fraser Harris Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords: queryset, update, annotate, count
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Fraser Harris)

A Queryset that has been annotated with a Count of a ManyToMany field can not be update'd.

Example:

>>> Category.objects.annotate(Count('tiles')).update(name='foo')
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/models/query.py", line 490, in update
    rows = query.get_compiler(self.db).execute_sql(None)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 976, in execute_sql
    cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 782, in execute_sql
    cursor.execute(sql, params)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/backends/util.py", line 69, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/backends/util.py", line 53, in execute
    return self.cursor.execute(sql, params)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/utils.py", line 99, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/opt/secondfunnel/venv/local/lib/python2.7/site-packages/django/db/backends/util.py", line 53, in execute
    return self.cursor.execute(sql, params)
ProgrammingError: subquery has too many columns
LINE 1: ..." SET "name" = 'foo' WHERE "assets_category"."id" IN (SELECT...
                                                             ^

Models:

class Category(BaseModel):
    tiles = models.ManyToManyField(Tile, related_name='categories')
    name = models.CharField(max_length=255)

class Tile(Model):
    pass

SQL Query from django.db.connections.queries:

UPDATE "assets_category" SET "name" = \'foo\' WHERE "assets_category"."id" IN (SELECT U0."id", COUNT(U2."tile_id") AS "tiles__count" FROM "assets_category" U0 LEFT OUTER JOIN "assets_category_tiles" U2 ON ( U0."id" = U2."category_id" ) GROUP BY U0."id", U0."name")

Change History (10)

comment:1 by Fraser Harris, 9 years ago

Description: modified (diff)
Keywords: annotate count added; filter removed

comment:2 by Tim Graham, 9 years ago

I guess trying to update after annotations or aggregations probably isn't designed to work. Should we throw a friendlier error message?

in reply to:  2 comment:3 by Carl Meyer, 9 years ago

Triage Stage: UnreviewedAccepted

Replying to timgraham:

I guess trying to update after annotations or aggregations probably isn't designed to work. Should we throw a friendlier error message?

Well, certainly not after a terminal aggregation. But I don't see any a priori reason to assume that if you have an annotated QuerySet you shouldn't be able to update it. ISTM this should be accepted as a bug, and we should only resort to "friendlier error message" if somebody looks at it closely and reports more authoritatively that it's not feasible to support.

comment:4 by Shai Berger, 9 years ago

For the query given in the report:

Category.objects.annotate(Count('tiles')).update(name='foo')

I would like to see a warning (if not an error) because the annotation is "dead code" and probably a bug.
However, I'd expect this to work (assuming an additional numeric field "score" on the Category model)

Category.objects.annotate(n_tiles=Count('tiles')).update(score=F('score')+F('n_tiles'))

in reply to:  4 comment:5 by Carl Meyer, 9 years ago

Replying to shaib:

For the query given in the report:

Category.objects.annotate(Count('tiles')).update(name='foo')

I would like to see a warning (if not an error) because the annotation is "dead code" and probably a bug.

Sure, but unless that falls out naturally in an implementation that supports the useful cases, I think it's firmly in nice-to-have territory. An implementation that let this pass silently wouldn't bother or surprise me at all.

comment:6 by zauddelig, 9 years ago

This would be legit

categories = Category.objects.annotate(Count('tiles'))
#  some code
categories.update(name='foo')

Another thing that fails is this:

Category.objects.all().annotate(tiles_count=Count("tiles")).filter(tiles_count__gt=0).update(used=True)

Anyway the problem is due to the fact that it tries to use Category.id although it seems to be out of scope.
The problem seem to be with the SQLUpdateCompiler or somewhere in the Query object, I searched but I know very little about this field.

Last edited 9 years ago by zauddelig (previous) (diff)

comment:7 by Josh Smeaton, 9 years ago

The query that would work in the above situation (aggregate, filter, update) would be roughly:

update category
set used=true
where id in (
    select X.id
    from (select id, COUNT(tiles_id) tile_count from category group by id) X
    where X.tile_count >= 0
)

-- OR preferably?

update category
set used=true
where id in (
    select id tile_count from category group by id
    having COUNT(tiles_id) > 0
)

Which I think can be generated with roughly the following ORM code (not tested):

inner = Category.objects.all().annotate(tiles_count=Count("tiles")).filter(tiles_count__gt=0)
# maybe inner = inner.values('pk') ?
Category.objects.filter(pk__in=inner).update(used=True)

It might be possible to get SQLUpdateCompiler to detect annotations, and then create a new queryset, passing the existing queryset as an argument to pk__in=previous. Or teach it to drop anything from the subquerys annotations_select but ensure it stills GROUPs and HAVINGs.

Version 0, edited 9 years ago by Josh Smeaton (next)

comment:8 by zauddelig, 9 years ago

If you take out the annotation part then you wouldn't be able to use annotated values in filters nor in F functions.

comment:9 by Tim Graham, 9 years ago

Is this a duplicate of or related to #19513? It describes a failure of update() after annotate(val=Sum(...)).

comment:10 by Josh Smeaton, 9 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #19513 as Tim pointed out.

Last edited 7 years ago by Tim Graham (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top