Opened 3 years ago

Closed 15 months 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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tomer Chachamu

Owner: changed from nobody to Tomer Chachamu
Status: newassigned

comment:3 Changed 3 years ago by Josh Smeaton

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 Changed 3 years ago by Tomer Chachamu

Has patch: set
Owner: Tomer Chachamu deleted
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 3 years ago by Tim Graham (previous) (diff)

comment:5 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham

#28767 may be related if not a duplicate.

comment:7 Changed 3 years ago by Tomer Chachamu

Owner: set to Tomer Chachamu
Status: newassigned

comment:10 Changed 3 years ago by Tomer Chachamu

Needs tests: unset
Status: assignednew

comment:11 Changed 3 years ago by Tim Martin

Triage Stage: AcceptedReady for checkin

comment:12 Changed 3 years ago by Tomer Chachamu

Cc: Tomer Chachamu added

comment:13 Changed 3 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I left comments for improvement on the PR.

comment:14 Changed 2 years ago by Tim Graham

#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 Changed 15 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In ceab25bc:

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

Fixed in d87bd29c4f8dfcdf3f4a4eb8340e6770a2416fe3.

comment:16 Changed 15 months ago by felixxm

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