[Django] #24141: contains() method for QuerySets
Django
----------------------------------------------+--------------------
Reporter: gormster | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Tests for object membership in a QuerySet. Trying to alleviate some of the
newbies using `in` to test for membership and accidentally selecting the
entire table.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gormster | Owner: gormster
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gormster):
* owner: nobody => gormster
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:1>
Django
Reporter: gormster | Owner:
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by gormster):
* owner: gormster =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:2>
Django
Reporter: gormster | Owner:
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
Comment:
Closing per discussion on the [https://github.com/django/django/pull/3906
pull request].
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:3>
Django
Reporter: gormster | Owner:
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by carljm):
Tim, I wouldn't say the discussion on that PR settled the question of
whether `QuerySet.contains()` is a useful feature or not.
I think the original PR (to implement `QuerySet.__contains__()` as
`.filter(pk=obj.pk).exists()` under the hood) is a non-starter, but adding
`Queryset.contains()` seems reasonable to me. Gormster is right that there
are a number of questions on SO with people confused about how to
efficiently check for object containment in a queryset, and that M2M
relationships present a not-uncommon use case for it. My inclination would
be slightly in favor of adding it, if gormster updated the patch with
tests and docs. But I'm interested in what others think.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:4>
Django
Reporter: gormster | Owner:
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: closed => new
* needs_docs: 0 => 1
* resolution: wontfix =>
* needs_tests: 0 => 1
Comment:
Oops. Sorry, Carl. I saw your comment "The main argument against
`contains()`, I think, would just be that it's rarely if ever needed." and
not the one before it "Adding this as a .contains() method to QuerySet is
a reasonable feature request that's consistent with the current design
philosophy".
I am not sure if this proposal will lead to better code for anyone who
doesn't read the docs or not. Since it hides the underlying fact of
whether or not the QuerySet is cached, it seems to me that it won't
necessarily be more intuitive to write code that's more efficient. For
example:
{{{
queryset = Foo.objects.all()
for obj in range objs:
if queryset.contains(obj):
print("Found %s" % obj)
}}}
In the new example, my understanding is we'll get `len(obj)` queries,
where as if we use `if obj in queryset`, we'll get 1 query. Please correct
me if I'm wrong.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:5>
Django
Reporter: gormster | Owner:
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (by carljm):
You're right, but note that this is exactly parallel to the way
`QuerySet.count()` and `QuerySet.exists()` currently work - they use the
cache if it is populated, otherwise they execute a query. Of course,
there's no reason to ever call `QuerySet.count()` or `QuerySet.exists()`
multiple times in a loop, so maybe it will be more of a trap in this case.
But it's already true that if you do `.count()` on a queryset that you are
later populating fully, you've done an unnecessary extra query.
For your demonstration case, the cutoff between when it's more efficient
to do more queries vs more efficient to pull in the entire QuerySet is not
a priori clear; it would depend very much on the size of the whole
queryset and the number of objs you are checking for containment. Probably
for many such check-containment-of-multiple-objects cases the most
efficient approach will be to do a `values_list` query and check obj IDs
for membership in that set of IDs.
It's already true and will always be true, I think, that in order to
maximize query efficiency using the Django ORM, you have to understand how
it works. That includes things like knowing when to use `select_related`
and `prefetch_related`, and it also includes understanding the result
cache and what triggers populating it. In the end I don't think adding
`.contains(obj)` really changes much, it's just a nicer spelling for
`.filter(pk=obj.pk).exists()`. The question is really whether good use
cases for the latter are common enough to warrant adding a method. And I'm
really fine with either answer to that question, just didn't want the
feature request to be dismissed without good rationale.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:6>
Django
Reporter: gormster | Owner:
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Changes (by timgraham):
* status: new => closed
* resolution: => needsinfo
Comment:
I guess a discussion on the DevelopersMailingList needs to happen in order
to move this forward. Closing until that discussion happens and there's
agreement to add this.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:7>
Django
Reporter: gormster | Owner: (none)
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Changes (by Johan Schiff):
* cc: Johan Schiff (added)
* status: closed => new
* resolution: needsinfo =>
* needs_tests: 1 => 0
Old description:
> Tests for object membership in a QuerySet. Trying to alleviate some of
> the newbies using `in` to test for membership and accidentally selecting
> the entire table.
New description:
Tests for object membership in a QuerySet. Trying to alleviate some of the
newbies using `in` to test for membership and accidentally selecting the
entire table.
Discussion in django-developers:
[https://groups.google.com/forum/#!msg/django-developers/NZaMq9BALrs/YC-
Loq5ZAwAJ]
Pull request:
[https://github.com/django/django/pull/13038]
--
Comment:
Reopening, based on discussion in django-developers.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:8>
Django
Reporter: gormster | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Changes (by felixxm):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:9>
Django
Reporter: gormster | Owner: (none)
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by Johan Schiff):
* needs_docs: 1 => 0
Old description:
> Tests for object membership in a QuerySet. Trying to alleviate some of
> the newbies using `in` to test for membership and accidentally selecting
> the entire table.
>
> Discussion in django-developers:
> [https://groups.google.com/forum/#!msg/django-developers/NZaMq9BALrs/YC-
> Loq5ZAwAJ]
>
> Pull request:
> [https://github.com/django/django/pull/13038]
New description:
Tests for object membership in a QuerySet. Trying to alleviate some of the
newbies using `in` to test for membership and accidentally selecting the
entire table.
[https://groups.google.com/forum/#!msg/django-developers/NZaMq9BALrs/YC-
Loq5ZAwAJ django-developers]
[https://github.com/django/django/pull/13038 PR]
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:10>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* owner: (none) => Johan Schiff
* needs_better_patch: 0 => 1
* status: new => assigned
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:11>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_docs: 1 => 0
Comment:
Author added release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:12>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Looks like the patch still needs improvements regarding invalid model type
handling and the fact the `_result_cache` scanning is O(n).
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:13>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
Comment:
Author appears to have resolved Simon's comments. (Tip: unchecking Needs
Improvements gets your patch back in the review queue.)
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:14>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:15>
Django
Reporter: gormster | Owner: Johan
| Schiff
Type: New feature | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:16>
Django
Reporter: gormster | Owner: Johan
| Schiff
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d01709aae21de9cd2565b9c52f32732ea28a2d98" d01709a]:
{{{
#!CommitTicketReference repository=""
revision="d01709aae21de9cd2565b9c52f32732ea28a2d98"
Fixed #24141 -- Added QuerySet.contains().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24141#comment:17>