[Django] #26430: Coalesce in Aggregations ignored when EmptyResultSet returned
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) | Keywords: aggregation coalesce in
Severity: Normal | queryset
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Using an empty list when using the `__in=` returns an EmptyResultSet and
prevents an Aggregate Coalesce from working properly. See below:
# Test with matched Queryset. Sum will return 50
{{{OrderItem.objects.filter(pk__in=[1]).aggregate(test=Coalesce(Sum('quantity'),
Value(0)))}}}
{{{>>>{'test': 50}}}}
# Test with unmatched Queryset. Sum will return 0
{{{OrderItem.objects.filter(pk__in=[-1]).aggregate(test=Coalesce(Sum('quantity'),
Value(0)))}}}
{{{>>> {'test':0}}}}
# Test with unmatched Queryset (using empty list). EmptyResultSet returned
because of empty list. Sum will return NONE
{{{OrderItem.objects.filter(pk__in=[]).aggregate(test=Coalesce(Sum('quantity'),
Value(0)))}}}
{{{>>> {'test': None}}}}
Simon Charette on django-users suggested the following:
''From what I understand the ORM simply doesn't perform any query in this
case
as the `pk__in` lookup cannot match any `OrderItem` and result in an
`EmptyResultSet` exception[1].''
''This exception is caught in the `Query.get_aggregation()` method where
all
aggregates are converted to `None`[2].''
''I suppose we should alter the `except EmptyResultSet` clause to account
for
`outer_query.annotation_select` items that are `Coalesce()` instances used
with
`Value()` but I'm unsure about how it should be done.''
[1]
https://github.com/django/django/blob/2e0cd26ffb29189add1e0435913fd1490f52b20d/django/db/models/lookups.py#L221-L223
[2]
https://github.com/django/django/blob/2e0cd26ffb29189add1e0435913fd1490f52b20d/django/db/models/sql/query.py#L439-L445
See full discussion here:
https://groups.google.com/forum/#!topic/django-users/HGD3Vv3IerA
--
Ticket URL: <https://code.djangoproject.com/ticket/26430>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage:
coalesce in queryset | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I do agree that this is a bug, but I don't think the correct fix is to
detect specific expressions and apply custom logic for each one. Simon
mentioned nested expressions which would be very difficult to resolve the
correct behaviour for.
For what it's worth, a `Count` will also show `None` in an EmptyResultSet
even though its converters should coerce `None` to `0`. The reason it
doesn't is because the expressions and aggregates aren't actually used.
Everything is Noned.
If I was writing this from scratch I'd raise an error on receiving an
EmptyResultSet. An empty list in a filter clause seems like broken
behaviour. We can't do that though, because it's backwards incompatible,
and there's probably valid use cases for this behaviour now.
A possible fix would be to implement something like `on_empty_result` for
Expressions, to let each subclass decide what it should return if it's not
executed. It would have to recursively call this method, and then decide
which children should be allowed to propagate their values. But honestly,
this seems like a lot of work and complexity for very little gain.
Short of documentation changes or a deprecation, I'd be tempted to wontfix
this. I'm certainly open to ideas I haven't considered though.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage:
coalesce in queryset | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ryanprater):
I can agree that detecting specific expressions is overkill and for that
matter, not scalable. However, I don't think an "empty list in a filter
clause seems like broken behavior". As you mentioned about valid use
cases: I'm using this with a SelectMultiple which obviously allows users
to select no options. Writing custom validation to prevent an empty query
should be the responsibility of the developer, but not enforced at the
framework level.
It seems like this could be avoided by modifying the logic of `In.rhs`
(and/or not throwing the `EmptyResultSet` at all?), but this is my first
bug and I'm not versed enough to discern.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage:
coalesce in queryset | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
I agree with Josh that the `on_empty_result` approach seems like a lot of
work for very little gain but I can't see any other way to fix this issue
appropriately.
@ryanprater not throwing `EmptyResultSet` in the `In` lookup would solve
your particular issue but the underlying one would remain, using
`Coalesce` with aggregation can result in an unexpected return value as
`EmptyResultSet` is thrown by other part of Django (`queryset.none()`
relies on it).
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage:
coalesce in queryset | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
Maybe we could somehow skip throwing EmptyResultSet when doing an
.aggregate() call, but throw it when doing normal queries. One idea is to
use query.add_context('in_aggregate_query', True), and then check
compiler.query.context before throwing the EmptyResultSet.
Another possible idea is to add as_python() methods to pretty much every
part of the ORM. The as_python() method would run the expressions as
Python code, so you could run whole querysets without actually touching
the database. For the case where the query for .aggregate() generates an
EmptyResultSet we would run the queryset in Python against one row having
all None values, thus producing the wanted answer. This could be extremely
convenient for testing, too. OTOH doing this properly will require at
least a couple of months of work, so this is for sure overkill for this
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage:
coalesce in queryset | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
FWIW I gave a naive try at implementing the `get_empty_result()` approach
suggested by Josh:
https://github.com/django/django/pull/6361
The full suite pass but coalesced results relying on database functions
(e.g. `NOW()`) are not supported.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: ryanprater | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
See #26433 for related issue and a new idea for solving this.
Marking as accepted, I think we can solve this without unreasonable
effort.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
This happens to be addressed by
https://github.com/django/django/pull/11715
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nick Pope):
Replying to [comment:7 Simon Charette]:
> This happens to be addressed by
https://github.com/django/django/pull/12602
I was looking into reviving https://github.com/django/django/pull/12602.
It seems that some parts of that were already addressed in
c2d4926702045e342a668057f0a758eec9db9436.
Notably this
[https://github.com/django/django/commit/c2d4926702045e342a668057f0a758eec9db9436
#diff-68490047146b30a432d0d7b7793005e89c112289dcb273fa082b4e9df9b81af7R977
solved] the `None` instead of `0` issue for `Count()` in
`aggregation_regress.tests.AggregationTests.test_empty_filter_aggregate`.
It doesn't seem to be true that this ticket will be addressed by that PR
as implied by
[https://github.com/django/django/pull/11715#issuecomment-602050401 this
comment].
Changing the test to use `Coalesce()` as below still fails with
`total_age` being `None`. There is nothing to determine that the second
argument of `Coalesce()` should be used.
{{{#!diff
self.assertEqual(
-
Author.objects.filter(id__in=[]).annotate(Count('friends')).aggregate(Count('pk')),
- {'pk__count': 0},
+
Author.objects.filter(pk__in=[]).annotate(Count('friends')).aggregate(total_age=Coalesce(Sum('age'),
0)),
+ {'total_age': 0},
)
}}}
In the case of `Count()` we get 0 because of `Count.convert_value()`
explicitly returning `0` if the value is `None`.
It seems that https://github.com/django/django/pull/6361 would be the way
forward.
Was there a reason you abandoned that approach, Simon? Would you be happy
for me to pick that up again?
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Hannes Ljungberg (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Anton Agestam (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:11>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Sorry for the delayed answer Nick.
> It seems that https://github.com/django/django/pull/6361 would be the
way forward. Was there a reason you abandoned that approach, Simon? Would
you be happy for me to pick that up again?
I think it would be the most straightforward way to get there but the main
issue with the `get_empty_result` approach is coalescence to database
expressions e.g. `Coalesce('col', Now())`.
It's tempting to mock our way there and ''cheat'' for provided expressions
in implementing Python equivalent (e.g. `Now.get_empty_result ->
timezone.now`) but it has its limits (e.g. imagine coalescing to calling a
database procedure). In the end `EmptyResultSet` is meant to be an
optimization so I believe we should turn it off in circumstances where it
affects the correctness of the returned results.
A simpler approach, inspired by Anssi's comment:4, would be to add a
`SQLCompiler.supports_empty_result_set = False` property and set it to
`False` for `SQLAggregateCompiler`. Parts of the code that raise this
exception could then be adjusted not to do so when the current compiler
doesn't support it.
(completely untested)
{{{#!diff
diff --git a/django/db/models/lookups.py b/django/db/models/lookups.py
index 8d3648b393..0b02287d2a 100644
--- a/django/db/models/lookups.py
+++ b/django/db/models/lookups.py
@@ -392,7 +392,7 @@ def process_rhs(self, compiler, connection):
except TypeError: # Unhashable items in self.rhs
rhs = [r for r in self.rhs if r is not None]
- if not rhs:
+ if not rhs and compiler.supports_empty_result_set:
raise EmptyResultSet
# rhs should be an iterable; use batch_process_rhs() to
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
index 7264929da8..6cfcbf7f9f 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -25,6 +25,7 @@ class SQLCompiler:
r'^(.*)\s(?:ASC|DESC).*',
re.MULTILINE | re.DOTALL,
)
+ supports_empty_result_set = True
def __init__(self, query, connection, using):
self.query = query
@@ -1636,6 +1637,11 @@ def pre_sql_setup(self):
class SQLAggregateCompiler(SQLCompiler):
+ # Due to the nature of aggregation coalescence some expressions might
+ # need to be interpreted on the database to return the correct value
+ # when dealing with an empty result set.
+ supports_empty_result_set = False
+
def as_sql(self):
"""
Create the SQL for this query. Return the SQL string and list of
}}}
I guess we could also use an hybrid approach where `SQLAggregateCompiler`
only sets `supports_empty_result_set = False` if any of
`outer_query.annotation_select.items()` is not a literal value prior to
proceeding with the compilation/`as_sql` phase. That would keep the
optimization for most cases but fallback to the database when it cannot be
computed on the Python side.
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:12>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Tried implementing the above in
https://github.com/django/django/pull/14430
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:13>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Nick Pope (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:14>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* needs_better_patch: 0 => 1
* status: new => assigned
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:15>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"5371ad1d149480bf64c37b3273e06a1c76fe4b90" 5371ad1d]:
{{{
#!CommitTicketReference repository=""
revision="5371ad1d149480bf64c37b3273e06a1c76fe4b90"
Refs #26430 -- Added tests for PostgreSQL-specific aggregates on
EmptyQuerySets and used subTest().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:16>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
(models, ORM) |
Keywords: aggregation | Triage Stage: Accepted
coalesce in queryset |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:17>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
(models, ORM) |
Keywords: aggregation | Triage Stage: Ready for
coalesce in queryset | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:18>
Django
-------------------------------------+-------------------------------------
| Charette
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: aggregation | Triage Stage: Ready for
coalesce in queryset | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9f3cce172f6913c5ac74272fa5fc07f847b4e112" 9f3cce17]:
{{{
#!CommitTicketReference repository=""
revision="9f3cce172f6913c5ac74272fa5fc07f847b4e112"
Refs #26430 -- Re-introduced empty aggregation optimization.
The introduction of the Expression.empty_aggregate_value interface
allows the compilation stage to enable the EmptyResultSet optimization
if all the aggregates expressions implement it.
This also removes unnecessary RegrCount/Count.convert_value() methods.
Disabling the empty result set aggregation optimization when it wasn't
appropriate prevented None returned for a Count aggregation value.
Thanks Nick Pope for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:20>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: closed
(models, ORM) |
Keywords: aggregation | Triage Stage: Ready for
coalesce in queryset | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f3112fde981052801e0c2121027424496c03efdf" f3112fde]:
{{{
#!CommitTicketReference repository=""
revision="f3112fde981052801e0c2121027424496c03efdf"
Fixed #26430 -- Fixed coalesced aggregation of empty result sets.
Disable the EmptyResultSet optimization when performing aggregation as
it might interfere with coalescence.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:19>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: closed
(models, ORM) |
Keywords: aggregation | Triage Stage: Ready for
coalesce in queryset | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"0f3e1a54bfa19f034f88bf3c25c67402d19e906c" 0f3e1a54]:
{{{
#!CommitTicketReference repository=""
revision="0f3e1a54bfa19f034f88bf3c25c67402d19e906c"
Refs #26430 -- Removed unused branch in sql.Query.get_count().
Now that sql.Query.get_aggregation() properly deals with empty result
sets summary Count() annotations cannot result in None.
Unused since 9f3cce172f6913c5ac74272fa5fc07f847b4e112.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26430#comment:21>