[Django] #34564: returning None instead of zero in Count annotation
Django
-------------------------------------+-------------------------------------
Reporter: Amin | Owner: nobody
Aminian |
Type: Bug | Status: new
Component: Database | Version: 4.2
layer (models, ORM) | Keywords: count, orm,
Severity: Normal | annotate
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I am trying to upgrade the Django's version from 3.2 to 4.2 in my project.
after installation, some of my tests were broken.
The problem:
I am annotating a queryset with Count function, and the count result in
some objects should be zero.
This is my code:
{{{
Order.objects.annotate(count=Count("customer", distinct=True))
}}}
In Django==3.2, if some orders don't have any customers, the count field
would be zero (as I want).
But in Django==4.2, if some orders don't have any customers, the count
field would be None, which breaks my code.
I read the source code for both versions. In version 3.2, we have a method
called `convert_value` in `Count` class in `aggregates` module. The method
is:
{{{
class Count(Aggregate):
...
def convert_value(self, value, expression, connection):
return 0 if value is None else value
}}}
As we can see, this function is responsible to return 0 instead of None.
But this method is deleted in version 4.2, and the conversion is handling
in `convert_value` property in `BaseExpression` class in `expressions`
module:
{{{
class BaseExpression:
...
@cached_property
def convert_value(self):
field = self.output_field
internal_type = field.get_internal_type()
if internal_type == "FloatField":
return (
lambda value, expression, connection: None
if value is None
else float(value)
)
elif internal_type.endswith("IntegerField"):
return (
lambda value, expression, connection: None
if value is None
else int(value)
)
elif internal_type == "DecimalField":
return (
lambda value, expression, connection: None
if value is None
else Decimal(value)
)
return self._convert_value_noop
}}}
In this property we are return a lambda function to be called later, and
when we call this lambda, what ever we pass to it, we would get None as
the result.
Should it be something like returning 0, if the value is None ?
--
Ticket URL: <https://code.djangoproject.com/ticket/34564>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: count, orm, | Triage Stage:
annotate | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
This was an intended change in 9f3cce172f6913c5ac74272fa5fc07f847b4e112.
You can use `Coalesce()` to return `0` instead, e.g.
{{{
Coalesce(Count("customer", distinct=True)), 0)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: count, orm, | Triage Stage:
annotate | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
I think this should be considered a release blocker as this is an
unintended regression that will affect quite a few users given how common
count annotations are over multiple valued relationships.
Unlike `ArrayAgg` and friends in fee87345967b3d917b618533585076cbfa43451b
there was not deprecation period to force the usage of `default=0` for
`Count` in 9f3cce172f6913c5ac74272fa5fc07f847b4e112.
A simple solution for a backport would be to adjust `Count.__init__` to do
`extra.setdefault("default", 0)` (or define in it's signature for enhanced
introspection) so `Coalesce` is used.
Happy to submit a patch if you agree.
Amin, you can use `Count(..., default=0)` in the mean time.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: nobody
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
annotate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
* stage: Unreviewed => Accepted
Comment:
OK, let's accept this.
Replying to [comment:2 Simon Charette]:
> I think this should be considered a release blocker as this is an
unintended regression that will affect quite a few users given how common
count annotations are over multiple valued relationships.
This is a regression in Django 4.0, so it doesn't qualify for a backport
anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:3>
Django
-------------------------------------+-------------------------------------
| Charette
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
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Simon
| Charette
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 Amin Aminian):
Replying to [comment:2 Simon Charette]:
> I think this should be considered a release blocker as this is an
unintended regression that will affect quite a few users given how common
count annotations are over multiple valued relationships.
>
> Unlike `ArrayAgg` and friends in
fee87345967b3d917b618533585076cbfa43451b there was not deprecation period
to force the usage of `default=0` for `Count` in
9f3cce172f6913c5ac74272fa5fc07f847b4e112.
>
> A simple solution for a backport would be to adjust `Count.__init__` to
do `extra.setdefault("default", 0)` (or define in it's signature for
enhanced introspection) so `Coalesce` is used.
>
> Happy to submit a patch if you agree.
>
> Amin, you can use `Count(..., default=0)` in the mean time.
I have read 9f3cce172f6913c5ac74272fa5fc07f847b4e112. The thing that i'm
not understanding is why we want to return None instead of zero. What is
the reason ? How this is benefiting us ?
As far as I have read the changes, in Django==4.2, we have an attribute
called `empty_result_set_value` which is equal to `NotImplemented` in
`BaseExpression`:
{{{
class BaseExpression:
empty_result_set_value = NotImplemented
...
}}}
And aggregate classes have implemented this. All of classes have
`empty_result_set_value = None` instead of `Count` class and `RegrCount`
class, which it is `empty_result_set_value = 0` in those classes. And
because of this attr, we can't use `default=0`:
{{{
class Aggregate(Func):
...
empty_result_set_value = None
def __init__(
self, *expressions, distinct=False, filter=None, default=None,
**extra
):
...
if default is not None and self.empty_result_set_value is not
None:
raise TypeError(f"{self.__class__.__name__} does not allow
default.")
...
}}}
So as far as I understand, we are considering `empty_result_set_value` as
kind of a default value. So why don't we just return
`empty_result_set_value` in case of being None in `convert_value` property
?
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Simon
| Charette
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 Amin Aminian):
Replying to [comment:4 Mariusz Felisiak]:
Can't it be assigned to me ? I really would love to solve it, if we could
deal with the solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:6>
Django
-------------------------------------+-------------------------------------
| 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
-------------------------------------+-------------------------------------
* owner: Simon Charette => Amin Aminian
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:7>
Django
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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 Amin Aminian):
Thanks Simon for explaining `empty_result_set_value` usage.
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.
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 ?
What I am saying is a code like this:
{{{
class BaseExpression:
...
@cached_property
def convert_value(self):
field = self.output_field
internal_type = field.get_internal_type()
...
elif internal_type.endswith("IntegerField"):
return (
lambda value, expression, connection:
self.empty_result_set_value # instead of None
if value is None
else int(value)
)
...
return self._convert_value_noop
}}}
And about Python time conversions, we are already running this
`convert_property` for each row.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:9>
Django
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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 Amin Aminian):
All right then.
I will start working on first solution, as it sounds better for me, if
anything sounded like being complicated, I just simply use second solution
for now.
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:11>
Django
-------------------------------------+-------------------------------------
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 Amin Aminian):
Hi again.
I implemented the first solution, I want to just double check the output
...
I removed `empty_result_set_value` class attr from both `Count` and
`RegrCount`, so basically they are inheriting this attr from `Aggregate`
class.
In `__init__` method of `Count` and `RegrCount` class, I called
`super()...` with `default=0`, so the default value is 0 for `Count`. But
still, I don't let user pass default value for `Count` and the default
value would be 0 all the time.
So basically `Coalesce` is appended to `Count` query with this default
value. I fixed all tests that had `SELECT COUNT(...)` and changed it to
`SELECT COALESCE(COUNT(...), 0)`.
Is everything right and as we wanted ?
Thanks for your time Simon!
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:12>
Django
-------------------------------------+-------------------------------------
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):
Yep that sounds about right Amin!
[https://docs.djangoproject.com/en/4.2/internals/contributing/writing-code
/working-with-git/#publishing-work Submitting a PR with these changes]
should validate that everything work as expected.
Thanks for the report, rubber ducking, and working on a patch!
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:13>
Django
-------------------------------------+-------------------------------------
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 |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: count, orm, | Triage Stage: Ready for
annotate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Amin Aminian):
* status: assigned => closed
* resolution: => fixed
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
annotate |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
Ticket is not fixed until PR is merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: count, orm, | Triage Stage: Accepted
annotate |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* status: new => assigned
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:17>
Django
-------------------------------------+-------------------------------------
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: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Amin Aminian):
Hi again.
I was trying to write a regression test that I faced an issue.
Apparently, `Count` function is returning 0 usually. But I was unlucky
enough to face some situations that `Count` return `None`.
One situation is during my tests, when using `Timescaledb` (Postgres
extension). In this scenario, I'm getting `None` instead of 0 when using
`Count`.
And the another situation is on production server, when some complicated
queries are executed and I'm getting None there too. I cannot find out why
that leads to `None`, as I get 0 when using similar queries.
So I'm basically asking, do you know some simple scenarios that DB would
return None ? (I suppose the old `convert_value` function was there for
those scenarios)
And if your not, is it valid to use `Timescaledb` in test ? (Actually I
have written such a test already for TDD development during this PR)
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:18>
Django
-------------------------------------+-------------------------------------
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: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Django doesn't support TimescaleDB so any change against it is not
considered a regression from our perspective. You might have run into bugs
with TimescaleDB for all we know.
The fact you cannot reproduce against SQLite, Postgres, MySQL, or Oracle
might explain why this was not caught by the test suite in the first place
which would make more sense given how common such annotations are.
Here are my attempts at reproducing
- [https://dbfiddle.uk/eUmjoLFI Postgres]
- [https://dbfiddle.uk/XXhzrttD MySQL]
- [https://dbfiddle.uk/esgjJ2cA SQLite]
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:19>
Django
-------------------------------------+-------------------------------------
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: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Amin Aminian):
Replying to [comment:19 Simon Charette]:
>
> The fact you cannot reproduce against SQLite, Postgres, MySQL, or Oracle
might explain why this was not caught by the test suite in the first place
which would make more sense given how common such annotations are.
I'm sure about this problem on Timescale, and as it is an extension for
Postgres, I think it is a corner case for Postgres (or even other DBs).
I should have tried to do this regression test earlier. It is my bad,
sorry about that.
But still, I suggest to simply have old `convert_value` to prevent corner
cases for now and for the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: count, orm, | Triage Stage: Accepted
annotate |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => invalid
Comment:
Unfortunately this was not even demonstrated to be a corner case for
Postgres, if it was the case we'd certainly accept a patch for it.
We can't keep code around that we can't test for regression against I'm
afraid.
If someone is able to write a test demonstrating that `Count` can return
`NULL` values from modern SQL standard or against any supported database
backend that might diverge from the standard (Postgres, SQLite, MySQL,
Oracle) we can re-open this ticket but in the mean time this ticket
doesn't seem actionable.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Amin Aminian | Owner: Amin
| Aminian
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: count, orm, | Triage Stage: Accepted
annotate |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
[https://www.postgresql.org/docs/current/functions-aggregate.html PG docs
confirms] that `count(<expression>)` must always return a result as it
counts the number of rows for which `<expression>` is not null:
> count ( "any" ) → bigint
>
> Computes the number of input rows in which the input value is not null.
My advice would be to confirm this isn't documented behaviour with
Timescale, if it isn't then report the issue ¯\_(ツ)_/¯.
--
Ticket URL: <https://code.djangoproject.com/ticket/34564#comment:22>