Opened 14 months ago

Closed 12 months ago

Last modified 12 months ago

#22349 closed Cleanup/optimization (fixed)

RawQuerySet doesn't implement __len__, __bool__

Reported by: cdestigter Owned by: Tim Graham <timograham@…>
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (13)

comment:1 Changed 14 months ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:2 Changed 14 months ago by ryanmrubin

  • Owner changed from nobody to ryanmrubin
  • Status changed from new to assigned

comment:3 Changed 14 months ago by ryanmrubin

  • Owner ryanmrubin deleted
  • Status changed from assigned to new

comment:4 Changed 14 months ago 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.

comment:5 Changed 14 months ago 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

comment:6 Changed 14 months ago by loic84

  • Cc loic@… added

comment:7 Changed 14 months ago 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.

comment:8 Changed 14 months ago by charettes

  • Component changed from Database layer (models, ORM) to Documentation
  • Needs documentation set
  • Type changed from Bug to Cleanup/optimization

Moving back to documentation based on the provided feedback.

comment:9 Changed 12 months ago by mardini

  • Has patch set

comment:10 Changed 12 months ago by mardini

  • Needs documentation unset

comment:11 Changed 12 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 2d425116e2f26ab546c380784df1de79562009a7:

Fixed #22349 -- Added a note clarifying RawQuerySet has no __len__.

Thanks cdestigter for the report.

comment:12 Changed 12 months ago by Tim Graham <timograham@…>

In d3bf537324c1164dee45070f90d48661b27a781f:

[1.7.x] Fixed #22349 -- Added a note clarifying RawQuerySet has no __len__.

Thanks cdestigter for the report.

Backport of 2d425116e2 from master

comment:13 Changed 12 months ago by Tim Graham <timograham@…>

In 26983c4c6414f00a2bcf364436ecce0f5e73851d:

[1.6.x] Fixed #22349 -- Added a note clarifying RawQuerySet has no __len__.

Thanks cdestigter for the report.

Backport of 2d425116e2 from master

Note: See TracTickets for help on using tickets.
Back to Top