update to Django 5.1 #82
Conversation
timgraham
force-pushed
the
django51
branch
3 times, most recently
from
c1920be
to
98ee338
Compare
timgraham
force-pushed
the
django51
branch
2 times, most recently
from
55fd0c5
to
ee44faf
Compare
timgraham
force-pushed
the
django51
branch
2 times, most recently
from
8eca616
to
d4176d2
Compare
timgraham
force-pushed
the
django51
branch
3 times, most recently
from
9ec98b9
to
6cd1c8b
Compare
| @@ -19,7 +19,7 @@ def aggregate( | |||
| resolve_inner_expression=False, | |||
| **extra_context, # noqa: ARG001 | |||
| ): | |||
| if self.filter: | |||
| if self.filter and not isinstance(self.filter, Col): | |||
There was a problem hiding this comment.
@WaVEV I'm not sure if this change is correct, but it fixes aggregation.tests.AggregateTestCase.test_distinct_on_aggregate which otherwise incorrectly go down this branch after django/django@42b567a. Incidentally, that's also the commit that requires the additional of the if expr is Nonecheck in query_utils.py.
There was a problem hiding this comment.
🤔 wow interesting, I will check
There was a problem hiding this comment.
It's a bit strange. Here’s what I noticed:
-
In the
mongodb-5.1.xbranch, Django is generating some aggregation nodes like:Avg(, distinct=True, filter=Col(aggregation_book, <django.db.models.fields.FloatField: ratings>)). Meanwhile, version 5.1 using SQLite is generating the following node:Avg(Col(aggregation_book, aggregation.Book.rating), distinct=True). The first aggregation node with empty expressions and the proper column in the filter is unusual. -
We can handle cases where the filter has no expression but a column, though this might not be a common scenario. I’ll need to debug Django to understand why these versions generate different nodes. I believe it should always receive an expression, so this situation might be an edge case.
-
In the
mongodb-5.0.xbranch, the node is the same as in Django 5.1, so I think an error may have been introduced inmongodb-5.1.x.
There was a problem hiding this comment.
so I think an error may have been introduced in mongodb-5.1.x.
Right. Did you study the Django commit I linked to? I think it's likely we should update our code in some way other than this isinstance check, but I haven't dug in to understand the details. (This isn't the highest priority right now, but if you have some free cycles...)
There was a problem hiding this comment.
Yes. I found the problem, we need to adapt Django-mongodb. As you noticed the set_source_expressions has changed, so we have to change the line 94 from the file compiler with:
replacing_expr.set_source_expressions([inner_column, None]).
Django 5.1 will be released ~August 7, 2024.
This is to check compatibility in the meantime.