Opened 6 years ago

Closed 6 years ago

#12258 closed (fixed)

QuerySet 'get' method should clear ordering before executing query

Reported by: mattdennewitz@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.1
Severity: Keywords: ordering, database, queryset, mq, mq-200912-dallas
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The SQL generated by the ORM when calling get includes ordering parameters specified in Meta.ordering. This might be small in the greater scheme of things, but clearing the ordering before evaluating the cloned QuerySet would prevent unnecessary filesorts by the database.

Attachments (3)

order_by.patch (472 bytes) - added by anonymous 6 years ago.
d (458 bytes) - added by jdunck 6 years ago.
d.diff (458 bytes) - added by jdunck 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by anonymous

comment:1 follow-up: Changed 6 years ago by nbv4

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I'm not 100% positive, but I'm pretty sure SQL databases internally optimize queries so that "LIMIT 1" automatically "cancels out" any ordering clause so that it doesn't execute the ordering code on that query.

comment:2 in reply to: ↑ 1 Changed 6 years ago by lukeplant

Replying to nbv4:

I'm not 100% positive, but I'm pretty sure SQL databases internally optimize queries so that "LIMIT 1" automatically "cancels out" any ordering clause so that it doesn't execute the ordering code on that query.

If they did, they would produce wrong results. .get() does not do a LIMIT 1 anyway — the only reason we can remove the ORDER BY here is because we retrieve all results and throw an exception if there isn't exactly one i.e. we do the limiting to one after the database has returned results, and excess rows cause no data to be returned.

comment:3 Changed 6 years ago by jdunck

Updated patch which passes all tests. The existing patch broke .latest() because you can't change ordering after slicing.

Changed 6 years ago by jdunck

  • Attachment d added

comment:4 Changed 6 years ago by jdunck

  • Keywords mq mq-200912-dallas added
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:5 Changed 6 years ago by jdunck

  • Triage Stage changed from Ready for checkin to Accepted

Changed 6 years ago by jdunck

comment:6 Changed 6 years ago by Alex

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 6 years ago by lukeplant

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

(In [11916]) Fixed #12258 - QuerySet.get() should clear ordering.

We only clear ordering when doing so cannot change the result returned by
the get() method i.e. when the query does not already define limits.

Thanks to mattdennewitz@… for the report, and jdunck for the patch

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