Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#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:1 by Petar Netev, 3 months ago

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.

Last edited 3 months ago by Natalia Bidart (previous) (diff)

comment:2 by Simon Charette, 3 months ago

Cc: Simon Charette 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 on F() expressions with different types as described in Using F() 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 with ExpressionWrapper:

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.

Last edited 3 months ago by Simon Charette (previous) (diff)

comment:3 by Petar Netev, 3 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 Natalia Bidart, 3 months ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/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 Natalia Bidart, 3 months ago

Summary: Unexcepted behaviour for the filter() method of the QuerySet in version 5.0.1Extend release docs to mention that non-wrapped arithmetic against integer fields might require explicit output_field

comment:6 by Petar Netev, 3 months ago

Hi Natalia and Simon,

I would like to prepare the patch, but I would need some clarifications on it.

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!

Last edited 3 months ago by Tim Graham (previous) (diff)

comment:7 by Tim Graham, 3 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_fieldDocument that non-wrapped arithmetic with integer fields might require explicit output_field

in reply to:  6 comment:9 by Simon Charette, 3 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.

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.

Last edited 3 months ago by Tim Graham (previous) (diff)

comment:10 by Petar Netev, 3 months ago

Owner: changed from nobody to Petar Netev
Status: newassigned

comment:11 by Natalia Bidart, 3 months ago

Has patch: set

comment:12 by Mariusz Felisiak, 3 months ago

Patch needs improvement: set

comment:13 by Mariusz Felisiak, 3 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In 0630ca5:

Fixed #35147 -- Added backward incompatibility note about filtering against overflowing integers.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 3 months ago

In 741f080:

[5.0.x] Fixed #35147 -- Added backward incompatibility note about filtering against overflowing integers.

Backport of 0630ca5725ba5b17c61cd1f6a05dce2660c4724e from main

Note: See TracTickets for help on using tickets.
Back to Top