[Django] #33018: Incorrect annotation value when doing a subquery on empty queryset
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
decomorreno |
Type: Bug | Status: new
Component: Database | Version: 3.2
layer (models, ORM) | Keywords: orm, annotate,
Severity: Normal | EmptyQuerySet, empty, count
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
ORM seems to generate annotation/subqueries incorrectly if empty queryset
is used.
Models:
{{{
class Article(models.Model):
author_name = models.CharField(max_length=100)
content = models.TextField()
is_public = models.BooleanField()
class Comment(models.Model):
article = models.ForeignKey(Article, related_name="comments",
on_delete=models.CASCADE)
author_name = models.CharField(max_length=100)
content = models.TextField()
}}}
test data:
{{{
article = Article.objects.create(author_name="Jack", content="Example
content", is_public=True)
comment = Comment.objects.create(article=article, author_name="John",
content="Example comment")
}}}
queries:
{{{
qs = Article.objects.all()
# keep one list_x uncommented to see the difference:
list_x = ["random_thing_that_is_not_equal_to_any_authors_name"] # list not
empty, bug doesnt occur
#list_x = [] # if this list is empty, then the bug occurs
comment_qs = Comment.objects.filter(author_name__in=list_x)
qs = qs.annotate(
A=Coalesce(Subquery(
comment_qs.annotate(x=Count('content')).values('x')[:1],
output_field=IntegerField(),
), 101) # if list_x == [], Coalesce wont work and A will be 0 instead
of 101
)
# please note that above annotation doesnt make much logical sense, its
just for testing purposes
qs = qs.annotate(
B=Value(99, output_field=IntegerField())
)
qs = qs.annotate(
C=F("A") + F("B") # if list_x == [], C will result in 0 sic! instead
of 101 + 99 = 200
)
data = {
"A": qs.last().A,
"B": qs.last().B,
"C": qs.last().C,
}
print(data)
print(format_sql(qs.query))
}}}
console output for
{{{list_x=["random_thing_that_is_not_equal_to_any_authors_name"]}}}
(expected, correct):
{{{
{'A': 101, 'B': 99, 'C': 200}
SELECT "articles_article"."id",
"articles_article"."author_name",
"articles_article"."content",
"articles_article"."is_public",
COALESCE(
(SELECT COUNT(U0."content") AS "x"
FROM "articles_comment" U0
WHERE U0."author_name" IN
(random_thing_that_is_not_equal_to_any_authors_name)
GROUP BY U0."id", U0."article_id", U0."author_name",
U0."content"
LIMIT 1), 101) AS "A",
99 AS "B",
(COALESCE(
(SELECT COUNT(U0."content") AS "x"
FROM "articles_comment" U0
WHERE U0."author_name" IN
(random_thing_that_is_not_equal_to_any_authors_name)
GROUP BY U0."id", U0."article_id", U0."author_name",
U0."content"
LIMIT 1), 101) + 99) AS "C"
FROM "articles_article"
}}}
console output for {{{list_x=[]}}} (incorrect):
{{{
{'A': 0, 'B': 99, 'C': 0}
SELECT "articles_article"."id",
"articles_article"."author_name",
"articles_article"."content",
"articles_article"."is_public",
0 AS "A",
99 AS "B",
0 AS "C"
FROM "articles_article"
}}}
Background story: Above queries are made up (simplified), but based on
some parts of logic that I had in my code. list_x was generated
dynamically, and it was very hard to detect what is causing unexpected
results. This behavior is very strange, I believe its a bug and needs to
be fixed, because it is totally unintuitive that:
{{{SomeModel.objects.filter(x__in=["something_that_causes_this_qs_lenth_to_be_0"])}}}
and
{{{SomeModel.objects.filter(x__in=[]) }}}
may yield different results when used in queries later, even though
results of this querysets are logically equivalent
I will attach a minimal repro project (with code from above)
--
Ticket URL: <https://code.djangoproject.com/ticket/33018>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage:
EmptyQuerySet, empty, count | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "bug_repro.zip" added.
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage:
EmptyQuerySet, empty, count | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
EmptyQuerySet, empty, count |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
The `0` assignment on empty result set comes from
[https://github.com/django/django/blob/6c3525a09db5177bf4e3856de85bf8b1300402d5/django/db/models/sql/compiler.py#L268-L270
this line]. I assume we could adjust the logic to rely on `getattr(col,
'empty_aggregate_value', NotImplemented)` and fallback to `'0'` if it's
missing.
Makes me wonder if we want to rename `empty_aggregate_value` to
`empty_result_set_value` instead since it's not entirely bound to
aggregation anymore.
e.g. the following should exhibit the same behavior
{{{#!python
Author.objects.annotate(annotation=Coalesce(Author.objects.empty(), 42))
}}}
It also seems weird that we default to `0` as opposed to `NULL` which
would be a more correct value.
Alternatively we could adjust `Coalesce.as_sql` to catch `EmptyResultSet`
when it's compiling its source expressions but that's more involved as
most of the logic for that currently lives in `Func.as_sql`. We could also
use both of these approaches.
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:2>
Django
-------------------------------------+-------------------------------------
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage: Accepted
EmptyQuerySet, empty, count |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* has_patch: 0 => 1
* status: new => assigned
Comment:
Hi!
Thanks for the hints Simon, I tried a first patch where we catch empty
values here https://github.com/django/django/pull/14770
Should we expand the solution a bit further and rename
`empty_aggregate_value` as you suggested, and use it in the `SQLCompiler`
too?
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage: Accepted
EmptyQuerySet, empty, count |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by decomorreno):
Hi, thanks for your PR, do we have any updates on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage: Accepted
EmptyQuerySet, empty, count |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Wobrock):
Hi, the patch is waiting on some review I believe. So once a maintainer
has a bit of time available, we'll be able to move forward :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage: Accepted
EmptyQuerySet, empty, count |
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orm, annotate, | Triage Stage: Accepted
EmptyQuerySet, empty, count |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
EmptyQuerySet, empty, count | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: orm, annotate, | Triage Stage: Ready for
EmptyQuerySet, empty, count | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"dd1fa3a31b4680c0d3712e6ae122b878138580c7" dd1fa3a3]:
{{{
#!CommitTicketReference repository=""
revision="dd1fa3a31b4680c0d3712e6ae122b878138580c7"
Fixed #33018 -- Fixed annotations with empty queryset.
Thanks Simon Charette for the review and implementation idea.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: decomorreno | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: orm, annotate, | Triage Stage: Ready for
EmptyQuerySet, empty, count | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b2a0978610413e4cd5ebb716b8bfa7803dff8d5b" b2a0978]:
{{{
#!CommitTicketReference repository=""
revision="b2a0978610413e4cd5ebb716b8bfa7803dff8d5b"
[4.0.x] Fixed #33018 -- Fixed annotations with empty queryset.
Thanks Simon Charette for the review and implementation idea.
Backport of dd1fa3a31b4680c0d3712e6ae122b878138580c7 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33018#comment:10>