Code

Opened 6 years ago

Closed 21 months ago

#9961 closed Cleanup/optimization (wontfix)

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

Reported by: rdaysky 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 rdaysky 6 years ago.
select_related_depth.diff (4.3 KB) - added by punteney 5 years ago.
Updated patch to rev11528 and included tests and docs
9961_r17068.diff (4.3 KB) - added by koenb 3 years ago.
updated the patch to r17068

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by rdaysky

comment:1 Changed 5 years ago by jacob

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

comment:2 Changed 5 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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 5 years ago by mtredinnick

  • 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 5 years ago by punteney

Updated patch to rev11528 and included tests and docs

comment:4 Changed 5 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 3 years ago by mk

  • Patch needs improvement set

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

comment:6 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Cleanup/optimization

Changed 3 years ago by koenb

updated the patch to r17068

comment:7 Changed 3 years ago by koenb

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

Added an updated patch (r17068).

comment:8 Changed 21 months ago by lrekucki

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

The depth parameter is now deprecated, see #16855.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.