Re: [Django] #34975: Getting refs does not work properly with models.Window and aggregation
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
Another instance of a crash due to mishandling of
`get_source_expressions()` being `Expression | None`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergey Nesterenko):
Replying to [comment:4 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
>
> {{{#!python
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:5>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
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'))`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
Comment:
Confirmed regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d, refs
#25307.
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Sergey Nesterenko | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: QuerySet, Window, | Triage Stage: Accepted
Aggregate, F |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34975#comment:9>