Remove subqueries from group by. Fixes #343 (#344)

* collapse group by considers subexpressions in order to remove subquery expressions and avoid query failing

* method renamed to make it "protected".

* constants are removed from group by because sql server does not allow them

* a django test that now passes is not excluded anymore

Author: federico-martinez-invgate

mssql/compiler.py

@@ -155,7 +155,7 @@ def _as_sql_window(self, compiler, connection, template=None):
     else:
         # MSSQL window functions require an OVER clause with ORDER BY
         window_sql.append('ORDER BY (SELECT NULL)')
-    
+
     if self.frame:
         frame_sql, frame_params = compiler.compile(self.frame)
         window_sql.append(frame_sql)
@@ -443,7 +443,45 @@ def compile(self, node, *args, **kwargs):
 
     def collapse_group_by(self, expressions, having):
         expressions = super().collapse_group_by(expressions, having)
-        return [e for e in expressions if not isinstance(e, Subquery)]
+        # SQL server does not allow subqueries or constant expressions in the group by
+        # For constants: Each GROUP BY expression must contain at least one column that is not an outer reference.
+        # For subqueries: Cannot use an aggregate or a subquery in an expression used for the group by list of a GROUP BY clause.
+        return self._filter_subquery_and_constant_expressions(expressions)
+
+    def _is_constant_expression(self, expression):
+        if isinstance(expression, Value):
+            return True
+        sub_exprs = expression.get_source_expressions()
+        if not sub_exprs:
+            return False
+        for each in sub_exprs:
+            if not self._is_constant_expression(each):
+                return False
+        return True
+
+
+
+    def _filter_subquery_and_constant_expressions(self, expressions):
+        ret = []
+        for expression in expressions:
+            if self._is_subquery(expression):
+                continue
+            if self._is_constant_expression(expression):
+                continue
+            if not self._has_nested_subquery(expression):
+                ret.append(expression)
+        return ret
+
+    def _has_nested_subquery(self, expression):
+        if self._is_subquery(expression):
+            return True
+        for sub_expr in expression.get_source_expressions():
+            if self._has_nested_subquery(sub_expr):
+                return True
+        return False
+
+    def _is_subquery(self, expression):
+        return isinstance(expression, Subquery)
 
     def _as_microsoft(self, node):
         as_microsoft = None

testapp/settings.py

@@ -168,7 +168,6 @@
     'expressions.tests.FTimeDeltaTests.test_time_subquery_subtraction',
     'migrations.test_operations.OperationTests.test_alter_field_reloads_state_on_fk_with_to_field_target_type_change',
     'schema.tests.SchemaTests.test_alter_smallint_pk_to_smallautofield_pk',
-    'annotations.tests.NonAggregateAnnotationTestCase.test_combined_expression_annotation_with_aggregation',
     'db_functions.datetime.test_extract_trunc.DateFunctionTests.test_extract_func',
     'db_functions.datetime.test_extract_trunc.DateFunctionTests.test_extract_iso_weekday_func',
     'db_functions.datetime.test_extract_trunc.DateFunctionWithTimeZoneTests.test_extract_func',

testapp/tests/test_expressions.py

@@ -5,10 +5,10 @@
 
 from django import VERSION
 from django.db.models import IntegerField, F
-from django.db.models.expressions import Case, Exists, OuterRef, Subquery, Value, When
+from django.db.models.expressions import Case, Exists, OuterRef, Subquery, Value, When, ExpressionWrapper
 from django.test import TestCase, skipUnlessDBFeature
 
-
+from django.db.models.aggregates import Count
 from ..models import Author, Comment, Post, Editor
 
 DJANGO3 = VERSION[0] >= 3
@@ -47,6 +47,32 @@ def test_with_case_when(self):
         ).get()
         self.assertEqual(author.has_post, 1)
 
+    def test_unnecessary_exists_group_by(self):
+        author = Author.objects.annotate(
+            has_post=Case(
+                When(Exists(Post.objects.filter(author=OuterRef('pk')).values('pk')), then=Value(1)),
+                default=Value(0),
+                output_field=IntegerField(),
+            )).annotate(
+            amount=Count("post")
+        ).get()
+        self.assertEqual(author.amount, 1)
+        self.assertEqual(author.has_post, 1)
+
+    def test_combined_expression_annotation_with_aggregation(self):
+        book = Author.objects.annotate(
+            combined=ExpressionWrapper(
+                Value(2) * Value(5), output_field=IntegerField()
+            ),
+            null_value=ExpressionWrapper(
+                Value(None), output_field=IntegerField()
+            ),
+            rating_count=Count("post"),
+        ).first()
+        self.assertEqual(book.combined, 10)
+        self.assertEqual(book.null_value, None)
+
+
     @skipUnless(DJANGO3, "Django 3 specific tests")
     def test_order_by_exists(self):
         author_without_posts = Author.objects.create(name="other author")