#24233: group-by column construction is broken
-------------------------------------+-------------------------------------
Reporter: megatrump | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.8alpha1
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by jarshwah):
* status: new => closed
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* resolution: => wontfix
* stage: Unreviewed => Accepted
Comment:
Yes, I believe this change was introduced with this PR
https://github.com/django/django/pull/3669, but it may have been another.
Grouping was pretty broken when using complicated expressions, because the
backing columns were being added to the GROUP BY rather than the entire
expression. It was lucky you were getting the right results.
Now, the reason this is failing for you now, is because you're effectively
writing RawSQL (there's a private expression type for this), without any
of the benefits of new expressions. You may as well be writing `.extra()`
calls. To fully leverage the power of expressions, you should use the
smaller types to build up bigger ones. Expressions know that if they
reference an aggregate, then they shouldn't be added to the group by
clause. But you've just stuck a raw SUM expression into the roi
calculation, and Django can't know that it's now referencing an aggregate.
I've replicated (mostly..) your query only using full expressions. I
cheated with a couple (using Func(function='..')) rather than building out
the types fully, but they'll serve for now. I've broken each part down
into logical chunks, and have printed the underlying query at each stage
to make this easier to follow.
First, annotate with the day of week. Annotate can do everything extra can
do with regards to adding columns to the select list:
{{{
In [30]: qs = Costing.objects.annotate(dow=Func(F('ts'), Value('D'),
function='TO_CHAR')).values('dow')
In [31]: print(qs.query)
SELECT TO_CHAR("scratch_costing"."ts", D) AS "dow" FROM "scratch_costing"
}}}
Next, add the aggregates we're interested in:
{{{
In [32]: qs = qs.annotate(sum_cost=Sum('cost'), sum_rev=Sum('rev'))
In [33]: print(qs.query)
SELECT TO_CHAR("scratch_costing"."ts", D) AS "dow",
SUM("scratch_costing"."cost") AS "sum_cost", SUM("scratch_costing"."rev")
AS "sum_rev" FROM "scratch_costing" GROUP BY
TO_CHAR("scratch_costing"."ts", D)
}}}
Now, the ROI calculation. Note, this could be improved by using
[https://docs.djangoproject.com/en/dev/ref/models/conditional-expressions/
Case/When expressions]
{{{
In [34]: qsroi = qs.annotate(roi=Func( Coalesce(F('sum_rev') /
Coalesce('sum_cost', Value(0)), Value(0)) , Value(2), function='ROUND',
output_field=FloatField()))
In [35]: print(qsroi.query)
SELECT TO_CHAR("scratch_costing"."ts", D) AS "dow",
SUM("scratch_costing"."cost") AS "sum_cost", SUM("scratch_costing"."rev")
AS "sum_rev", ROUND(COALESCE((SUM("scratch_costing"."rev") /
COALESCE(SUM("scratch_costing"."cost"), 0)), 0), 2) AS "roi" FROM
"scratch_costing" GROUP BY TO_CHAR("scratch_costing"."ts", D)
In [36]: roi = qsroi[0]
In [37]: print(roi)
{'roi': 0.54, 'sum_cost': Decimal('30.30'), 'sum_rev': Decimal('16.50'),
'dow': u'4'}
}}}
And a quick Case/When example, to show how you could avoid doing division
by NULL or 0:
{{{
In [39]: class Round(Func):
....: function = 'ROUND'
....:
In [40]: qsroi =
qs.annotate(roi=Round(Case(When(Q(sum_cost__isnull=True)|Q(sum_cost=0),
then='sum_rev'), default=F('sum_rev')/F('sum_cost')), Value(2),
output_field=FloatField()))
In [41]: print(qsroi[0])
{'roi': 0.54, 'sum_cost': Decimal('30.30'), 'sum_rev': Decimal('16.50'),
'dow': u'4'}
In [42]: print(qsroi.query)
SELECT TO_CHAR("scratch_costing"."ts", D) AS "dow",
SUM("scratch_costing"."cost") AS "sum_cost", SUM("scratch_costing"."rev")
AS "sum_rev", ROUND(CASE WHEN (SUM("scratch_costing"."cost") IS NULL OR
SUM("scratch_costing"."cost") = 0) THEN SUM("scratch_costing"."rev") ELSE
(SUM("scratch_costing"."rev") / SUM("scratch_costing"."cost")) END, 2) AS
"roi" FROM "scratch_costing" GROUP BY TO_CHAR("scratch_costing"."ts", D)
}}}
I've probably put a little too much detail above, but hopefully it'll help
you solve your problem in a supported way. I'm going to close this as
wontfix, because the types aren't being used in the right way. If the
documentation for the expressions or function types needs to be improved,
please reopen with some comments on where the clarification is required,
and I'll work on updating them to make it clearer.
--
Ticket URL: <https://code.djangoproject.com/ticket/24233#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.92bb02f34629cd2cc14729a53bb1e89e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
