Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#31651 closed Bug (fixed)

Constant expressions of an ExpressionWrapper object are incorrectly placed at the GROUP BY clause

Reported by: Thodoris Sotiropoulos Owned by: nobody
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I have a function that expects an arbitrary Query expression and constructs a query on a Postgres db

  def execQuery(expr):
      expr = ExpressionWrapper(expr, output_field=IntegerField())
      return Model.objects.annotate(expr_res=expr).values('expr_res', 'column_a').annotate(sum=Sum('column_b'))

However, when the given expr is a constant expression (e.g., Value(3)), Django generates an SQL query that contains this constant expression in its GROUP BY clause.

SELECT "model"."column_a", 3 AS "expr_res", SUM("model"."column_b") AS "sum" FROM "model" GROUP BY "model"."column_a", 3

This leads to an exception because in Postgres, the query above is invalid:

django.db.utils.ProgrammingError: aggregate functions are not allowed in GROUP BY
LINE 1: SELECT "model"."column_a", 3 AS "expr_res", SUM("model"."col...

Note that when the given query expression is not wrapped by the ExpressionWrapper object, Django correctly identifies and omits the constant from the GROUP BY clause. For example, the query below runs correctly.

  def execQuery(expr):
      return Model.objects.annotate(expr_res=Value(3, output_field=IntegerField())).values('expr_res', 'column_a').annotate(sum=Sum('column_b'))
SELECT "model"."column_a", 3 AS "expr_res", SUM("model"."column_b") AS "sum" FROM "model" GROUP BY "model"."column_a"

Change History (7)

comment:1 by Simon Charette, 5 years ago

Component: UncategorizedDatabase layer (models, ORM)
Has patch: set
Needs tests: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Can you confirm the following patch against master resolves your issue?

diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index c1a76584f0..6bd1471692 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -863,6 +863,9 @@ class ExpressionWrapper(Expression):
     def get_source_expressions(self):
         return [self.expression]

+    def get_group_by_cols(self, alias=None):
+        return self.expression.get_group_by_cols(alias=alias)
+
     def as_sql(self, compiler, connection):
         return self.expression.as_sql(compiler, connection)

The underlying issue is that BaseExpression.get_group_by_cols which ExpressionWrapper inherits defaults to returning [self] when it doesn't contain aggregates while Value.get_group_by_cols is overridden to return []. Deferring grouping column resolution to the wrapped expression makes more sense IMO.

comment:2 by Thodoris Sotiropoulos, 5 years ago

Indeed, the patch above resolves the issue!

comment:3 by Simon Charette, 5 years ago

Easy pickings: set

Would you be interested in submitting a PR with the above patch and a regression test in tests/expression/tests.py?

A SimpleTestCase subclass with a test method that creates a ExpressionWrapper(Value(42)) and asserts the return value of get_group_by_cols(alias=None) is [] should do!

comment:4 by Thodoris Sotiropoulos, 5 years ago

comment:5 by Simon Charette, 5 years ago

Needs tests: unset

Patch seems good to me given the comments are addressed.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: newclosed

In df32fd4:

Fixed #31651 -- Made ExpressionWrapper use grouping columns from wrapped expression.

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In fdd2b01:

[3.1.x] Fixed #31651 -- Made ExpressionWrapper use grouping columns from wrapped expression.

Backport of df32fd42b84cc6dbba173201f244491b0d154a63 from master

Note: See TracTickets for help on using tickets.
Back to Top