#34564: returning None instead of zero in Count annotation
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: count, orm, | Triage Stage: Accepted
annotate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Thanks for your consideration Mariusz, I missed that it wasn't a recent
regression, time fly by!
If that can help you Amin, the ORM compiler has some logic in place to
prevent doing database queries when it knows that it cannot match any rows
(e.g. `id__in=[]`). When this happens (aggregation or when annotating
subqueries that don't match any rows) we still need to provide some values
to the user that are coherent with what would have happened if the query
was still performed, that's when `empty_result_set_value` kicks in. You
might notice in this patch that `QuerySet.none()` is used, that's a public
way of making sure that evaluating the queryset results in no query.
Through a series a patch that tried to make the logic more coherent in
this area and the introduction of `Aggregate(..., default=default)` which
is basically a shorthand for `Coalesce(Aggregate(...), default)` the
`empty().aggregate(cnt=Count())` long standing issue where `{"cnt": None}`
was returned was fixed (due to `Count.empty_result_set_value = 0`) but
surprisingly (this is mind blowing to me) we didn't have any tests for the
case where `.annotate(cnt=Cnt("possibly_empty_multi_value"))` so
`convert_value` was wrongly removed assuming that it was covered by the
adjusted logic.
As for the patch I'm pretty sure I could see us going to ways
1. Drop `Count.empty_result_set_value` to use `extra["default"] = 0` so
`Coalesce(..., 0)` is systematically used and we inherit the
`empty_result_set_value` from the `Coaesce` and `Value` chain. This might
require adjusting some tests that check the exact SQL generated or assert
that `Count(default)` cannot be used.
2. Simply add back the `convert_value` that were removed in
9f3cce172f6913c5ac74272fa5fc07f847b4e112
1. seems more desirable to me as it pushes the logic to the database and
avoids Python time conversions (`convert_value` comes at a cost as its run
for each row) while 2. is less invasive as it requires no test changes and
keeps the generated SQL the same (no `Coalesce` wrapping for each usage of
`Count`).
Whatever solution we choose we should make sure to add address the same
issue we have with `RegrCount`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/010701881a6fd526-de444877-e078-4fa1-91dc-cba46479606d-000000%40eu-central-1.amazonses.com.
