Opened 3 weeks ago

Last modified 2 weeks 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 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 (10)

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

Owner: changed from nobody to Tomer Chachamu
Status: newassigned

comment:3 Changed 3 weeks 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 3 weeks 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 weeks ago by Tim Graham (previous) (diff)

comment:5 Changed 3 weeks 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 2 weeks ago by Tim Graham

#28767 may be related if not a duplicate.

comment:7 Changed 2 weeks ago by Tomer Chachamu

Owner: set to Tomer Chachamu
Status: newassigned

comment:8 Changed 2 weeks ago by Tomer Chachamu

Needs tests: unset
Owner: Tomer Chachamu deleted
Status: assignednew

comment:9 Changed 2 weeks ago by Tomer Chachamu

Needs tests: set
Owner: set to Tomer Chachamu
Status: newassigned

comment:10 Changed 2 weeks ago by Tomer Chachamu

Needs tests: unset
Owner: Tomer Chachamu deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top