Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15116 closed (fixed)

Don't ORDER BY when using .in_bulk()

Reported by: lamby Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Specifying an ORDER BY is wasteful as the database engine will order the results (according to our model's default ordering) and then we throw that ordering away when we put them in a dictionary.

Patch attached.

Attachments (1)

in_bulk_ordering.diff (456 bytes) - added by lamby 4 years ago.

Download all attachments as: .zip

Change History (5)

Changed 4 years ago by lamby

comment:1 Changed 4 years ago by benweatherman

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

It seems like there are a couple of options here:

  1. If there is an ordering specified, we could return a django.utils.datastructures.SortedDict. If there is no ordering specified, keep the code as it is in trunk.
  2. We could do what is in the patch and ignore the ordering.
  3. If there's an ordering specified, we could throw an assertion (similar to the patch, but more explicit and seems more consistent with limiting or offset when using in_bulk).

comment:2 Changed 4 years ago by lukeplant

  • Triage Stage changed from Design decision needed to Ready for checkin

We certainly don't want to start throwing exceptions (Option 3) because there happens to be an ordering on the queryset - the semantics of in_bulk() make it obvious that ordering becomes irrelevant, but not that ordering is illegal. The assertion is thrown for limit/offset because the method would return incorrect results otherwise, due to the adding of the filter.

Option 1 would add the overhead of a SortedDict (which is significant), and it would probably do this in most cases, because most querysets will inherit a default ordering from the Model. If people need ordering, they have other options. So option 2 - the original patch - seems by far the best option. Tests/docs not needed, since this is a performance tweak that is virtually impossible to test, and we already have at least one test that will check with in_bulk() still works.

comment:3 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

In [15457]:

Fixed #15116 -- Strip ordering clause from in_bulk() queries, since ordering information will be lost. Thanks to lamby for the report and patch.

comment:4 Changed 4 years ago by russellm

In [15459]:

[1.2.X] Fixed #15116 -- Strip ordering clause from in_bulk() queries, since ordering information will be lost. Thanks to lamby for the report and patch.

Backport of r15457 from trunk.

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