Fixed #27021 -- Allowed lookup expressions in annotations, aggregations, and QuerySet.filter(). #14494

LilyFirefly · 2021-06-05T14:00:08Z

https://code.djangoproject.com/ticket/27021

django/db/models/lookups.py

charettes

Thanks for the patch Ian! We've had recent questions about this exact usage cases.

I think we should defer the decision around __gt__ and friends and focus on making Lookup implement as much of the expression API as possible. That will not only pave the way for annotations but also direct usage in filter(...) and friends which can be useful under certain circumstances e.g. https://groups.google.com/g/django-developers/c/iiOnxAn3vy4

django/db/models/expressions.py

django/db/models/lookups.py

felixxm

@Ian-Foote Great job, thanks 🚀

Release notes are missing. I would also

add examples in the Query Expressions -> Some examples,
add a short paragraph in the Built-in Expressions or Lookup docs 🤔

Can we add extra tests?

combined lookups, e.g. Exact(F('integer'), 42) | GreaterThan(F('integer'), 42),
using lookups directly in the QuerySet.filter(), e.g. .filter(GreaterThan(F('integer'), 42)),
using lookups in the Queryset.alias().

django/db/models/lookups.py

charettes

Thanks for the adjustments Ian!

django/db/models/lookups.py

charettes · 2021-06-10T03:16:11Z

Note that the making Lookup(Expression) and implementing .resolve_expression will allow some simplification in Where.resolve_expression

django/django/db/models/sql/where.py

Lines 189 to 209 in ed3af3f

    
           @staticmethod 
        
           def _resolve_leaf(expr, query, *args, **kwargs): 
        
               if hasattr(expr, 'resolve_expression'): 
        
                   expr = expr.resolve_expression(query, *args, **kwargs) 
        
               return expr 
        
           @classmethod 
        
           def _resolve_node(cls, node, query, *args, **kwargs): 
        
               if hasattr(node, 'children'): 
        
                   for child in node.children: 
        
                       cls._resolve_node(child, query, *args, **kwargs) 
        
               if hasattr(node, 'lhs'): 
        
                   node.lhs = cls._resolve_leaf(node.lhs, query, *args, **kwargs) 
        
               if hasattr(node, 'rhs'): 
        
                   node.rhs = cls._resolve_leaf(node.rhs, query, *args, **kwargs) 
        
           def resolve_expression(self, *args, **kwargs): 
        
               clone = self.clone() 
        
               clone._resolve_node(clone, *args, **kwargs) 
        
               clone.resolved = True 
        
               return clone

LilyFirefly · 2021-06-10T07:42:03Z

@charettes I'm not clear what change you're suggesting in Where.resolve_expression.

charettes · 2021-06-16T05:45:35Z

@Ian-Foote not a big deal and it can be handled in a following the PR.

The idea is that Where.resolve_expression does a lot of duck-type digging where it could possibly simply call resolve_expression on all it's children now that Lookup.resolve_expression is implemented.

e.g. (still untested)

def resolve_expression(self, *args, **kwargs): 
    clone = self.clone()
    clone.children = [
        child.resolve_expression(*args, **kwargs) for child in clone.children
    ]
    clone.resolved = True
    return clone

LilyFirefly · 2021-06-16T18:42:03Z

@charettes I just tested that and got a bunch of errors. I spent a little time trying to debug, but decided to leave it for now.

charettes · 2021-06-16T18:44:21Z

@Ian-Foote sounds good, thanks for having a look!

LilyFirefly · 2021-06-16T22:21:37Z

@felixxm Good call on the combined expressions test - that needed some more tweaks to WhereNode.

felixxm

@Ian-Foote Thanks for updates 👍 We're close 🚀. I pushed edits to fix failures on Oracle.

I would add a few more tests:

using combined lookups directly in QuerySet.filter(),
using annotation of combined lookups in QuerySet.filter(),
using combined lookups/lookups in QuerySet.aggregate().

django/db/models/fields/related_lookups.py

docs/ref/models/lookups.txt

