Opened 8 years ago

Closed 4 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

Description

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 8 years ago.
select_related_depth.diff (4.3 KB) - added by punteney 7 years ago.
Updated patch to rev11528 and included tests and docs
9961_r17068.diff (4.3 KB) - added by Koen Biermans 5 years ago.
updated the patch to r17068

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by Roman Odaisky

Attachment: select_related.patch added

comment:1 Changed 8 years ago by Jacob

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 8 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 8 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 7 years ago by punteney

Attachment: select_related_depth.diff added

Updated patch to rev11528 and included tests and docs

comment:4 Changed 7 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 6 years ago by Matthias Kestenholz

Patch needs improvement: set

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

comment:6 Changed 5 years ago by Chris Beaven

Severity: Normal
Type: Cleanup/optimization

Changed 5 years ago by Koen Biermans

Attachment: 9961_r17068.diff added

updated the patch to r17068

comment:7 Changed 5 years ago by Koen Biermans

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

Added an updated patch (r17068).

comment:8 Changed 4 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