Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  Josh Smeaton |Owner:  Josh
 |  Smeaton
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Release blocker  |   Resolution:  fixed
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Sardorbek Imomaliev):

 * cc: Sardorbek Imomaliev (added)


-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.f141386af1baed478436e97b4c30fbc7%40djangoproject.com.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  Josh Smeaton |Owner:  Josh
 |  Smeaton
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Release blocker  |   Resolution:  fixed
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham ):

 In [changeset:"ebc4ee3369694e6dca5cf216d4176bdefd930fd6" ebc4ee33]:
 {{{
 #!CommitTicketReference repository=""
 revision="ebc4ee3369694e6dca5cf216d4176bdefd930fd6"
 Refs #23941 -- Prevented incorrect rounding of DecimalField annotations on
 SQLite.
 }}}

-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.0f9c4e11706bc9372d9934ec5b0b70fd%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:  fixed
 Severity:  Release blocker  | Triage Stage:  Ready for
 Keywords:   |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Tim Graham ):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"267a1dcd9b7877d97917ba6222fd247da060f9b0"]:
 {{{
 #!CommitTicketReference repository=""
 revision="267a1dcd9b7877d97917ba6222fd247da060f9b0"
 Fixed #23941 -- Removed implicit decimal formatting from expressions.
 }}}

--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.75769f29a73f3ef5aecdf692c4659818%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Ready for
 Keywords:   |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by jarshwah):

 * needs_better_patch:  1 => 0
 * stage:  Accepted => Ready for checkin


--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.7ac5a440719a38e3b2ba451422eef371%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by carljm):

 (I guess on further review, we seem to have made a lot of previously-
 required arguments to field instantiation optional now, so the final
 concern there isn't as much of an issue.)

--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.449ab2d369c2249d0a6ba0ed738ab513%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by carljm):

 It seems this issue is maybe being presented as broader than it really is?
 We don't ever do "validation" (in the Django sense) on annotation outputs,
 we just convert from db value to Python value. We have one special case
 with `DecimalField` where that conversion itself can actually raise an
 exception. The problem is not with validation, the problem is that
 `max_digits` is not just a validation, it is a hard limit on the value
 that the field knows how to represent (due to its use in setting the
 decimal context precision). There is no parallel problem with e.g.
 `max_length` on a `CharField` (or with any other field with validation
 constraints), because the `max_length` (or any other validation
 constraint) isn't checked anywhere in the conversion from DB value to
 Python value. So in a way, this is simply a bug in `DecimalField` (or
 perhaps in the way an internal-type of `DecimalField` is special-cased in
 `ExpressionNode.convert_value`), in that its "validation" constraints are
 applied (and result in hard errors) in data conversion, where normally
 field validation does not apply at all.

 (As an aside, it also seems a little odd to me that
 `ExpressionNode.convert_value` assumes that any field with internal-type
 of `DecimalField` will have a `format_number` method, when
 `DecimalField.format_number()` has a docstring indicating that it only
 exists for legacy code.)

 In the case where no explicit `output_field` is given for an annotation,
 we should definitely not apply the `max_digits` or `decimal_places` values
 from the source field to the result; we should simply convert to Decimal
 and return.

 The trickier case is where an explicit `output_field` was given. I hear
 Anssi's argument that we should respect the `max_digits` and
 `decimal_places` in that case, otherwise we are "lying" to the user, but
 I'm not convinced. Why should `DecimalField` be special? If you specify a
 `CharField` as `output_field`, its `max_length` will not be respected in
 converting from db to Python. If you specify a `PositiveIntegerField` as
 `output_field` for an expression, you can get a negative result with no
 error, etc. Conversion is not the same thing as validation.

 It seems to me that the idea of applying field validation to the output of
 an expression would be a major new feature addition that would need quite
 a lot of separate discussion to figure out how it would work, or if its
 even a good idea (there is currently no such thing as getting
 `ValidationError` from attempting to do a database query in Django). In
 the meantime, it's simply a bug (and a regression) that such validation is
 sort-of applied in the case of `DecimalField`, and the correct fix is to
 just stop doing that, as the current pull request does.

 This does leave us in the odd situation where specifying `output_field`
 sometimes requires that you provide field parameters which are effectively
 ignored. I think the ideal API for this would be to allow (or require?)
 `output_field` to be a field class, not a field instance. But that would
 require making `get_internal_type` a classmethod rather than an instance
 method on Fields. Which perhaps it should be, but that's also a major
 invasive change for third-party field classes.

--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.f804ccf7ef7528ef6aab4ffceed8ccb7%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Bug  |   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by timgraham):

 * type:  Uncategorized => Bug
 * stage:  Unreviewed => Accepted


--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.4d8faa8f6daa1501999cb95a26a72346%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Uncategorized|   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by jarshwah):

 As discussed on IRC - the other issue is that when a user declares the
 output_field to be DecimalField(digits=3, decimals=2), and we don't honour
 that by throwing an exception, it feels like a lie. We're ignoring a
 request from the user.

 However, if the output_field is derived from the backing field
 F('the_price'), then the user hasn't explicitly asked for a return type,
 but it'll be enforced anyway. Also, DecimalField requires the max_digits
 and decimal_places arguments, and the user can't be expected to know the
 bounds of the data, in code, especially for the future.

 I could see a couple of options.

 1. Ignore all model field "validation" for annotations. The data type and
 size is dynamic based on the (changing) dataset, and therefore doesn't
 require strict validation like a column would.
 2. Special case DecimalField(max_digits='?', decimal_places='?') for the
 purposes of annotations. This sounds really confusing though, and starts
 to get very complicated.

 I'm open to suggestions.

--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.691a66b5d28730a4237d3b8dff48071c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  jarshwah
 Type:  Uncategorized|   Status:  assigned
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by jarshwah):

 * status:  new => assigned
 * needs_better_patch:  0 => 1
 * has_patch:  0 => 1
 * owner:  nobody => jarshwah


--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.e5c4ddc90acbfc37cdc9e2f255cc745e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #23941: Aggregating over decimal field regression

[Django]

#23941: Aggregating over decimal field regression
-+-
 Reporter:  jarshwah |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by jarshwah):

 * needs_better_patch:   => 0
 * needs_tests:   => 0
 * needs_docs:   => 0


Comment:

 See https://github.com/django/django/pull/3655

 The patch ignores decimal_places/max_digits for annotations, which don't
 make sense for aggregation. I'm concerned this may have an impact on
 regular annotations though, so I'd like to discuss the impact.

 It should be easy enough to move the decimal convert_value path to the
 aggregate base class if annotations would be negatively affected.

--
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.8cd528ef594ebd221c43d65e70476e65%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.