Opened 8 years ago

Last modified 7 years ago

#26650 new Cleanup/optimization

Automatically apply Cast based on output_field

Reported by: Matt C Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: output_field expression query annotate aggregate
Cc: Sergey Fedoseev Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given the following model:

import django.db.models

class TempModel(django.db.models.Model):
    field1 = django.db.models.DecimalField(max_digits=12, decimal_places=2)

...when I run a query like so:

TempModel.objects.all().aggregate(
    my_sum=django.db.models.Sum(
        'field1', output_field=django.db.models.DateTimeField()
    )
)

...the result produces a Decimal result, no matter what output_field is specified.

I came across this issue by simply wanting to specify the number of decimal places that would get output in queries such as this.
That led me to find #23941, which shocked me in seeing that the "sane" behaviour/implementation was replaced with something that breaks down/invalidates the API.

Then I tried the query above, with all different kinds of output_field values and realised the problem is more systemic and severe, in the sense that specifying output_field is often pointless/futile.

Why can't the implementation actually honour the output_field parameter and construct SQL to cast/coerce fields into the appropriate DB field types?

The only other means (in which I am aware of) for ensuring that a query gets compiled to SQL that honours the clients' field type specifications is to use .extra() or .raw(), but then that is no longer DB agnostic.
I would have thought it was the job of output_field to achieve this in a DB agnostic manner, utilising the power of the ORM.

Change History (6)

comment:1 by Simon Charette, 8 years ago

Did you try using the recently introduced Cast() expression?

TempModel.objects.aggregate(
    my_sum=Cast(Sum('field1'), DecimalField(max_digits=30, decimal_places=2)),
)

comment:2 by Matt C, 8 years ago

Thank you, I was not aware of the feature.

Cast() does address my issue.

So it appears that instead of respecting output_field as I proposed in the description of this ticket, the implementation has just tacked on more API, rather than refactoring the implementation of the existing API.

If the core devs are happy travelling down this path, I guess this ticket has nothing further to offer and hence can be closed.

comment:3 by Simon Charette, 8 years ago

I guess it could be worth investigating if a Cast should be added when an output_field is explicitly specified.

comment:4 by Matt C, 8 years ago

My suggestion would be to use the implementation of Cast() everywhere that output_field is used and to get rid of Cast() from the public API.

I.e. Why does Cast() have to be restricted to SQL functions?

comment:5 by Tim Graham, 8 years ago

Summary: output_field key-word arg not respected in model query APIAutomatically apply Cast based on output_field
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Accepting for further investigation.

comment:6 by Sergey Fedoseev, 7 years ago

Cc: Sergey Fedoseev added
Note: See TracTickets for help on using tickets.
Back to Top