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')
Version 0, edited 9 years ago by zauddelig (next)

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 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.

Last edited 9 years ago by Josh Smeaton (previous) (diff)

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