#35147 closed Cleanup/optimization (fixed)
Document that non-wrapped arithmetic with integer fields might require explicit output_field
Reported by: | Petar Netev | Owned by: | Petar Netev |
---|---|---|---|
Component: | Documentation | Version: | 5.0 |
Severity: | Normal | Keywords: | annotate, bitand, filter, integerfield, overflow |
Cc: | Petar Netev, Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi team,
I faced an unexpected behaviour of the bitand method of the F model when used with a BigIntField value.
The change is that when I try to annotate and then filter on the promoted field, the query set is always empty, when it shouldn't be.
Code is as follows:
records_qs = Model.objects.annotate(flag_some_flag=F("flags").bitand(2**34)).filter(flag_some_flag=2**34)
The returned queryset is empty, but it shouldn't be.
Simple test with using only annotate and further filtration using python list comprehension from the queryset using the annotated field, returns the needed records.
The filtration works properly for flag values < 2**31
.
I was looking for some notes in the Django 5 release notes, but I found nothing, and this is the version where the problem is faced. With Django 4.2.8 this problem is not faced.
Regards,
Petar Netev
Change History (14)
comment:2 by , 10 months ago
Cc: | added |
---|---|
Keywords: | integerfield overflow added |
Thank you for your report Petar.
The output field resolving logic or the ORM, particularly the combinator part used by bitand
and friends, is far from perfect and it is to blame here as any IntegerField
subclass combined with another for bitwise connectors will result in a CombinedExpression(output_field=IntegerField())
.
The problem does not only manifests itself in expression combinations though, functions such as django.db.models.functions.Power("integer_field", "integer_field")
can also overflow.
What makes combinator particularly problematic though is that they cannot have the user provide an explicit output_field
like it's the case with other expressions such as functions. The only solution I can think of with the current state of things is to use ExpressionWrapper
which is documented to be required in this case
ExpressionWrapper
is necessary when using arithmetic onF()
expressions with different types as described in UsingF()
with annotations.
And from the annotation documentation about the usage of F
If the fields that you’re combining are of different types you’ll need to tell Django what kind of field will be returned. Most expressions support output_field for this case, but since
F()
does not, you will need to wrap the expression withExpressionWrapper
:
You haven't provided your model definition but I suspect wrapping flag_some_flag
in ExpressionWrapper(F("flags").bitand(2**34), BigIntegerField())
will address your problem?
If that's the case I'm not against adjusting the 5.0 release notes to mention that non-wrapped arithmetic against integer fields might require adjustments, as documented, and possibly accepting a ticket to try to make the logic more generic wrt/ to integer field subclasses. Since this is clearly documented though I'm not convinced this qualifies as a release blocker that would warrant a code backport.
comment:3 by , 10 months ago
Hi Simon,
Thanks for your notes.
Indeed using ExpressionWrapper produces the expected behaviour. In our case we did took another approach usiing the Exact Exact(F("flags").bitand(flag_value), flag_value)
.
It is not a blocker, as you said.
comment:4 by , 10 months ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
Given the information shared so far (thanks Simon and Petar!), I agree this is not a release blocker and that documentation changes could help future users. Accepting on such basis.
Petar, would you be able/like to prepare a patch for the docs improvements?
comment:5 by , 10 months ago
Summary: | Unexcepted behaviour for the filter() method of the QuerySet in version 5.0.1 → Extend release docs to mention that non-wrapped arithmetic against integer fields might require explicit output_field |
---|
follow-up: 9 comment:6 by , 10 months ago
Hi Natalia and Simon,
I would like to prepare the patch, but I would need some clarifications on it.
- Do we reference the release notes for version 5.0, specifically https://docs.djangoproject.com/en/5.0/releases/5.0/#models ?
- Do we reference the documentation of the F() https://docs.djangoproject.com/en/5.0/releases/5.0/#models ?
Simon, can you share the commit related to "I'm fairly certain this relates to a commit meant to address a SQLite crash and a DDOS vector on Postgres only present in 5.0" in order to mention it in the docs improvements?
Thank you both!
comment:7 by , 10 months ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Summary: | Extend release docs to mention that non-wrapped arithmetic against integer fields might require explicit output_field → Document that non-wrapped arithmetic with integer fields might require explicit output_field |
comment:9 by , 10 months ago
Replying to Petar Netev:
Hi Natalia and Simon,
I would like to prepare the patch, but I would need some clarifications on it.
- Do we refference the release notes for version 5.0, specifically https://docs.djangoproject.com/en/5.0/releases/5.0/#models ?
- Do we refference the documentation of the F() https://docs.djangoproject.com/en/5.0/releases/5.0/#models ?
Simon, can you share the commit related to "I'm fairly certain this relates to a commit meant to address a SQLite crash and a DDOS vector on Postgres only present in 5.0" in order to mention it in the docs improvements?
dde2537fbb04ad78a673092a931b449245a2d6ae is the commit I was referring to, we don't refer to commits or ticket numbers in docs though.
I think the existing documentation is clear enough that ExpressionWrapper
might be necessary in these cases, the only admonition I would add, assuming we believe it might be valuable is a note in the miscellaneous section of the 5.0 release notes mentioning that explicit ExpressionWrapper
wrapping might be necessary for integer field arithmetic that can overflow while it wasn't the case in previous release by referencing at the documentation mentioned in comment:2.
comment:10 by , 10 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 10 months ago
Has patch: | set |
---|
comment:12 by , 10 months ago
Patch needs improvement: | set |
---|
comment:13 by , 10 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
A clue would be Simon Charette's answer:
I'm fairly certain this relates to a commit meant to address a SQLite crash and a DDOS vector on Postgres only present in 5.0 so far[0].
I suspect we'll want to adapt
_connector_combinations
so when bitwise operators are used between integers fields the output field is the wider of both and not systematically IntegerField.I suspect the problem manifests itself in other cases as well such as IntegerField + IntegerField which can overflow into a BigIntegerField.