[Django] #23662: QuerySet __nonzero__, __len__ cause queryset evaluation
Django
-------------------------------+--------------------
Reporter: smishlayev | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Current implementation:
https://github.com/django/django/blob/master/django/db/models/query.py
class QuerySet(object):
...
def __nonzero__(self):
self._fetch_all()
return bool(self._result_cache)
...
def __len__(self):
self._fetch_all()
return len(self._result_cache)
These methods call self._fetch_all(), thus evaluating the queryset.
Although, this behaviour is documented
(https://docs.djangoproject.com/en/1.7/ref/models/querysets/#when-
querysets-are-evaluated), it is not obvious.
It seems logical to evaluate queryset, when casting it to a list() or
iterating over it, but these particular cases have nothing to do with the
queryset contents. There exist specific lazy methods (QuerySet.exists()
and QuerySet.count() respectively) which, IMHO, should be used for the
magic method implementation.
If I already have fetched the results of a queryset, I'm okay to call
__len__() on them, but if they haven't been retrieved yet, I'd rather use
SQL COUNT() instead. That is exactly, what QuerySet.count() does. The same
goes for the __nonzero__() and exists().
--
Ticket URL: <https://code.djangoproject.com/ticket/23662>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: smishlayev | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.7
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by smishlayev):
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
* has_patch: 0 => 1
* ui_ux: 0 => 1
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/23662#comment:1>
Django
Reporter: smishlayev | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.7
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> Current implementation:
> https://github.com/django/django/blob/master/django/db/models/query.py
> class QuerySet(object):
> ...
> def __nonzero__(self):
> self._fetch_all()
> return bool(self._result_cache)
> ...
> def __len__(self):
> self._fetch_all()
> return len(self._result_cache)
>
> These methods call self._fetch_all(), thus evaluating the queryset.
> Although, this behaviour is documented
> (https://docs.djangoproject.com/en/1.7/ref/models/querysets/#when-
> querysets-are-evaluated), it is not obvious.
>
> It seems logical to evaluate queryset, when casting it to a list() or
> iterating over it, but these particular cases have nothing to do with the
> queryset contents. There exist specific lazy methods (QuerySet.exists()
> and QuerySet.count() respectively) which, IMHO, should be used for the
> magic method implementation.
>
> If I already have fetched the results of a queryset, I'm okay to call
> __len__() on them, but if they haven't been retrieved yet, I'd rather use
> SQL COUNT() instead. That is exactly, what QuerySet.count() does. The
> same goes for the __nonzero__() and exists().
New description:
Current implementation:
https://github.com/django/django/blob/master/django/db/models/query.py
--
Comment (by dfunckt):
Edited bug description to properly format example code
--
Ticket URL: <https://code.djangoproject.com/ticket/23662#comment:2>
Django
Reporter: smishlayev | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.7
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> Current implementation:
> https://github.com/django/django/blob/master/django/db/models/query.py
>
> {{{
> class QuerySet(object):
> ...
> def __nonzero__(self):
> self._fetch_all()
> return bool(self._result_cache)
> ...
> def __len__(self):
> self._fetch_all()
> return len(self._result_cache)
> }}}
>
> These methods call self._fetch_all(), thus evaluating the queryset.
> Although, this behaviour is documented
> (https://docs.djangoproject.com/en/1.7/ref/models/querysets/#when-
> querysets-are-evaluated), it is not obvious.
>
> It seems logical to evaluate queryset, when casting it to a list() or
> iterating over it, but these particular cases have nothing to do with the
> queryset contents. There exist specific lazy methods (QuerySet.exists()
> and QuerySet.count() respectively) which, IMHO, should be used for the
> magic method implementation.
>
> If I already have fetched the results of a queryset, I'm okay to call
> __len__() on them, but if they haven't been retrieved yet, I'd rather use
> SQL COUNT() instead. That is exactly, what QuerySet.count() does. The
> same goes for the __nonzero__() and exists().
New description:
Current implementation:
https://github.com/django/django/blob/master/django/db/models/query.py
{{{
class QuerySet(object):
...
def __nonzero__(self):
self._fetch_all()
return bool(self._result_cache)
...
def __len__(self):
self._fetch_all()
return len(self._result_cache)
}}}
These methods call `self._fetch_all()`, thus evaluating the queryset.
Although, this behaviour is
[https://docs.djangoproject.com/en/1.7/ref/models/querysets/#when-
querysets-are-evaluated documented], it is not obvious.
It seems logical to evaluate queryset, when casting it to a `list()` or
iterating over it, but these particular cases have nothing to do with the
queryset contents. There exist specific lazy methods (`QuerySet.exists()`
and `QuerySet.count()` respectively) which, IMHO, should be used for the
magic method implementation.
If I already have fetched the results of a queryset, I'm okay to call
`__len__()` on them, but if they haven't been retrieved yet, I'd rather
use `SQL COUNT()` instead. That is exactly, what QuerySet.count() does.
The same goes for the `__nonzero__()` and `exists()`.
--
Comment (by dfunckt):
Ditto
--
Ticket URL: <https://code.djangoproject.com/ticket/23662#comment:3>
Django
Reporter: smishlayev | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.7
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by tchaumeny):
In my opinion, that could almost be considered as a backward incompatible
change as people might rely on the fact that doing `len(qs)` followed by
`for ... in qs:` will only perform one query (which is important on
Postgres as a `COUNT(*)` query is rather expensive). Same goes for
`bool(qs)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23662#comment:4>
Django
Reporter: smishlayev | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: 1.7
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
There is merit in your request but it's too late to change this, it would
be a major backward incompatibility. Lot of code rely on the fact that a
`QuerySet` is evaluated after being tested for equality or after a call to
`len()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23662#comment:5>