#34975 closed Bug (fixed)
aggregate() crashes when referencing existing aggregations or window expressions through conditional expressions
Reported by: | Sergey Nesterenko | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Release blocker | Keywords: | QuerySet, Window, Aggregate, F |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I found out that getting refs in QuerySet doesn't work with models.Window when I try to use aggregate method. It happened when I tried to update Django to 4.2.7.
For example:
MyModel.objects.annotate( new_ordering=Window(RowNumber(), order_by=[F('ordering')]) ).aggregate( new_count=Count('id', filter=Q(ordering=F('new_ordering'))) )
And as a result, it fails with:
Traceback (most recent call last): File "/main.py", line 38, in <module> result = ordering_query.aggregate( File "/python3.10/site-packages/django/db/models/query.py", line 592, in aggregate return self.query.chain().get_aggregation(self.db, kwargs) File "/python3.10/site-packages/django/db/models/sql/query.py", line 398, in get_aggregation aggregate = aggregate_expr.resolve_expression( File "/python3.10/site-packages/django/db/models/aggregates.py", line 70, in resolve_expression for ref in c.get_refs(): File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs refs |= expr.get_refs() File "/python3.10/site-packages/django/db/models/sql/where.py", line 233, in get_refs refs |= child.get_refs() File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs refs |= expr.get_refs() File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs refs |= expr.get_refs() AttributeError: 'NoneType' object has no attribute 'get_refs'
It happens because of Window.get_source_expressions return:
def get_source_expressions(self): return [self.source_expression, self.partition_by, self.order_by, self.frame]
and if we don't specify 'frame' (for example) it will be None in the list.
And in models.aggregates.Aggregate.resolve_expression when we try to find all refs https://github.com/django/django/blob/47f9b8dca16b1fdc054b338d81eb080eabad0edf/django/db/models/aggregates.py#L70C30-L70C30 it fails because of we have None in Window.get_source_expressions
def get_refs(self): refs = set() for expr in self.get_source_expressions(): refs |= expr.get_refs() return refs
Change History (17)
comment:1 by , 13 months ago
Description: | modified (diff) |
---|
comment:2 by , 13 months ago
Summary: | Getting refs is not work properly with models.Window and aggregation → Getting refs does not work properly with models.Window and aggregation |
---|
comment:3 by , 13 months ago
Triage Stage: | Unreviewed → Accepted |
---|
follow-up: 5 comment:4 by , 13 months ago
I'm not sure if something else will break as I don't think we have extensive testing for performing filtered aggregation over a window function but does the following patch helps
diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index 3a0c75ebf2..74ae9cab8e 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -417,6 +417,8 @@ def replace_expressions(self, replacements): def get_refs(self): refs = set() for expr in self.get_source_expressions(): + if expr is None: + continue refs |= expr.get_refs() return refs
comment:5 by , 13 months ago
Replying to Simon Charette:
I'm not sure if something else will break as I don't think we have extensive testing for performing filtered aggregation over a window function but does the following patch helps
diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index 3a0c75ebf2..74ae9cab8e 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -417,6 +417,8 @@ def replace_expressions(self, replacements): def get_refs(self): refs = set() for expr in self.get_source_expressions(): + if expr is None: + continue refs |= expr.get_refs() return refs
I thought about it, but I wasn't sure it wouldn't break anything, but it might work.
comment:6 by , 13 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 11 comment:7 by , 13 months ago
I've got an idea of how to solve this, it's not clear to me if it's a regression as the underlying cause has little to do with window functions.
In the mean time, can you confirm that switching the order of your lookup resolve the issue. That is doing filter=Q(new_ordering=F('ordering'))
instead of filter=Q(ordering=F('new_ordering'))
.
comment:8 by , 13 months ago
Severity: | Normal → Release blocker |
---|
Confirmed regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d, refs #25307.
comment:9 by , 13 months ago
Has patch: | set |
---|
comment:10 by , 13 months ago
Summary: | Getting refs does not work properly with models.Window and aggregation → aggregate() crashes when referencing existing aggregations or window expressions through conditional expressions |
---|
comment:11 by , 13 months ago
Replying to Simon Charette:
filter=Q(new_ordering=F('ordering'))
instead offilter=Q(ordering=F('new_ordering'))
.
Yes, it solves my problem. Thankx!
comment:12 by , 13 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Another instance of a crash due to mishandling of
get_source_expressions()
beingExpression | None
.