[Django] #22349: RawQuerySet doesn't implement __len__, __bool__
Django
----------------------------------------------+--------------------
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
RawQuerySet should mirror QuerySet behaviour where possible.
However, `RawQuerySet.__bool__` always returns True, instead of evaluating
the query results and checking the length is nonzero.
`RawQuerySet.__len__` doesn't work at all:
*** TypeError: object of type 'RawQuerySet' has no len()
This is not intuitive since it's not how QuerySet works. It basically
means that you have to cast RawQuerySet to a list before doing much with
its results.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
For the sake of consistency and DRYness it might be worth creating common
ancestor class for `QuerySet` and `RawQuerySet` (say `BaseQuerySet`) that
implements common operations.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:1>
Django
Reporter: cdestigter | Owner:
Type: Bug | ryanmrubin
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ryanmrubin):
* status: new => assigned
* owner: nobody => ryanmrubin
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:2>
Django
Reporter: cdestigter | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by ryanmrubin):
* status: assigned => new
* owner: ryanmrubin =>
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:3>
Django
Reporter: cdestigter | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Dunno if it's by design, but `RawQuerySet` and `QuerySet` operate vastly
differently as there is no such thing as an "evaluated" `RawQuerySet`:
{{{
list(queryset)
with assertNumQueries(1)
for i in range(10):
queryset[0]
list(raw_queryset)
with assertNumQueries(10)
for i in range(10):
raw_queryset[0]
}}}
This fundamental difference makes having a shared ancestor impractical.
With that in mind, I think adding `__len__` would be a nasty footgun, if
you only want the number of rows, you can write a `COUNT` SQL query, if
you want the count and the result, then you should definitely store the
result in a list to avoid a second query.
Of course we could make `RawQuerySet` maintain an internal cache akin to
`QuerySet`, but that'd have to be a ticket in its own right.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:4>
Django
Reporter: cdestigter | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by cdestigter):
Wow, really? That's crazy. I had no idea. Perhaps the docs should make
mention of that? I have been using RawQueryset for `UPDATE ... RETURNING
*` queries and I'll need to check my code for bugs if that's the case.
Either we should add the internal cache, or at least the docs should add
some big huge scary warning about that inconsistency.
https://docs.djangoproject.com/en/dev/topics/db/sql/#django.db.models.Manager.raw
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:5>
Django
Reporter: cdestigter | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: loic@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:6>
Django
Reporter: cdestigter | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
We discussed this with Loic on IRC. Introducing `__len__` and friends
without caching the results is a bad idea. One possibility considered was
adding caching to RawQuerySet so that it works like QuerySet, and also add
.iterator() method to RawQuerySet so that when iterating over large
queryset one can avoid the performance and memory penalty of caching.
There doesn't seem to be a way to introduce this change using deprecation
path or check framework, so we would need to just do the change and warn
about the change in release notes. As the current behavior doesn't seem to
be a big pain point for users, and some users could be relying on current
behavior, I think it is better to leave things as they are. However, we
should add some documentation about the evaluation time differences
between qs and rawqs, so I'll leave this ticket open for that.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:7>
Django
Reporter: cdestigter | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_docs: 0 => 1
* component: Database layer (models, ORM) => Documentation
* type: Bug => Cleanup/optimization
Comment:
Moving back to documentation based on the provided feedback.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:8>
Django
Reporter: cdestigter | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mardini):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/2805
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:9>
Django
Reporter: cdestigter | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mardini):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:10>
Django
Reporter: cdestigter | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2d425116e2f26ab546c380784df1de79562009a7"]:
{{{
#!CommitTicketReference repository=""
revision="2d425116e2f26ab546c380784df1de79562009a7"
Fixed #22349 -- Added a note clarifying `RawQuerySet` has no `__len__`.
Thanks cdestigter for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:11>
Django
Reporter: cdestigter | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d3bf537324c1164dee45070f90d48661b27a781f"]:
{{{
#!CommitTicketReference repository=""
revision="d3bf537324c1164dee45070f90d48661b27a781f"
[1.7.x] Fixed #22349 -- Added a note clarifying `RawQuerySet` has no
`__len__`.
Thanks cdestigter for the report.
Backport of 2d425116e2 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:12>
Django
Reporter: cdestigter | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"26983c4c6414f00a2bcf364436ecce0f5e73851d"]:
{{{
#!CommitTicketReference repository=""
revision="26983c4c6414f00a2bcf364436ecce0f5e73851d"
[1.6.x] Fixed #22349 -- Added a note clarifying `RawQuerySet` has no
`__len__`.
Thanks cdestigter for the report.
Backport of 2d425116e2 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:13>
Django
Reporter: Craig de Stigter | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Following up to the discussion here, #29339 added results caching to
`RawQuerySet` and #29337 added `__len__()` and `__bool__()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22349#comment:14>