[Django] #22128: The Aggregation guide should warn that this feature does not work when using the sqlite backend and certain types
Django
using the sqlite backend and certain types
--------------------------------------+--------------------
Reporter: zr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Keywords: sqlite
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+--------------------
When using aggregation clauses like "HAVING SUM" in sqlite3, SUM is a
function and therefore does not have affinity. Therefore, the correct type
must be used in the statement, or it might yield incorrect results:
{{{
#!python
>>> len(list(cursor.execute("SELECT * from thing_thing WHERE
thing_thing.cost > '0' GROUP BY thing_thing.cost HAVING
SUM(thing_thing.cost) > 0")))
3
>>> len(list(cursor.execute("SELECT * from thing_thing WHERE
thing_thing.cost > '0' GROUP BY thing_thing.cost HAVING
SUM(thing_thing.cost) > '0'")))
0
}}}
This can be problematic when using the `decimal.Decimal` type, as the it
will be coerced to a string by the
`django.db.backends.utils.rev_typecast_decimal` adapter. For example:
{{{
#!python
>>> query = "SELECT * from thing_thing WHERE thing_thing.cost > '0' GROUP
BY thing_thing.cost HAVING SUM(thing_thing.cost) > ?"
>>> len(list(cursor.execute(query, (0.0,))))
3
>>> len(list(cursor.execute(query, ('0.0',))))
0
>>> from decimal import Decimal
>>> len(list(cursor.execute(query, (Decimal('0.0'),))))
0
}}}
Therefore, any queryset instantiated by a sqlite3 backend that uses the
aggregation features described in the Django documentation
[https://docs.djangoproject.com/en/dev/topics/db/aggregation/ Aggregation
guide] will most probably return incorrect results. The problem has been
time consuming to debug, as the origin is not immediately clear.
The master ticket for this issue appears to be #21179. The duplicated
#22117 was recently open, but it is expected that more will come.
Therefore, I recommend that a warning message be included after the first
few paragraphs of the
[https://docs.djangoproject.com/en/dev/topics/db/aggregation/ Aggregation
guide], that provides an explanation of the problem.
An example message could be:
{{{
.. warning::
Aggregation is not currently supported by the Django sqlite backend
when using types that coerce
to strings. One example is :class:`decimal.Decimal`, as instances of
this type will be converted
to `str` instead of `float`, in order to preserve their arithmetic
precision.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22128>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
using the sqlite backend and certain types
Reporter: zr | Owner: zr
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by zr):
* owner: nobody => zr
* needs_better_patch: => 0
* status: new => assigned
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:1>
Django
using the sqlite backend and certain types
Reporter: zr | Owner: zr
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by zr):
Sorry, I made a mistake in the description. The master ticket is #18247,
and not the unrelated #21179.
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:2>
Django
using the sqlite backend and certain types
Reporter: zr | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by zr):
* owner: zr =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:3>
Django
using the sqlite backend and certain types
Reporter: zr | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Old description:
New description:
The master ticket for this issue appears to be #18247. The duplicated
#22117 was recently open, but it is expected that more will come.
Therefore, I recommend that a warning message be included after the first
few paragraphs of the
[https://docs.djangoproject.com/en/dev/topics/db/aggregation/ Aggregation
guide], that provides an explanation of the problem.
An example message could be:
{{{
.. warning::
Aggregation is not currently supported by the Django sqlite backend
when using types that coerce
to strings. One example is :class:`decimal.Decimal`, as instances of
this type will be converted
to `str` instead of `float`, in order to preserve their arithmetic
precision.
}}}
--
Comment (by shai):
Description corrected per comment:3.
To the point: Would this warning still be needed after #18247 is fixed? If
not, it would just be documenting a bug.
Even if it were, is it really that "aggregation is not supported" or only
that "comparisons against aggregates may not perform as expected"?
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:4>
Django
using the sqlite backend and certain types
Reporter: zr | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by zr):
> Would this warning still be needed after #18247 is fixed? If not, it
would just be documenting a bug.
No, but fixing #18247 may take some time. Also, this isn't really a Django
bug, but an integration problem between the Python sqlite bindings and
decimal.Decimal. I think the correct wording is "Django does not support
the aggregation feature using the sqlite backend" because Django does not
attempt to handle this situation in the ORM. A possible way to handle this
would be to coerce the decimal.Decimal into a float, but this would lose
precision, so it isn't a straightforward decision to make.
> Even if it were, is it really that "aggregation is not supported" or
only that "comparisons against aggregates may not perform as expected"?
More like the latter, but then again, the problem stems from the fact that
Django ORM does not attempt to resolve the problem for you. In a sense, it
does not support this use-case.
I think a warning message on the page would save users much grief and
frustration, while a decision is being taken on how to best resolve the
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:5>
Django
using the sqlite backend and certain types
Reporter: zr | Owner:
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Documentation | Resolution: duplicate
Severity: Normal | Triage Stage:
Keywords: sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by shai):
* cc: shai (added)
* status: new => closed
* resolution: => duplicate
Comment:
Replying to [comment:5 zr]:
> > Would this warning still be needed after #18247 is fixed? If not, it
would just be documenting a bug.
>
> No, but fixing #18247 may take some time.
We usually avoid documenting bugs, long-standing bugs included. A warning
might be fitting in the database-specific warnings (rather than
aggregation guide), if #18247 was wontfix'd. However, this does not seem
likely at this point.
> > Even if it were, is it really that "aggregation is not supported" or
only that "comparisons against aggregates may not perform as expected"?
>
> More like the latter, but then again, the problem stems from the fact
that Django ORM does not attempt to resolve the problem for you. In a
sense, it does not support this use-case.
I think you are mixing behavior with implementation. When you document
that something is "not supported" it usually means "intentionally
unsupported", not "we just neglected to do anything about it".
I sympathize with your aggravation over the hard debugging, but I don't
think this is a good way to mitigate it.
I will add a comment about this to #18247 -- this bug, then, will also be
made a duplicate of it. Feel free to reopen if you can come up with new
arguments.
--
Ticket URL: <https://code.djangoproject.com/ticket/22128#comment:6>