[Django] #33257: Comparisons against DecimalField annotations behave inconsistently on sqlite
Django
sqlite
-------------------------------------+-------------------------------------
Reporter: Matthijs | Owner: nobody
Kooijman |
Type: Bug | Status: new
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I noticed that, on sqlite, some comparisons against DecimalField
annotations behave unexpectedly, in particular when wrapping a
DecimalField value in a Case/When or ExpressionWrapper. I suspect that
there might be some inconsistencies in the type conversions here somehow.
I've created a few testcase to illustrate the problem on current git main:
https://github.com/matthijskooijman/django/commit/3470b98b42c39fd9a9a4e1443341f16780da7a98
and see below.
{{{
@override_settings(DEBUG=True)
def test_00compare_field(self):
"""Comparing a Case annotation wrapping a field to a literal
works."""
Foo.objects.create(a='', d=1)
try:
Foo.objects.filter(d__gt=0).get()
finally:
from django.db import connection
print(connection.queries[-1]['sql'])
@override_settings(DEBUG=True)
def test_01compare_annotation_value_literal(self):
"""Comparing a literal annotation using Value to a literal
works."""
# Fields are not actually used here
Foo.objects.create(a='', d=0)
try:
Foo.objects.annotate(
x=models.Value(1,
output_field=models.fields.DecimalField(max_digits=1, decimal_places=0)),
).filter(x__gt=0).get()
finally:
from django.db import connection
print(connection.queries[-1]['sql'])
@override_settings(DEBUG=True)
def test_02compare_annotation_expressionwrapper_literal(self):
"""Comparing a literal annotation using ExpressionWraper and Value
to a literal works."""
# Fields are not actually used here
Foo.objects.create(a='', d=0)
try:
Foo.objects.annotate(
x=models.ExpressionWrapper(
models.Value(1),
output_field=models.fields.DecimalField(max_digits=1,
decimal_places=0),
),
).filter(x__gt=0).get()
finally:
from django.db import connection
print(connection.queries[-1]['sql'])
@override_settings(DEBUG=True)
def test_03compare_case_annotation(self):
"""Comparing a Case annotation wrapping a field to a literal
works."""
Foo.objects.create(a='', d=1)
try:
Foo.objects.annotate(
x=models.Case(models.When(a='', then=models.F('d'))),
).filter(x__gt=0).get()
finally:
from django.db import connection
print(connection.queries[-1]['sql'])
}}}
- test_00compare_field compares a field directly with a literal,
which
works.
- test_01compare_annotation_value_literal adds a literal annotation
using just
Value and then compares it, which also works.
- test_02compare_annotation_expressionwrapper_literal adds a literal
annotation using Value wrapped in ExpressionWrapper, which does not
work becomes a literal int, rather than a string like the compared
value.
- test_03compare_case_annotation wraps the field in a case/when and
then
compares it, which also does not work (maybe the CASE changes the
type?)
Running these testcases against sqlite gives:
{{{
SELECT "model_fields_foo"."id", "model_fields_foo"."a",
"model_fields_foo"."d" FROM "model_fields_foo" WHERE
"model_fields_foo"."d" > '0' LIMIT 21
.SELECT "model_fields_foo"."id", "model_fields_foo"."a",
"model_fields_foo"."d", CAST('1' AS NUMERIC) AS "x" FROM
"model_fields_foo" WHERE CAST('1' AS NUMERIC) > '0' LIMIT 21
.SELECT "model_fields_foo"."id", "model_fields_foo"."a",
"model_fields_foo"."d", 1 AS "x" FROM "model_fields_foo" WHERE 1 > '0'
LIMIT 21
ESELECT "model_fields_foo"."id", "model_fields_foo"."a",
"model_fields_foo"."d", CASE WHEN "model_fields_foo"."a" = '' THEN
"model_fields_foo"."d" ELSE NULL END AS "x" FROM "model_fields_foo" WHERE
CASE WHEN ("model_fields_foo"."a" = '') THEN "model_fields_foo"."d" ELSE
NULL END > '0' LIMIT 21
E.s...........
======================================================================
ERROR: test_02compare_annotation_expressionwrapper_literal
(model_fields.test_decimalfield.DecimalFieldTests)
Comparing a literal annotation using ExpressionWraper and Value to a
literal works.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/matthijs/docs/src/upstream/django/django/test/utils.py",
line 437, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/tests/model_fields/test_decimalfield.py",
line 142, in test_02compare_annotation_expressionwrapper_literal
Foo.objects.annotate(
File
"/home/matthijs/docs/src/upstream/django/django/db/models/query.py", line
441, in get
raise self.model.DoesNotExist(
model_fields.models.Foo.DoesNotExist: Foo matching query does not exist.
======================================================================
ERROR: test_03compare_case_annotation
(model_fields.test_decimalfield.DecimalFieldTests)
Comparing a Case annotation wrapping a field to a literal works.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/matthijs/docs/src/upstream/django/django/test/utils.py",
line 437, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/tests/model_fields/test_decimalfield.py",
line 154, in test_03compare_case_annotation
Foo.objects.annotate(
File
"/home/matthijs/docs/src/upstream/django/django/db/models/query.py", line
441, in get
raise self.model.DoesNotExist(
model_fields.models.Foo.DoesNotExist: Foo matching query does not exist.
----------------------------------------------------------------------
}}}
Note in the printed queries that the 0 value that is compared with is a
string in the query (`'0'`), while the ExpressionWrappered value is just a
plain number (`1`). The Value without ExpressionWrapper has a cast to
NUMERIC that I suspect makes that case work?
Also note that the DEBUG stuff and try-catch is only added to capture the
query that is being generated, it is not needed to trigger the problem. I
tried printing the `QuerySet.query` attribute first, but that seems to
hide the quotes around the literal 0 in the query, so took me a while to
realize what was happening. The numbers in the testcase names are to
ensure the SQL queries are printed in a recognizable order.
All four testcases work on MySQL, there the 0 value is written in the
query without the quotes, and the cast to NUMERIC is also missing.
I suspect this issue is highly similar to #18247, and can a fix can
probably build on the fix for that issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/33257>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
sqlite
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Matthijs Kooijman):
It seems that adding SQLiteNumericMixin to Case and ExpressionWrapper
actually fixes this, with the below patch all my testcases pass:
{{{diff
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -933,7 +933,7 @@ def as_sqlite(self, compiler, connection,
**extra_context):
return self.as_sql(compiler, connection, **extra_context)
-class ExpressionWrapper(Expression):
+class ExpressionWrapper(SQLiteNumericMixin, Expression):
"""
An expression that can wrap another expression so that it can provide
extra context to the inner expression, such as the output_field.
@@ -1032,7 +1032,7 @@ def get_group_by_cols(self, alias=None):
return cols
-class Case(Expression):
+class Case(SQLiteNumericMixin, Expression):
"""
An SQL searched CASE expression:
}}}
I wonder if this mixing should be added to all expressions, so maybe to
Expression or BaseExpression? I quickly tried, but got
`django.db.utils.OperationalError: near "WHEN": syntax error` though.
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Matthijs Kooijman
* status: new => assigned
* stage: Unreviewed => Accepted
Comment:
Thanks for the detailed report!
> It seems that adding SQLiteNumericMixin to Case and ExpressionWrapper
actually fixes this, with the below patch all my testcases pass:
Yes, that should fix the issue (see related #31723 and #32585).
> I wonder if this mixing should be added to all expressions, so maybe to
Expression or BaseExpression?
This can be more complicated because "partial" expressions as `When()`
(`WHEN ... THEN ...`) cannot be wrapped.
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Matthijs Kooijman):
Thanks for confirming that SQLiteNumericMixin is the right approach here.
I'll try to find some time to prepare a PR with that.
Any suggestions on where the testcases for this should live? I put them at
model_fields/decimal_field now, but they also relate to SQLite, query
expressions, and maybe other SQLite numeric fields, so maybe there is a
better place for them?
I also had a look through expressions.py, and it seems that most
expressions already have SQLiteNumericMixin, except:
- Case & ExpressionWrapper: These break without, so should get it.
- When: This is a partial expression, so probably does not need it.
- Subquery: I suspect this also needs it, but I'll try to make a testcase
that breaks it first.
- OrderBy: I'm not sure about this one, I couldn't find any docs on how
this is used exactly..
- WindowFrame: This looks like a partial expression to me that doesn't
need it?
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> Any suggestions on where the testcases for this should live?
- `Case` -> `tests/expressions_case`,
- `ExpressionWrapper` -> `tests/expressions`
> I also had a look through expressions.py, and it seems that most
expressions already have SQLiteNumericMixin, except:
I agree with you analysis: `When`, `WindowFrame`, and `OrderBy` don't need
it. I'm not sure about `Subquery`, you can try to break it, but as far as
I'm aware it returns a single column/expression so casting to a numeric
value should be already handled there.
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Matthijs Kooijman):
> I agree with you analysis: When, WindowFrame, and OrderBy don't need
it.
Ok, I left those out.
> I'm not sure about Subquery, you can try to break it, but as far as I'm
aware it returns a single column/expression so casting to a numeric value
should be already handled there.
I tried, and could indeed not break Subquery.
Pullrequest is here: https://github.com/django/django/pull/15062
Regarding Case and ExpressionWrapper, I noticed a small difference between
them: ExpressionWrapper with output_field=DecimalField fails to convert a
non-decimal value to decimal (but works if the value is already a Decimal
literal or DecimalField), while Case actually breaks when applied to
existing decimal values (I suspect that sqlite does some type conversion
when executing the query). I now put both fixes in the same commit, if you
prefer separate commits, let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1a5023883bf4c8cccb34a830edcc3c82aa862455" 1a502388]:
{{{
#!CommitTicketReference repository=""
revision="1a5023883bf4c8cccb34a830edcc3c82aa862455"
Fixed #33257 -- Fixed Case() and ExpressionWrapper() with decimal values
on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33257#comment:8>