[Django] #25307: Cannot use .annotate with conditional expressions
Django
----------------------------------------------+--------------------
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
I am trying to create a query that first annotates a `Count`, then
aggregates some `Sum`s together using `Case` objects. If I do the query
without the annotate, everything is fine. If I do the query without any
`Case` objects in the aggregate, everything is fine. But when they are
both present I get this error:
{{{
'WhereNode' object has no attribute 'get_source_expressions'
}}}
My query is something like this:
{{{
counts = models.Package.objects.annotate(
used_count=Count('projects')
).distinct().aggregate(
no_parent=Sum(Case(
When(parent_id__isnull=True, then=1),
default=0,
output_field=IntegerField()
))
)
}}}
I would like to eventually add the `used_count` value in the aggregate,
but this itself won't even run.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Any chance you could provide a regression test for Django's test suite?
It's difficult to reproduce the issue without your models.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:1>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by martsberger):
I have the same issue, here is enough code to reproduce the issue.
{{{
# In models.py
from django.db import models
class DateModel(models.Model):
date1 = models.DateTimeField()
date2 = models.DateTimeField()
# elsewhere
from django.db.models import Q, Case, When
from django.db.models.functions import Coalesce
from datetime import date
annotate = {'date': Coalesce('date1', 'date2')}
dms = DateModel.objects.annotate(**annotation)
aggregation = {
'count_recent': Count(Case(When(Q(date__gte=date(year=2015, month=1,
day=1)))))
}
dms = dms.aggregate(**aggregation)
}}}
AttributeError: 'WhereNode' object has no attribute
'get_source_expressions'
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:2>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jarshwah):
* cc: josh.smeaton@… (added)
* version: 1.8 => master
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:3>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
Looks related to #25316
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:4>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freshquiz):
Please correct me if I'm wrong, but it seems as if the issue lies with the
`django.db.models.expressions.When` class, in that the `condition`
attribute is supposed to be a `django.db.models.Q` object and yet the
`get_source_epxressions()` and `set_source_expressions()` methods are
treating the `condition` attribute as an
`django.db.models.expressions.Expression`, when it most certainly is not.
This violates the API and hence client-code of the API (such as
aggregation code in this case) will fail unexpectedly.
E.g.
In the example provided by @martsberger, the `count_recent` expression
gets down to `django.db.models.sql.query.Query.rewrite_cols()`. That
method recursively travels into the nested expressions until it hits the
`Q` object returned by the `When` expression's `get_source_expressions()`
and that's where it fails.
The problem is only triggered (during aggregation) by column rewriting,
when this line evaluates to `True` in
`django.db.models.sql.query.Query.get_aggregation()`:
{{{
if (isinstance(self.group_by, list) or has_limit or
has_existing_annotations or self.distinct):
}}}
As I said initially, I believe the root cause is the violation of the
`Expression` API by the `When` subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:5>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by freshquiz):
* cc: matt@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:6>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freshquiz):
I'm not sure when/how, but the `condition` attribute of the `When`
`Expression` gets converted to a `django.db.models.sql.where.WhereNode`,
so it's actually a `WhereNode` object that is having
`.get_source_expressions()` called on it.
Regardless, `django.db.models.expressions.When`'s
`get_source_expressions()` and `set_source_expressions()` methods need to
bridge the gap between `Expression`s and `django.utils.tree.Node`s, in
order to properly re-write the column names to the aliases from the
subquery.
I can't see a quick/easy way to do this, but as I'm not experienced in the
ORM code, I could be wrong.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:7>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
We have a couple of things that *resolve* to an expression. For example,
Q() and F() objects are not expressions, but will resolve to an expression
when added to a query.
If WhereNode doesn't respect the expression API, then the fix is to make
WhereNodes full expressions.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:8>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freshquiz):
To clarify, I think it's the `django.db.models.expressions.When` class
(which is a subclass of `Expression`) that is not respecting the API, with
regards to its `get_source_expressions()` and `set_source_expressions()`
methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:9>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
Comment:
Also just hit this when upgrading an old project using
[https://github.com/henriquebastos/django-aggregate-if django-aggregate-
if] on Django 1.7 to conditional aggregation on Django 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:10>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
From a short investigation it looks like the issue is actually caused by a
`distinct()` and `aggregate()` of conditional expression combination.
Can the people affected confirm that removing the `distinct()` call from
their query silence the exception?
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:11>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jproffitt):
That does make it break as well, but not simply just the use of distinct.
If you use EITHER `.distinct()` OR `.annotate()` along with an
`.aggregate()` that contains a conditional expression. (not sure exactly
what part of the condition expression, but it must be either because of
the `Case()` or the `When()`.
I have made a completely useless example using the polls app in the django
tutorial. I copied the polls models exactly, and created this query, which
will cause the error to happen:
{{{
counts = models.Choice.objects.annotate(
used_count=Count('question')
).distinct().aggregate(
yes=Sum(Case(
When(choice_text='yes', then=F('votes')),
default=0,
output_field=IntegerField()
)),
no=Sum(Case(
When(choice_text='no', then=F('votes')),
default=0,
output_field=IntegerField()
)),
)
}}}
(Obviously, then `used_count` will always be 1, and its pretty pointless
in this example, but you get the idea).
So if you remove the either the `.annotate()` OR the `.distinct()`, the
query still breaks the same way. The presence of either one causes it to
break. But if you remove BOTH, the query runs fine.
I do not know anything about the internals of the Query api, or I would
help fix the problem. But it seems there is a problem when combining
certain queryset methods like `annotate` or `distinct` with `aggregate`s
that have condition expressions.
I know it has to do with condition expressions, because doing the query
like the following works just fine:
{{{
counts = models.Choice.objects.annotate(
used_count=Count('question')
).distinct().aggregate(
yes=Sum('votes'),
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:12>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/6266
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:13>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by newportt):
* cc: newport.travis@… (added)
Comment:
As mentioned on the pull request, this issue seems to exist for any of the
4 cases
[https://github.com/django/django/blob/master/django/db/models/sql/query.py#L400-L401
here]:
{{{
if (isinstance(self.group_by, list) or has_limit or
has_existing_annotations or
self.distinct):
}}}
I was able to write unit tests for all but the
{{{isinstance(self.group_by, list)}}} case, if someone is able to provide
a test for that case I can add it to my changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:14>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
There's a failing test on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:15>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:16>
Django
Reporter: jproffitt | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Two test failures for Oracle remain.
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:17>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Josh Smeaton):
* stage: Accepted => Ready for checkin
Comment:
https://github.com/django/django/pull/7849
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:18>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1df89a60c5b7a28d7fda4c9ba7c07f02fd7de0fa" 1df89a60]:
{{{
#!CommitTicketReference repository=""
revision="1df89a60c5b7a28d7fda4c9ba7c07f02fd7de0fa"
Fixed #25307 -- Fixed QuerySet.annotate() crash with conditional
expressions.
Thanks Travis Newport for the tests and Josh Smeaton for contributing
to the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:19>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: closed
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d" b181cae2]:
{{{
#!CommitTicketReference repository=""
revision="b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d"
Refs #25307 -- Replaced SQLQuery.rewrite_cols() by replace_expressions().
The latter offers a more generic interface that doesn't require
specialized expression types handling.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:20>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"7530cf3900ab98104edcde69e8a2a415e82b345a" 7530cf39]:
{{{
#!CommitTicketReference repository=""
revision="7530cf3900ab98104edcde69e8a2a415e82b345a"
Fixed #34975 -- Fixed crash of conditional aggregate() over aggregations.
Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.
Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.
Refs #25307.
Thanks Sergey Nesterenko for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:21>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"49f1ced86398177c125e256d7e812eb34fae672e" 49f1ced8]:
{{{
#!CommitTicketReference repository=""
revision="49f1ced86398177c125e256d7e812eb34fae672e"
[5.0.x] Fixed #34975 -- Fixed crash of conditional aggregate() over
aggregations.
Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.
Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.
Refs #25307.
Thanks Sergey Nesterenko for the report.
Backport of 7530cf3900ab98104edcde69e8a2a415e82b345a from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:22>
Django
Reporter: Jared Proffitt | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"acf4cee95144c55a12492cdd71fa795d7accfe26" acf4cee]:
{{{
#!CommitTicketReference repository=""
revision="acf4cee95144c55a12492cdd71fa795d7accfe26"
[4.2.x] Fixed #34975 -- Fixed crash of conditional aggregate() over
aggregations.
Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.
Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.
Refs #25307.
Thanks Sergey Nesterenko for the report.
Backport of 7530cf3900ab98104edcde69e8a2a415e82b345a from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25307#comment:23>