#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):
> So, as far as I understood, if the query is going to return None, we
return empty_result_set_value instead, without running the query. Right ?
If it's right, there is still something that I'm missing.
That's not exactly how it works, there's a difference between returning
`None` and dealing with an empty result. Some aggregate functions might
return `None` even when not dealing with empty results (e.g. when
aggregation over a set of value that contain a `NULL`). It is not the case
for `Count` but that's the rationale for `empty_result_set_value` being a
distinct feature from `default` and how `convert_value` deals with `None`.
> My point is, assume that we have a query that the compiler does not know
that it is going to return None (normal queries like
objects.annotate(count=Count("..."))). In this case, the query is going to
be run, and the result would be fetched. Now why we can't use
empty_result_set_value in case of result is None ?
Because `empty_result_set_value` is used a single very peculiar case which
is when an `EmptyResultSet` is exception is raised at query compilation
time. The change you are proposing will work but they blur the line in
terms of `empty_result_set_value` usage. Why should expressions with an
`IntegerField` as an `output_field` default to `empty_result_set_value`
when other types don't? It also impacts the locality of change since one
can no longer see that `Count` is treated differently that the normal
`COUNT` SQL function solely by looking at its definition.
> And about Python time conversions, we are already running this
convert_property for each row.
Correct but adding even more logic to it makes it harder to remove
eventually which is something option 1. would eventually benefit from. In
reality these converters would only need to be run if an ''explicit''
`output_field` is provided otherwise the ORM has the possibility to
generate the proper SQL and backend pecularities can be dealt with
`get_db_converters`.
Since the regression is solely about `Count`, I believe that either 1. or
2. still remain the two best options.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:10>
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/010701881ff2c2a1-06e1b273-924a-4103-bccb-277b8b1732f8-000000%40eu-central-1.amazonses.com.