LilyFirefly · 2021-06-24T20:50:47Z

@felixxm I added those extra tests and fixed another edge-case. I'm not sure quite how to get the docs to build correctly.

docs/ref/models/lookups.txt

felixxm

@Ian-Foote Thanks for updates 👍 I skipped one test on Oracle, pushed small edits to docs, and left comments. Can we add one more test 😉? i.e. using lookups directly in Case().

tests/expressions/tests.py

django/db/models/lookups.py

django/db/backends/oracle/operations.py

charettes · 2021-06-27T23:27:39Z

@felixxm I didn't look into details of the Oracle failure but lmk if you hit a wall.

felixxm · 2021-06-28T05:38:55Z

@felixxm I didn't look into details of the Oracle failure but lmk if you hit a wall.

@Ian-Foote test_annotation_aggregate_with_m2o fails because book_contact_set__isnull is unnecessarily added to the GROUP BY clause:

SELECT
    "ANNOTATIONS_AUTHOR"."NAME",
    CASE WHEN "ANNOTATIONS_BOOK"."ID" IS NULL THEN 0 ELSE MAX(T4."PAGES") END AS "MAX_PAGES"
FROM
    "ANNOTATIONS_AUTHOR"
    LEFT OUTER JOIN "ANNOTATIONS_BOOK" ON ("ANNOTATIONS_AUTHOR"."ID" = "ANNOTATIONS_BOOK"."CONTACT_ID")
    LEFT OUTER JOIN "ANNOTATIONS_BOOK_AUTHORS" ON ("ANNOTATIONS_AUTHOR"."ID" = "ANNOTATIONS_BOOK_AUTHORS"."AUTHOR_ID")
    LEFT OUTER JOIN "ANNOTATIONS_BOOK" T4 ON ("ANNOTATIONS_BOOK_AUTHORS"."BOOK_ID" = T4."ID")
WHERE "ANNOTATIONS_AUTHOR"."AGE" < 30
GROUP BY
    "ANNOTATIONS_AUTHOR"."ID",
    "ANNOTATIONS_AUTHOR"."NAME",
    "ANNOTATIONS_AUTHOR"."AGE",
    CASE WHEN "ANNOTATIONS_BOOK"."ID" IS NULL THEN 1 ELSE 0 END

previously

SELECT
    "ANNOTATIONS_AUTHOR"."NAME",
    CASE WHEN "ANNOTATIONS_BOOK"."ID" IS NULL THEN 0 ELSE MAX(T4."PAGES") END AS "MAX_PAGES"
FROM
    "ANNOTATIONS_AUTHOR"
    LEFT OUTER JOIN "ANNOTATIONS_BOOK" ON ("ANNOTATIONS_AUTHOR"."ID" = "ANNOTATIONS_BOOK"."CONTACT_ID")
    LEFT OUTER JOIN "ANNOTATIONS_BOOK_AUTHORS" ON ("ANNOTATIONS_AUTHOR"."ID" = "ANNOTATIONS_BOOK_AUTHORS"."AUTHOR_ID")
    LEFT OUTER JOIN "ANNOTATIONS_BOOK" T4 ON ("ANNOTATIONS_BOOK_AUTHORS"."BOOK_ID" = T4."ID")
WHERE "ANNOTATIONS_AUTHOR"."AGE" < 30
GROUP BY
    "ANNOTATIONS_AUTHOR"."ID",
    "ANNOTATIONS_AUTHOR"."NAME",
    "ANNOTATIONS_AUTHOR"."AGE",
    "ANNOTATIONS_BOOK"."ID"

felixxm · 2021-07-07T06:30:07Z

@Ian-Foote Do you have time to keep working on this? If not I can try to fix remaining comments.

LilyFirefly · 2021-07-07T11:00:27Z

I had a look last night, but I didn't get anywhere (not having a working oracle setup was the biggest blocker).

felixxm · 2021-07-07T11:04:04Z

I had a look last night, but I didn't get anywhere (not having a working oracle setup was the biggest blocker).

You could check other comments and leave an Oracle issue to me 🕵️

LilyFirefly · 2021-07-07T11:06:51Z

Yeah, I'll do that now.

felixxm · 2021-07-07T11:08:09Z

Yeah, I'll do that now.

Great, thanks ❤️

LilyFirefly · 2021-07-07T12:05:41Z

@felixxm All yours ;)

felixxm · 2021-07-08T07:22:06Z

@Ian-Foote I fixed Oracle failure by adding get_group_by_cols() ☺️

felixxm · 2021-07-08T10:51:54Z

@Ian-Foote Thanks for updates 👍 We are almost there 🏃

I rebased, squashed commits, fix tests failures on Oracle, added new tests test_lookup_in_order_by, test_conditional_expression, test_filter_lookup_lhs and fixed related failures on Oracle, and pushed small edits.

Unfortunately test_filter_lookup_lhs crashes on PostgreSQL 😭 , it's an issue similar to the 170b006. It seems that LHS should be wrapped in parentheses to respect operator precedence 🤔 (\cc @charettes).

felixxm · 2021-07-09T07:31:47Z

Unfortunately test_filter_lookup_lhs crashes on PostgreSQL sob , it's an issue similar to the 170b006. It seems that LHS should be wrapped in parentheses to respect operator precedence thinking (\cc @charettes).

Fixed 🥳

charettes · 2021-07-09T09:25:34Z

django/db/models/lookups.py

I wonder if a crash similar to 170b006 can happen if the lhs is a Subquery annotation.

Challenge accepted 😉 I added two more tests test_filter_exists_lhs and test_filter_subquery_lhs.

…ns, and QuerySet.filter(). Thanks Hannes Ljungberg and Simon Charette for reviews. Co-authored-by: Mariusz Felisiak <[email protected]>

LilyFirefly · 2021-07-09T14:53:21Z

Thanks for getting this over the line @felixxm and thanks @charettes, @hannseman and @pauloxnet for the reviews!

LilyFirefly mentioned this pull request Jun 5, 2021

Fixed #470 -- Support database defaults for fields #13709

Closed

LilyFirefly force-pushed the ticket_27021 branch from 1fb3f69 to a2d0b27 Compare June 5, 2021 14:03

hannseman reviewed Jun 5, 2021

View reviewed changes

django/db/models/lookups.py Outdated Show resolved Hide resolved

charettes reviewed Jun 5, 2021

View reviewed changes

django/db/models/expressions.py Outdated Show resolved Hide resolved

django/db/models/lookups.py Outdated Show resolved Hide resolved

django/db/models/lookups.py Outdated Show resolved Hide resolved

LilyFirefly force-pushed the ticket_27021 branch 3 times, most recently from 10fd9d6 to 3175ca5 Compare June 6, 2021 12:51

felixxm reviewed Jun 8, 2021

View reviewed changes

django/db/models/lookups.py Outdated Show resolved Hide resolved

LilyFirefly force-pushed the ticket_27021 branch 2 times, most recently from 4ae8d11 to 0e7ef74 Compare June 8, 2021 21:17

charettes reviewed Jun 9, 2021

View reviewed changes

django/db/models/lookups.py Outdated Show resolved Hide resolved

LilyFirefly force-pushed the ticket_27021 branch from 78ec728 to f84c77c Compare June 16, 2021 18:41

LilyFirefly force-pushed the ticket_27021 branch from f84c77c to 22ba28c Compare June 16, 2021 22:20

LilyFirefly force-pushed the ticket_27021 branch 2 times, most recently from 6b924a0 to 66aa6fd Compare June 16, 2021 22:49

felixxm reviewed Jun 24, 2021

View reviewed changes

django/db/models/fields/related_lookups.py Outdated Show resolved Hide resolved

docs/ref/models/lookups.txt Outdated Show resolved Hide resolved

docs/ref/models/lookups.txt Outdated Show resolved Hide resolved

felixxm force-pushed the ticket_27021 branch 2 times, most recently from a496b71 to ca95e5a Compare June 24, 2021 07:41

pauloxnet reviewed Jun 24, 2021

View reviewed changes

docs/ref/models/lookups.txt Outdated Show resolved Hide resolved

hannseman reviewed Jun 24, 2021

View reviewed changes

docs/ref/models/lookups.txt Outdated Show resolved Hide resolved

LilyFirefly force-pushed the ticket_27021 branch from 25102e9 to 5281e6e Compare June 24, 2021 21:07

felixxm reviewed Jun 25, 2021

View reviewed changes

felixxm force-pushed the ticket_27021 branch from 474814e to 1c9f8f3 Compare June 25, 2021 07:49

charettes approved these changes Jun 27, 2021

View reviewed changes

django/db/backends/oracle/operations.py Outdated Show resolved Hide resolved

LilyFirefly force-pushed the ticket_27021 branch from 1c9f8f3 to acd4d5e Compare July 7, 2021 11:39

felixxm force-pushed the ticket_27021 branch from 6d5e872 to cf319f0 Compare July 8, 2021 07:21

felixxm force-pushed the ticket_27021 branch from cf319f0 to 6956166 Compare July 8, 2021 07:30

felixxm changed the title ~~Fixed #27021 -- Support annotating lookups~~ Fixed #27021 -- Allowed lookup expressions in annotations, aggregations, and QuerySet.filter(). Jul 8, 2021

felixxm force-pushed the ticket_27021 branch from 6956166 to 27d5219 Compare July 8, 2021 10:46

felixxm force-pushed the ticket_27021 branch from 27d5219 to 455adac Compare July 9, 2021 07:31

felixxm force-pushed the ticket_27021 branch from 455adac to d9911bd Compare July 9, 2021 07:36

charettes reviewed Jul 9, 2021

View reviewed changes

Fixed #27021 -- Allowed lookup expressions in annotations, aggregatio…

f42ccdd

…ns, and QuerySet.filter(). Thanks Hannes Ljungberg and Simon Charette for reviews. Co-authored-by: Mariusz Felisiak <[email protected]>

felixxm force-pushed the ticket_27021 branch from d9911bd to f42ccdd Compare July 9, 2021 09:43

felixxm merged commit f42ccdd into django:main Jul 9, 2021

LilyFirefly deleted the ticket_27021 branch July 9, 2021 14:50

Uh oh!

Fixed #27021 -- Allowed lookup expressions in annotations, aggregations, and QuerySet.filter(). #14494

Fixed #27021 -- Allowed lookup expressions in annotations, aggregations, and QuerySet.filter(). #14494

Uh oh!

Conversation

LilyFirefly commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

charettes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charettes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charettes commented Jun 10, 2021

Uh oh!

LilyFirefly commented Jun 10, 2021

Uh oh!

charettes commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LilyFirefly commented Jun 16, 2021

Uh oh!

charettes commented Jun 16, 2021

Uh oh!

LilyFirefly commented Jun 16, 2021

Uh oh!

felixxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LilyFirefly commented Jun 24, 2021

Uh oh!

Uh oh!

felixxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

charettes commented Jun 27, 2021

Uh oh!

felixxm commented Jun 28, 2021

Uh oh!

felixxm commented Jul 7, 2021

Uh oh!

LilyFirefly commented Jul 7, 2021

Uh oh!

felixxm commented Jul 7, 2021

Uh oh!

LilyFirefly commented Jul 7, 2021

Uh oh!

felixxm commented Jul 7, 2021

Uh oh!

LilyFirefly commented Jul 7, 2021

Uh oh!

felixxm commented Jul 8, 2021

Uh oh!

felixxm commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixxm commented Jul 9, 2021

Uh oh!

charettes Jul 9, 2021

Choose a reason for hiding this comment

LilyFirefly commented Jun 5, 2021 •

edited

Loading

charettes commented Jun 16, 2021 •

edited

Loading

felixxm commented Jul 8, 2021 •

edited

Loading