Opened 7 years ago

Closed 6 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

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

Version 0, edited 7 years ago by Tomer Chachamu (next)

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, 7 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I left comments for improvement on the PR.

comment:14 by Tim Graham, 7 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@…>, 6 years ago

In ceab25bc:

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

Fixed in d87bd29c4f8dfcdf3f4a4eb8340e6770a2416fe3.

comment:16 by Mariusz Felisiak, 6 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