Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15116 closed (fixed)

Don't ORDER BY when using .in_bulk()

Reported by: Chris Lamb 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 Chris Lamb 6 years ago.

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by Chris Lamb

Attachment: in_bulk_ordering.diff added

comment:1 Changed 6 years ago by benweatherman

Triage Stage: UnreviewedDesign 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 6 years ago by Luke Plant

Triage Stage: Design decision neededReady 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 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

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 6 years ago by Russell Keith-Magee

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