Opened 6 months ago

Last modified 2 months ago

#28762 new Bug

Can't aggregate annotations with array parameters

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

Description

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/compiler.py", 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 (12)

comment:1 Changed 6 months 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 6 months ago by Tomer Chachamu

Owner: changed from nobody to Tomer Chachamu
Status: newassigned

comment:3 Changed 6 months 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 https://github.com/django/django/blob/master/django/db/models/expressions.py#L774 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 6 months 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 6 months ago by Tim Graham (previous) (diff)

comment:5 Changed 6 months 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 6 months ago by Tim Graham

#28767 may be related if not a duplicate.

comment:7 Changed 6 months ago by Tomer Chachamu

Owner: set to Tomer Chachamu
Status: newassigned

comment:10 Changed 6 months ago by Tomer Chachamu

Needs tests: unset
Status: assignednew

comment:11 Changed 5 months ago by Tim Martin

Triage Stage: AcceptedReady for checkin

comment:12 Changed 5 months ago by Tomer Chachamu

Cc: Tomer Chachamu added

comment:13 Changed 4 months 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 months 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).

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