First, thanks for all your hard work in this area Josh.
>From a quick look at the code, my understanding is that output_field can be
None, in which case the ORM will use the model's field type, or it can be
specified explicitly in the query.
I think that if specified explicitly, any constraints should be honoured
and an exception raised if the computed aggregate value violates a
constraint. If it's not specified explicitly, any constraints on the model
field should be ignored as per your reasoning above. I should be able to
write Model.objects.aggregate(sum_price=Sum('price')), where the sum value
violates the constraints, without any error.
That provides sensible default behaviour which can be easily overridden if
necessary.
Cheers,
Alex
On Friday, December 5, 2014 10:04:21 AM UTC+8, Josh Smeaton wrote:
>
> Trac user jbg has found a regression with the aggregation/annotation
> refactor #14030. The crux of the problem comes down to whether annotations
> and aggregations should respect constraints on model fields. The specific
> example raised is as follows:
>
> Model.objects.aggregate(
> sum_price=Sum('price', output_field=DecimalField(max_digits=5,
> decimal_places=2)))
>
> Assuming the field "price" is defined as "DecimalField(max_digits=5,
> decimal_places=2)", it doesn't matter if output_field is defined or not,
> as the implicit F('price') will result in exactly the same behaviour.
>
> Each row in the table will adhere to the max_digits and decimal_places
> restriction. It has to, it's part of the column definition. But once you
> sum those values together, they are going to overflow. The same problem
> would exist if you were to concatenate two CharFields with a max_length=10.
>
> Ticket https://code.djangoproject.com/ticket/23941 has been opened to
> track this.
>
> These constraints do not necessarily make sense because we're not
> persisting these values anywhere. They are read-only computations, and
> should not require bounds. If these values were to be persisted in another
> model, then the input fields on that model are required to ensure the
> values are valid.
>
> I've implemented https://github.com/django/django/pull/3655/
> <https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2Fdjango%2Fdjango%2Fpull%2F3655%2F&sa=D&sntz=1&usg=AFQjCNG29EDZTLKdvo0QGAlSTo6_NlXF1Q>
>
> which effectively ignores the max_digits and decimal_places arguments in
> the base expression class. Subclasses are free to enforce validation if it
> is necessary. Further, callers using aren't required to supply bounds
> checking in this situation. This is valid (with my patch):
>
> Model.objects.aggregate(
> sum_price=Sum('price', output_field=DecimalField()))
>
> And the same goes with CharField() not requiring a max_length argument.
>
> Can anyone see any issues with this approach?
>
> If the above seems sensible, I'll alter the test cases to remove arguments
> like decimal_places and max_length, and update the documentation to call
> out that these kinds of arguments are not used for annotations.
>
> Thanks,
>
> Josh
>
>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/3c59d8f2-e771-477c-abd6-9079b182d318%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
