Opened 14 years ago

Closed 11 years ago

#9961 closed Cleanup/optimization (wontfix)

select_related(depth=0) should be a no-op

Reported by: Roman Odaisky Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The documentation does not define any behavior for queryset.select_related(depth=0). The implementation treats that as if the depth parameter were not set at all, selecting all related objects.

I propose having queryset.select_related(depth=0) return the same queryset, unaltered. That does not break documented behavior because it is not documented in the first place.

The reason is uniformity. Suppose I want to test how select_related affects performance. I add a constant and add select_related(depth=SELECT_RELATED_DEPTH) in several places. I want to benchmark the application with different values, but the existing implementation of select_related does not allow me to set a value that would disable select_related altogether.

The patch is very simple, attaching.

Attachments (3)

select_related.patch (2.0 KB) - added by Roman Odaisky 14 years ago.
select_related_depth.diff (4.3 KB) - added by punteney 14 years ago.
Updated patch to rev11528 and included tests and docs
9961_r17068.diff (4.3 KB) - added by Koen Biermans 12 years ago.
updated the patch to r17068

Download all attachments as: .zip

Change History (11)

Changed 14 years ago by Roman Odaisky

Attachment: select_related.patch added

comment:1 Changed 14 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 14 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

This seems relatively harmless; it won't impact any fast paths. The reasoning given in the description is unconvincing, since it's easy to test that functionality already. However, we currently don't have any way to turn off select_related() behaviour in a queryset and this would reintroduce that. There are use-cases for that (when you're passed an arbitrary queryset and only need something at the most local level, so wish to ensure the smallest query possible is executed).

The patch needs tests however. If we're going to support this, we need to ensure it doesn't break. Also needs documentation (in ref/models/querysets.txt).

comment:3 Changed 14 years ago by Malcolm Tredinnick

Needs documentation: set
Needs tests: set
Patch needs improvement: set

*sigh* One day I'll remember to set all the ticket flags when I make the review comments.

Changed 14 years ago by punteney

Attachment: select_related_depth.diff added

Updated patch to rev11528 and included tests and docs

comment:4 Changed 14 years ago by punteney

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Updated patch to current rev and added some tests to check for it as well as updating the docs. Removed the "patch needs improvement" as malcolm only mentioned that it needed tests and docs.

comment:5 Changed 12 years ago by Matthias Kestenholz

Patch needs improvement: set

Patch rot; does not apply anymore unfortunately. Marking as needs improvement again :-(

comment:6 Changed 12 years ago by Chris Beaven

Severity: Normal
Type: Cleanup/optimization

Changed 12 years ago by Koen Biermans

Attachment: 9961_r17068.diff added

updated the patch to r17068

comment:7 Changed 12 years ago by Koen Biermans

Easy pickings: unset
Patch needs improvement: unset
UI/UX: unset

Added an updated patch (r17068).

comment:8 Changed 11 years ago by Łukasz Rekucki

Resolution: wontfix
Status: newclosed

The depth parameter is now deprecated, see #16855.

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