Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #25367 -- Allowed boolean expressions in QuerySet.filter() and exclude(). #8119

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

schinckel
Copy link
Contributor

Here is a first crack at making it possible to do nice things like:

queryset.filter(Exists(subquery))

Also:

queryset.annotate(foo=Case(When(Exists(subquery), then=...))

@schinckel
Copy link
Contributor Author

I'm well aware of the superficial issues with this PR (imports and the like). What I want to know is if this is an appropriate approach to implement filter expressions.

@jarshwah and I have been discussing this for some time: we thought there might be a whole lot more work to it than this, but it seems to me that we can just inject some logic in to explicitly handle Expression objects, as long as they have an output_field that is a BooleanField.

@schinckel schinckel changed the title WIP: Filter expressions WIP: Filter expressions/Expression filters Feb 27, 2017
@jarshwah
Copy link
Member

A few things off the top of my head:

  • Does this also work with exclude()?
  • Does it handle __related fields?
  • Do regular lookups (perhaps by adding a boolean output_field) work in the same way?

This is really cool though. I expected there to be a lot more work involved.

@schinckel
Copy link
Contributor Author

Yes. .exclude(Exists(foo)), and also .filter(~Exists(...)), and .exclude(~Exists(...)) all appear to work.

I'm not sure about the other two: can you elaborate on what sort of queries you mean? Anything that is an expression that has an output_field that is a boolean should work.

@jarshwah
Copy link
Member

  1. Never mind - Exists takes a queryset which sets up joins etc.

  2. Does using Lookups as Expressions work? See:

from django.db.models.lookups import GreaterThan
# Lookups don't currently define an output_field, but if they did..
Model.objects.filter(GreaterThan('counter', 3))

@schinckel
Copy link
Contributor Author

Okay, @funkybob just showed me a pair of EXPLAINs that shows an order of magnitude improvement by only having the EXISTS in the WHERE clause, rather than in the select.

I'm surprised, as I thought postgres would do a better job of optimising this.

With EXISTS in SELECT: https://explain.depesz.com/s/xaH

With EXISTS only in WHERE: https://explain.depesz.com/s/nc9m

@schinckel
Copy link
Contributor Author

@jarshwah Currently Model.objects.filter(GreaterThan('foo', 2)) fails, because the Lookup object doesn't like a string (or an F, actually).

@schinckel
Copy link
Contributor Author

I think we could possibly avoid the need for a Lookup to define an output_field: wouldn't all lookups imply they are a boolean output? If so, we can allow any Lookup in a filter.

@schinckel
Copy link
Contributor Author

Hmm. I'm not sure it's possible without quite a lot of work to do anything with a Lookup, as it is passed something as lhs that already has an output_field, and without that, we can't even instantiate a GreaterThan.

@schinckel schinckel changed the title WIP: Filter expressions/Expression filters Filter expressions/Expression filters Jul 1, 2017
@funkybob
Copy link
Contributor

funkybob commented Jul 1, 2017

I am a person and I approve of this feature :)

@adamchainz
Copy link
Sponsor Member

adamchainz commented Jul 3, 2017

Oxford comma. The best type of comma.

👌 I'm a fan, though I'm not sure Django has an official policy.

@jarshwah
Copy link
Member

jarshwah commented Aug 7, 2017

Let's get this one merged I reckon. I've made a suggestion for a small improvement, but that's probably all it needs. Couldn't find the ticket (easily) so a link here or a number would be aces.

