Opened 7 years ago

Closed 5 years ago

#28762 closed Bug (fixed)

Can't aggregate annotations with array parameters

Reported by: Daniel Keller Owned by:
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Josh Smeaton, Tomer Chachamu Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


If I use a database function that takes an array parameter, for example a histogram:

import math
from django.db import models
from django.contrib.postgres.fields import ArrayField

class Log2Bucket(models.Func):
    function = 'width_bucket'

    def __init__(self, expression, a, b, **extra):
        max_exp = math.ceil(math.log2(b / a))
        buckets = [a * 2**exp for exp in range(max_exp + 1)]
        buckets = models.Value(buckets, output_field=ArrayField(models.FloatField())) ###
        super().__init__(expression, buckets,
            output_field=models.PositiveSmallIntegerField(), **extra)

and a simple model

class Foo(models.Model):
    bar = models.FloatField()

It works fine in queries like

Foo.objects.annotate(b=Log2Bucket('bar', 1, 100)).values('b')

but for

Foo.objects.annotate(b=Log2Bucket('bar', 1, 100)).values('b').annotate(count=models.Count('pk'))

I get

  File "/home/daniel_keller/shared/raumplus_2/env/lib/python3.4/site-packages/django/db/models/sql/", line 128, in get_group_by
    if (sql, tuple(params)) not in seen:
TypeError: unhashable type: 'list'

referring to the list passed to Value.

Honestly, I'm not sure if this counts as a bug or a new feature, since it's clearly unintended behavior, but none of Django's database functions need it to work.

I can work around it by replacing the marked line with

buckets = Value("{%s}" % ','.join(map(str, buckets))) ###

but this is IMO very ugly.

Change History (14)

comment:1 by Tim Graham, 7 years ago

Cc: Josh Smeaton added

I'm not sure what the proper resolution is either, however, I see the documentation for Value says, "When you need to represent the value of an integer, boolean, or string within an expression, you can wrap that value within a Value()." You're passing a list to Value which contradicts the documentation -- perhaps a more helpful message could be raised.

comment:2 by Tomer Chachamu, 7 years ago

Owner: changed from nobody to Tomer Chachamu
Status: newassigned

comment:3 by Josh Smeaton, 7 years ago

Specifically, Value can represent types that the database driver (such as psycopg2) has adapters for. But the bug you're hitting is that Value represents itself as a a SQL parameter, and is then hashed to see if it already exists.

Can you try ExpressionList and see if that works for your case? It might be worth documenting ExpressionList beside Value for these cases if it works.

comment:4 by Tomer Chachamu, 7 years ago

Has patch: set
Owner: Tomer Chachamu removed
Status: assignednew
Triage Stage: UnreviewedAccepted

It seems like we just need to make the parameters to cur.execute always be hashable. So in this case the list [1, 2, 4, 8, 16, 32, 64, 128] returned from get_db_prep_value. I've added a PR.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:5 by Tim Graham, 7 years ago

Needs tests: set
Type: UncategorizedBug

Tests are needed to demonstrate what's fixed.

Most likely the patch doesn't qualify for a backport to 1.11 based on our supported versions policy (it would have to be a regression from an older version of Django) so you can remove the release note in the PR.

comment:6 by Tim Graham, 7 years ago

#28767 may be related if not a duplicate.

comment:7 by Tomer Chachamu, 7 years ago

Owner: set to Tomer Chachamu
Status: newassigned

comment:10 by Tomer Chachamu, 7 years ago

Needs tests: unset
Status: assignednew

comment:11 by Tim Martin, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Tomer Chachamu, 7 years ago

Cc: Tomer Chachamu added

comment:13 by Tim Graham, 6 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I left comments for improvement on the PR.

comment:14 by Tim Graham, 6 years ago

#29139 is the same exception but a different cause involving aggregating JSONField with KeyTransform. The proposed patch doesn't fix that issue (not that is has to).

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In ceab25bc:

Refs #28762 -- Added test for aggregating over a function with ArrayField parameters.

Fixed in d87bd29c4f8dfcdf3f4a4eb8340e6770a2416fe3.

comment:16 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Resolution: fixed
Status: newclosed
Version: 1.112.2
Note: See TracTickets for help on using tickets.
Back to Top