Improved Query join promotion logic

There were multiple cases where join promotion was a bit too aggressive.
This resulted in using outer joins where not necessary.

Refs #21150.

3 files changed, 54 insertions, 18 deletions

diff --git a/tests/aggregation_regress/models.py b/tests/aggregation_regress/models.py
index 24859f4ce4..a2dc060640 100644
--- a/tests/aggregation_regress/models.py
+++ b/tests/aggregation_regress/models.py
@@ -90,7 +90,7 @@ class HardbackBook(Book):
 
 # Models for ticket #21150
 class Alfa(models.Model):
-    pass
+    name = models.CharField(max_length=10, null=True)
 
 class Bravo(models.Model):
     pass
diff --git a/tests/aggregation_regress/tests.py b/tests/aggregation_regress/tests.py
index 82be5afab2..9fcaefe6f5 100644
--- a/tests/aggregation_regress/tests.py
+++ b/tests/aggregation_regress/tests.py
@@ -12,9 +12,8 @@ from django.test import TestCase, skipUnlessDBFeature
 from django.test.utils import Approximate
 from django.utils import six
 
-from .models import (
-    Author, Book, Publisher, Clues, Entries, HardbackBook, ItemTag,
-    WithManualPK, Alfa, Bravo, Charlie)
+from .models import (Author, Book, Publisher, Clues, Entries, HardbackBook,
+        ItemTag, WithManualPK, Alfa, Bravo, Charlie)
 
 
 class AggregationTests(TestCase):
@@ -1137,6 +1136,8 @@ class AggregationTests(TestCase):
             'select__avg': Approximate(1.666, places=2),
         })
 
+
+class JoinPromotionTests(TestCase):
     def test_ticket_21150(self):
         b = Bravo.objects.create()
         c = Charlie.objects.create(bravo=b)
@@ -1152,3 +1153,19 @@ class AggregationTests(TestCase):
         self.assertQuerysetEqual(
             qs, [c], lambda x: x)
         self.assertEqual(qs[0].alfa, a)
+
+    def test_existing_join_not_promoted(self):
+        # No promotion for existing joins
+        qs = Charlie.objects.filter(alfa__name__isnull=False).annotate(Count('alfa__name'))
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        # Also, the existing join is unpromoted when doing filtering for already
+        # promoted join.
+        qs = Charlie.objects.annotate(Count('alfa__name')).filter(alfa__name__isnull=False)
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        # But, as the join is nullable first use by annotate will be LOUTER
+        qs = Charlie.objects.annotate(Count('alfa__name'))
+        self.assertTrue(' LEFT OUTER JOIN ' in str(qs.query))
+
+    def test_non_nullable_fk_not_promoted(self):
+        qs = Book.objects.annotate(Count('contact__name'))
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
diff --git a/tests/queries/tests.py b/tests/queries/tests.py
index 7bb7a150a2..a7b0af55a3 100644
--- a/tests/queries/tests.py
+++ b/tests/queries/tests.py
@@ -2689,6 +2689,15 @@ class NullJoinPromotionOrTest(TestCase):
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
         self.assertEqual(list(qs), [self.a2])
 
+    def test_null_join_demotion(self):
+        qs = ModelA.objects.filter(Q(b__name__isnull=False) & Q(b__name__isnull=True))
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        qs = ModelA.objects.filter(Q(b__name__isnull=True) & Q(b__name__isnull=False))
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        qs = ModelA.objects.filter(Q(b__name__isnull=False) | Q(b__name__isnull=True))
+        self.assertTrue(' LEFT OUTER JOIN ' in str(qs.query))
+        qs = ModelA.objects.filter(Q(b__name__isnull=True) | Q(b__name__isnull=False))
+        self.assertTrue(' LEFT OUTER JOIN ' in str(qs.query))
 
 class ReverseJoinTrimmingTest(TestCase):
     def test_reverse_trimming(self):
@@ -2785,22 +2794,19 @@ class DisjunctionPromotionTests(TestCase):
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
         self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 1)
 
-    @unittest.expectedFailure
-    def test_disjunction_promotion3_failing(self):
-        # Now the ORed filter creates LOUTER join, but we do not have
-        # logic to unpromote it for the AND filter after it. The query
-        # results will be correct, but we have one LOUTER JOIN too much
-        # currently.
+    def test_disjunction_promotion3_demote(self):
+        # This one needs demotion logic: the first filter causes a to be
+        # outer joined, the second filter makes it inner join again.
         qs = BaseA.objects.filter(
             Q(a__f1='foo') | Q(b__f2='foo')).filter(a__f2='bar')
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
         self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 1)
 
-    @unittest.expectedFailure
-    def test_disjunction_promotion4_failing(self):
-        # Failure because no join repromotion
+    def test_disjunction_promotion4_demote(self):
         qs = BaseA.objects.filter(Q(a=1) | Q(a=2))
         self.assertEqual(str(qs.query).count('JOIN'), 0)
+        # Demote needed for the "a" join. It is marked as outer join by
+        # above filter (even if it is trimmed away).
         qs = qs.filter(a__f1='foo')
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
 
@@ -2810,9 +2816,8 @@ class DisjunctionPromotionTests(TestCase):
         qs = qs.filter(Q(a=1) | Q(a=2))
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
 
-    @unittest.expectedFailure
-    def test_disjunction_promotion5_failing(self):
-        # Failure because no join repromotion logic.
+    def test_disjunction_promotion5_demote(self):
+        # Failure because no join demotion logic for this case.
         qs = BaseA.objects.filter(Q(a=1) | Q(a=2))
         # Note that the above filters on a force the join to an
         # inner join even if it is trimmed.
@@ -2823,8 +2828,8 @@ class DisjunctionPromotionTests(TestCase):
         self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 1)
         qs = BaseA.objects.filter(Q(a__f1='foo') | Q(b__f1='foo'))
         # Now the join to a is created as LOUTER
-        self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 0)
-        qs = qs.objects.filter(Q(a=1) | Q(a=2))
+        self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 2)
+        qs = qs.filter(Q(a=1) | Q(a=2))
         self.assertEqual(str(qs.query).count('INNER JOIN'), 1)
         self.assertEqual(str(qs.query).count('LEFT OUTER JOIN'), 1)
 
@@ -3079,3 +3084,17 @@ class Ticket21203Tests(TestCase):
         qs = Ticket21203Child.objects.select_related('parent').defer('parent__created')
         self.assertQuerysetEqual(qs, [c], lambda x: x)
         self.assertIs(qs[0].parent.parent_bool, True)
+
+class ValuesJoinPromotionTests(TestCase):
+    def test_values_no_promotion_for_existing(self):
+        qs = Node.objects.filter(parent__parent__isnull=False)
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        qs = qs.values('parent__parent__id')
+        self.assertTrue(' INNER JOIN ' in str(qs.query))
+        # Make sure there is a left outer join without the filter.
+        qs = Node.objects.values('parent__parent__id')
+        self.assertTrue(' LEFT OUTER JOIN ' in str(qs.query))
+
+    def test_non_nullable_fk_not_promoted(self):
+        qs = ObjectB.objects.values('objecta__name')
+        self.assertTrue(' INNER JOIN ' in str(qs.query))