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 , 7 years ago
Cc: | added |
---|
comment:2 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 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 , 7 years ago
Has patch: | set |
---|---|
Owner: | removed |
Status: | assigned → new |
Triage Stage: | Unreviewed → Accepted |
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.
comment:5 by , 7 years ago
Needs tests: | set |
---|---|
Type: | Uncategorized → Bug |
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:7 by , 7 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:10 by , 7 years ago
Needs tests: | unset |
---|---|
Status: | assigned → new |
comment:11 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:12 by , 7 years ago
Cc: | added |
---|
comment:13 by , 7 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I left comments for improvement on the PR.
comment:14 by , 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:16 by , 6 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Version: | 1.11 → 2.2 |
Fixed in d87bd29c4f8dfcdf3f4a4eb8340e6770a2416fe3.
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 aValue()
." You're passing alist
toValue
which contradicts the documentation -- perhaps a more helpful message could be raised.