Opened 16 years ago
Closed 12 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)
Change History (11)
by , 16 years ago
| Attachment: | select_related.patch added |
|---|
comment:1 by , 16 years ago
| Triage Stage: | Unreviewed → Design decision needed |
|---|
comment:2 by , 16 years ago
| Triage Stage: | Design decision needed → Accepted |
|---|
comment:3 by , 16 years ago
| 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.
by , 15 years ago
| Attachment: | select_related_depth.diff added |
|---|
Updated patch to rev11528 and included tests and docs
comment:4 by , 15 years ago
| 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 by , 14 years ago
| Patch needs improvement: | set |
|---|
Patch rot; does not apply anymore unfortunately. Marking as needs improvement again :-(
comment:6 by , 14 years ago
| Severity: | → Normal |
|---|---|
| Type: | → Cleanup/optimization |
comment:7 by , 13 years ago
| Easy pickings: | unset |
|---|---|
| Patch needs improvement: | unset |
| UI/UX: | unset |
Added an updated patch (r17068).
comment:8 by , 12 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
The depth parameter is now deprecated, see #16855.
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).