django/db/models/sql/query.py Outdated Show resolved Hide resolved
django/db/models/expressions.py Outdated Show resolved Hide resolved
django/db/models/expressions.py Outdated Show resolved Hide resolved
django/db/models/expressions.py Outdated Show resolved Hide resolved
docs/ref/models/expressions.txt Outdated Show resolved Hide resolved
docs/ref/models/expressions.txt Outdated Show resolved Hide resolved
tests/expressions/tests.py Outdated Show resolved Hide resolved
tests/expressions/tests.py Outdated Show resolved Hide resolved
docs/ref/models/expressions.txt Outdated Show resolved Hide resolved
docs/ref/models/conditional-expressions.txt Outdated Show resolved Hide resolved
docs/ref/models/expressions.txt Outdated Show resolved Hide resolved
@schinckel schinckel force-pushed the filter-expressions branch 2 times, most recently from 6bada76 to c8a518f Compare March 2, 2018 08:18
@schinckel
Copy link
Contributor Author

schinckel commented Mar 2, 2018

@jarshwah Finally found the issue: https://code.djangoproject.com/ticket/25367

@schinckel schinckel changed the title Filter expressions/Expression filters Fixed #25367: Allow expressions in .filter() calls Mar 2, 2018
django/db/models/expressions.py Outdated Show resolved Hide resolved
docs/ref/models/expressions.txt Outdated Show resolved Hide resolved
docs/ref/models/conditional-expressions.txt Outdated Show resolved Hide resolved
docs/releases/3.0.txt Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Aug 13, 2019

I merged #11652 (Thanks Simon!) we should be able to get rid of for_where workaround.

@NyanKiyoshi NyanKiyoshi force-pushed the filter-expressions branch 4 times, most recently from f4a5cfe to 034d980 Compare August 14, 2019 13:25
@NyanKiyoshi
Copy link

The pull request was updated against the latest reviews 👍

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to see that the diff is really small now that preliminary changes have landed 🎉

Thanks to everyone involved so far, really excited so this land in the near future.

django/db/models/expressions.py Outdated Show resolved Hide resolved
django/db/models/expressions.py Show resolved Hide resolved
@felixxm felixxm self-assigned this Aug 27, 2019
@felixxm felixxm changed the title Fixed #25367 -- Allow expressions in .filter() calls Fixed #25367 -- Allowed boolean expressions in QuerySet.filter() and exclude(). Aug 27, 2019
@felixxm
Copy link
Member

felixxm commented Aug 27, 2019

I squashed commits, rebased from master, pushed minor edits, and:

I'm going to continue this tomorrow to make final edits (in docs and tests), to add more tests, and to check remaining comments.

@NyanKiyoshi
Copy link

Thanks @felixxm! I'm very busy so I didn't get any time to make changes on the PR and we really need such feature.

@felixxm
Copy link
Member

felixxm commented Aug 28, 2019

PR didn't work properly on Oracle because it supports only filters and EXISTS in the WHERE clause, other must be compared with True. I added workaround that IMO is acceptable.

@felixxm
Copy link
Member

felixxm commented Aug 28, 2019

@jarshwah Can you take a look at a85460e4da5835a3095462b07b4382033d094371?

@felixxm felixxm force-pushed the filter-expressions branch 5 times, most recently from 27caea6 to a85460e Compare August 28, 2019 14:25
@felixxm
Copy link
Member

felixxm commented Aug 28, 2019

I pushed all edits.

…exclude().

This allows using expressions that have an output_field that is a
BooleanField to be used directly in a queryset filters, or in the
When() clauses of a Case() expression.

Thanks Josh Smeaton, Tim Graham, Simon Charette, Mariusz Felisiak, and
Adam Johnson for reviews.

Co-Authored-By: NyanKiyoshi <hello@vanille.bid>
@felixxm felixxm merged commit 4137fc2 into django:master Aug 29, 2019
@felixxm
Copy link
Member

felixxm commented Aug 29, 2019

Thanks y'all 🎉 🎊

@charettes
Copy link
Member

I was doing a bit of archaeology and I stumbled on #4801 (comment).

We're not currently affected by that since we don't expose expressions such as GreaterThanOrEqual, e.g. lookups are not currently expressions, but if we were to add support for things like #12041 which does exactly that we'd likely be affected by this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet