Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expressions. #16710

charettes · 2023-03-31T17:50:32Z

Thanks Roman Odaisky for the report.

First commit addresses the issue, second one is an optimization for join promotion that was missing when passing expressions to filter().

tests/lookup/tests.py

rdaysky · 2023-03-31T19:29:44Z

tests/lookup/tests.py

+        self.assertEqual(
+            Season.objects.get(Exact(F("games__home"), "NY") & Q(games__away="Boston")),
+            self.s1,
+        )


Let’s make sure changing the order doesn’t affect anything the way it happened in the ticket

Suggested change

self.assertEqual(

Season.objects.get(Exact(F("games__home"), "NY") & Q(games__away="Boston")),

self.s1,

)

self.assertEqual(

Season.objects.get(Exact(F("games__home"), "NY") & Q(games__away="Boston")),

self.s1,

)

self.assertEqual(

Season.objects.get(Q(games__away="Boston") & Exact(F("games__home"), "NY")),

self.s1,

)

charettes · 2023-03-31T19:37:08Z

@rdaysky thanks for the suggestion but did you actually test the change to confirm they address the issue you reported?

charettes · 2023-03-31T18:51:57Z

django/db/models/sql/query.py

+            branch_negated=True,
+            current_negated=True,
+            can_reuse=can_reuse,


These kwargs were originally passed along but didn't carry any meaning since passing an expression to build_filter didn't make use of them until now.

Not doing so causes queries.tests.ManyToManyExcludeTest failures with the optimization enabled.

charettes · 2023-03-31T18:53:40Z

django/db/models/sql/query.py

+            pre_joins = self.alias_refcount.copy()
            condition = filter_expr.resolve_expression(
-                self, allow_joins=allow_joins, summarize=summarize
+                self, allow_joins=allow_joins, reuse=can_reuse, summarize=summarize
            )
            if not isinstance(condition, Lookup):
                condition = self.build_lookup(["exact"], condition, True)
-            return WhereNode([condition], connector=AND), []
+            used_joins = {
+                k for k, v in self.alias_refcount.items() if v > pre_joins.get(k, 0)
+            }


This pre_joins and used_joins logic is not novel, it's something used in the typical filtering by (lookup: str, value: Any) case below but was just never implemented in this case. I wouldn't be surprise if this related to other reported issues when doing filter(obj: Expression) calls.

tests/lookup/tests.py

charettes · 2023-03-31T19:36:19Z

tests/lookup/tests.py

+        self.assertEqual(
+            Season.objects.get(Exact(F("games__home"), "NY") & Q(games__away="Boston")),
+            self.s1,
+        )


charettes · 2023-03-31T19:41:57Z

django/db/models/sql/query.py

+            }
+            return (
+                WhereNode([condition], connector=AND),
+                used_joins if not current_negated else (),


The require_outer logic below is more complex so I think we'll want to add move coverage around usage of things like IsNull(expr, True) as well.

This logic is not flawless though as ticket-25946 demonstrates.

charettes · 2023-04-01T01:56:11Z

django/db/models/sql/query.py

+            require_outer = current_negated or (
+                isinstance(condition, IsNull)
+                and condition.rhs is True
+                and not current_negated
+            )


I don't think we can get this logic any smarter until we add proper support for Expression.nullable which is the same reason why ticket-25946 is blocked for lookup transforms and ticket-32398 is blocked for expression references.

Per Anssi's explanation on the topic

Currently, the only lookup that can match null values is isnull. However, it seems plausible that custom lookups can be used to create other lookups that match null values. This will be immediately obvious when expressions are allowed in the filter() clause. For example, .filter(Equal(Coalesce('favourite_book__title', 'first_book__title'), 'Foo')) must use left outer joins for the joins, as otherwise an author that has favourite_book with title
'Foo', but no first book would not be matched.

I think that's a strong enough rationale on why we can't merge the second commit here unfortunately.

In order to add proper support for this optimization we'd need to add a way to let the join promotion logic know that an expression has special null-handling semantics (e.g. IsNull, Coalesce) and have it taint expressions that contain them like we do with contains_aggregate and similar attributes.

Until we add support for that we'll have to live with LEFT OUTER joins when using expressions just like we do when filtering against annotations.

Agreed, let's remove the 2nd commit.

felixxm

@charettes Thanks 👍 Let's remove the 2nd commit and we're ready to go.

felixxm · 2023-04-04T05:44:13Z

django/db/models/sql/query.py

+            require_outer = current_negated or (
+                isinstance(condition, IsNull)
+                and condition.rhs is True
+                and not current_negated
+            )


Agreed, let's remove the 2nd commit.

felixxm · 2023-04-04T05:44:37Z

tests/lookup/tests.py

+        cls.g1 = Game.objects.create(season=cls.s1, home="NY", away="Boston")
+        cls.g2 = Game.objects.create(season=cls.s1, home="NY", away="Tampa")
+        cls.g3 = Game.objects.create(season=cls.s3, home="Boston", away="Tampa")


Suggested change

cls.g1 = Game.objects.create(season=cls.s1, home="NY", away="Boston")

cls.g2 = Game.objects.create(season=cls.s1, home="NY", away="Tampa")

cls.g3 = Game.objects.create(season=cls.s3, home="Boston", away="Tampa")

Game.objects.create(season=cls.s1, home="NY", away="Boston")

Game.objects.create(season=cls.s1, home="NY", away="Tampa")

Game.objects.create(season=cls.s3, home="Boston", away="Tampa")

…ssions. Thanks Roman Odaisky for the report.

This required adjusting changes made in 8593e16 (refs #32143) to pass former build_filter() negation and reusable joins kwargs to ensure proper join promotion to outer on multi-valued exclusions.

charettes · 2023-04-04T11:40:03Z

@felixxm I've pushed an adjusted first commit but kept the second one in the PR for archival purposes since it might provide historical context on why some sub-optimal LEFT JOINs are generated when using lookups directly.

Is that okay with you if we only merge the first commit and close the PR afterwards? I can create a Trac issue for it and link it to ticket-25946 and ticket-32398 since they all relate to each other otherwise.

felixxm · 2023-04-04T11:47:42Z

@felixxm I've pushed an adjusted first commit but kept the second one in the PR for archival purposes since it might provide historical context on why some sub-optimal LEFT JOINs are generated when using lookups directly.

Is that okay with you if we only merge the first commit and close the PR afterwards? I can create a Trac issue for it and link it to ticket-25946 and ticket-32398 since they all relate to each other otherwise.

Yes, it works for me. I opened a separate PR (#16721) to double-check with CI that everything is green with the first commit. I'm going to close it as soon as CI confirms.

felixxm · 2023-04-04T12:37:01Z

Merged the 1st commit in 0e1aae7.

charettes commented Mar 31, 2023

View reviewed changes

tests/lookup/tests.py Outdated Show resolved Hide resolved

rdaysky reviewed Mar 31, 2023

View reviewed changes

tests/lookup/tests.py Outdated Show resolved Hide resolved

rdaysky reviewed Mar 31, 2023

View reviewed changes

charettes force-pushed the ticket-34450 branch from 4e0e21b to fd44f08 Compare March 31, 2023 19:32

charettes commented Mar 31, 2023

View reviewed changes

charettes force-pushed the ticket-34450 branch from fd44f08 to e2f43e4 Compare April 1, 2023 01:51

charettes commented Apr 1, 2023

View reviewed changes

felixxm reviewed Apr 4, 2023

View reviewed changes

charettes added 2 commits April 4, 2023 07:37

Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expre…

85779f3

…ssions. Thanks Roman Odaisky for the report.

Refs #34450 -- Optimized JOIN demotion when filtering by expression.

b42ef2c

This required adjusting changes made in 8593e16 (refs #32143) to pass former build_filter() negation and reusable joins kwargs to ensure proper join promotion to outer on multi-valued exclusions.

charettes force-pushed the ticket-34450 branch from e2f43e4 to b42ef2c Compare April 4, 2023 11:38

felixxm closed this Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expressions. #16710

Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expressions. #16710

charettes commented Mar 31, 2023 •

edited

rdaysky Mar 31, 2023

charettes Mar 31, 2023

charettes commented Mar 31, 2023

charettes Mar 31, 2023 •

edited

charettes Mar 31, 2023

charettes Mar 31, 2023

charettes Mar 31, 2023 •

edited

charettes Apr 1, 2023 •

edited

charettes Apr 1, 2023 •

edited

felixxm Apr 4, 2023

felixxm left a comment

felixxm Apr 4, 2023

felixxm Apr 4, 2023

charettes commented Apr 4, 2023 •

edited

felixxm commented Apr 4, 2023

felixxm commented Apr 4, 2023

Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expressions. #16710

Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by expressions. #16710

Conversation

charettes commented Mar 31, 2023 • edited

rdaysky Mar 31, 2023

Choose a reason for hiding this comment

charettes Mar 31, 2023

Choose a reason for hiding this comment

charettes commented Mar 31, 2023

charettes Mar 31, 2023 • edited

Choose a reason for hiding this comment

charettes Mar 31, 2023

Choose a reason for hiding this comment

charettes Mar 31, 2023

Choose a reason for hiding this comment

charettes Mar 31, 2023 • edited

Choose a reason for hiding this comment

charettes Apr 1, 2023 • edited

Choose a reason for hiding this comment

charettes Apr 1, 2023 • edited

Choose a reason for hiding this comment

felixxm Apr 4, 2023

Choose a reason for hiding this comment

felixxm left a comment

Choose a reason for hiding this comment

felixxm Apr 4, 2023

Choose a reason for hiding this comment

felixxm Apr 4, 2023

Choose a reason for hiding this comment

charettes commented Apr 4, 2023 • edited

felixxm commented Apr 4, 2023

felixxm commented Apr 4, 2023

charettes commented Mar 31, 2023 •

edited

charettes Mar 31, 2023 •

edited

charettes Mar 31, 2023 •

edited

charettes Apr 1, 2023 •

edited

charettes Apr 1, 2023 •

edited

charettes commented Apr 4, 2023 •

edited