Re: [Django] #34160: Django 4.1 Expression contains mixed types for (Big/Small)IntegerFields.
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Luke Plant):
I agree we should try to automatically resolve the type. Based on my
experience with the related patch, I think it would be worth checking what
MySQL and Postgres do for cases like this - there could be some unexpected
surprises e.g. the database actually casts to the smaller of the type, or
the first type. In this case, the user may need to know that it isn't
going to do what you expect.
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I agree with your suggested solution Markusz. Adding entries to
`_connector_combinations` seems like the way to go for a backport.
> Based on my experience with the related patch, I think it would be worth
checking what MySQL and Postgres do for cases like this - there could be
some unexpected surprises
I know that Postgres will silently cast values outside of the `bigint`
range to `numeric` with [https://code.jeremyevans.net/2022-11-01-forcing-
sequential-scans-on-postgresql.html its unfortunate side effect] so I'm
pretty that it has operators defined to make integer comparison as
implicit as possible.
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Martin, Would you like to prepare a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Martin Lehoux):
Hi, I'd be happy to! I will have a look tonight, and will ask a few
questions if I have trouble understanding what to do (but from what I read
the infrastructure is already there for this feature)
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:6 Martin Lehoux]:
> Hi, I'd be happy to! I will have a look tonight, and will ask a few
questions if I have trouble understanding what to do (but from what I read
the infrastructure is already there for this feature)
Thanks, it should be enough to add extra
[https://github.com/django/django/blob/2848e5d0ce5cf3c31fe87525536093b21d570f69/django/db/models/expressions.py#L532-L547
combinations for integer fields] (a regression test is also required, see
[https://github.com/django/django/blob/2848e5d0ce5cf3c31fe87525536093b21d570f69/tests/expressions/tests.py#L2411-L2437
test_resolve_output_field_number()]).
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:7>
Django
-------------------------------------+-------------------------------------
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Martin Lehoux
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Martin Lehoux):
Isn't it the `Case` that has an issue computing it's `output_field`? If
it's the case, I'm not sure that the files you pointed to me were the
right ones.
''django/db/models/expressions.py''
{{{
@deconstructible(path="django.db.models.Case")
class Case(SQLiteNumericMixin, Expression):
"""
An SQL searched CASE expression:
CASE
WHEN n > 0
THEN 'positive'
WHEN n < 0
THEN 'negative'
ELSE 'zero'
END
"""
template = "CASE %(cases)s ELSE %(default)s END"
case_joiner = " "
def __init__(self, *cases, default=None, output_field=None, **extra):
if not all(isinstance(case, When) for case in cases):
raise TypeError("Positional arguments must all be When
objects.")
super().__init__(output_field)
self.cases = list(cases)
self.default = self._parse_expressions(default)[0]
self.extra = extra
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
What I believe is happening here is that `F("inventory_count") + Value(1)`
has its `output_field` resolved to `IntegerField` because it's the common
base of `inventory_count` (which I assume is a `SmallIntegerField` since
the models were not provided).
While `CombinedExpression._resolve_output_field` resolving logic deals
with subclassing properly `BaseExpression._resolve_output_field`
[https://github.com/django/django/blob/2848e5d0ce5cf3c31fe87525536093b21d570f69/django/db/models/expressions.py#L327-L329
isn't smart enough] to resolve a mix of `IntegerField` subclasses to the
largest denominator.
The three ways we could be fixing that would be:
1. Document this requirement the usage of explicit `output_field` either
to `Value` [https://docs.djangoproject.com/en/4.1/releases/3.2/ beyond the
3.2 release notes].
2. Make `Value._resolve_field` use different kind of `IntegerField`
subclasses when dealing with smaller/larger values. This could have the
unintended side effect of surfacing even more errors for mixed type as
`Value(n < 255)` would resolve to `SmallIntegerField` so I think this is
a bad idea.
3. Actually make `BasExpression._resolve_output_field` smarter with the
deprecation period it requires.
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Martin Lehoux):
I can confirm `inventory_count = models.SmallIntegerField()`
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Martin Lehoux):
If I understand well, you mean that the future vision (given the comment
you pointed out) is not to try to infer everything, and that I should
indeed in my code specify this `output_field` in this case?
Just trying to understand what I should do on my side, and if there's a
way for me to help you with that
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Martin, the only way I see on how this can be solved is to revisit how
`BaseExpression._resolve_output_field` deals with variadic ''merge''
expressions (e.g. `Case`, `Coalesce`) in a way that tries to be a bit more
clever with regards to type integer types handling. You could give it a
shot but be aware that it's going to be exploration work, certainly a nice
way to learn more about the ORM works but not necessarily one that will
lead to a fool proof solution.
In the mean time, I think that the only solution is effectively to add
explicit `output_field`.
I'd be happy to review your exploration work but just a small disclaimer
that I'm not sure what the proper solution might be.
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Martin Lehoux):
Alright thank you for you explanation, I'd be happy to have a look. I will
get back to you if I find something interesting
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
`Case()` is trying to resolve an output type only for specific fields that
cannot be implicitly cast on PostgreSQL (namely `GenericIPAddressField`,
`IPAddressField`, `TimeField`, and `UUIDField`) in other cases PostgreSQL
does the work, see [https://www.postgresql.org/docs/current/typeconv-
union-case.html docs]. As far as I'm aware, we should be safe to ignore
errors in resolving an output field in other cases:
{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index c270ef16c7..575712806f 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -1483,8 +1483,11 @@ class Case(SQLiteNumericMixin, Expression):
sql_params.extend(default_params)
template = template or template_params.get("template",
self.template)
sql = template % template_params
- if self._output_field_or_none is not None:
- sql = connection.ops.unification_cast_sql(self.output_field)
% sql
+ try:
+ if self._output_field_or_none is not None:
+ sql =
connection.ops.unification_cast_sql(self.output_field) % sql
+ except FieldError:
+ pass
return sql, sql_params
def get_group_by_cols(self):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:15 Mariusz Felisiak]:
> `Case()` is trying to resolve an output type only for specific fields
that cannot be implicitly cast on PostgreSQL (namely
`GenericIPAddressField`, `IPAddressField`, `TimeField`, and `UUIDField`)
in other cases PostgreSQL does the work, see
[https://www.postgresql.org/docs/current/typeconv-union-case.html docs].
As far as I'm aware, we should be safe to ignore errors in resolving an
output field in other cases:
After a deeper investigation, this ☝️ only fixes one case:
{{{#!python
Case(
When(Value(True), then=F("small_integer") + Value(1)),
default=F("integer"),
)
}}}
the following still crashes:
{{{#!python
Case(
When(Value(True), then=F("small_integer") + Value(1)),
default=F("small_integer"),
)
}}}
I don't think it's worth changing anymore. I theory,
40b8a6174f001a310aa33f7880db0efeeb04d4c4 is a regression, however the
previous behavior was also buggy, e.g. `F("small_integer") +
Value(100000000)` was resolved to `SmallIntegerField`, so I think it
better to raise an exception in all cases. I'm going to add a small
release note and close this as "wontfix" when it gets merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
[https://github.com/django/django/pull/16339 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"e8dcef155c1848ef49e54f787a7d20faf3bf9296" e8dcef15]:
{{{
#!CommitTicketReference repository=""
revision="e8dcef155c1848ef49e54f787a7d20faf3bf9296"
Refs #33397, Refs #34160 -- Added release note for resolving output_field
changes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Lehoux | Owner: Martin
| Lehoux
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"58156f4ed761b9f7650d5e7a12f3f20be3d60c53" 58156f4e]:
{{{
#!CommitTicketReference repository=""
revision="58156f4ed761b9f7650d5e7a12f3f20be3d60c53"
[4.1.x] Refs #33397, Refs #34160 -- Added release note for resolving
output_field changes.
Backport of e8dcef155c1848ef49e54f787a7d20faf3bf9296 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34160#comment:19>