Re: [Django] #36025: __range lookup in conditional aggregate with subquery annotation does not use annotated related fields

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay.Amballi | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

An important lead here is likely this statement

> Explicitly using `__gte` and `__lte` instead of using `__range` resolved
the issue.

If that's truly the case then it means that the likely culprit is how
`RangeLookup` stores its resolved right-hand-side as a `tuple(Col(...),
Col(...))` which trips resolvable detection (`hasattr(expr,
"resolve_expression")`) that happens when alias relabeling takes place
(see `Expression.relabeled_clone`).

`Expression.relabeled_clone`, which is called when subqueries are involved
and tables need to be re-aliases to avoid conflicts, relies on
`self.get_source_expression` to ''walk'' through the expression tree and
make sure all nested references to tables are re-aliases. If you look at
`Lookup.get_source_expressions` though you'll notice that the right-hand-
side is excluded if it's not a ''compilable'' (this should likely be a
''resolvable'' check instead). Since `RangeLookup.rhs` is a `tuple` and
thus not considered a ''resolvable'' it falls on its face and isn't
considered for re-labeling.

We have a larger problem here where lookups that allow containers of
potentially ''resolvable'' members as right-hand-side (e.g. `__range`,
`__in`) that are not considered as ''resolvable'' themselves that we
should address but the immediate solution for `RangeLookup` appears to be
wrapping it's right-hand-side in an `ExpressionList` at resolving time.
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------

Reporter: Aashay Amballi | Owner: (none)

Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | 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):

Some related tickets of interest here are #22288 and #16487 which added
support for `tuple[Resolvable, Resolvable]` but missed the re-aliasing
support.
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:5>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Gregory Mariani):

Can't reproduce with the minimal example. I get:
{{{ {'out_of_bound_count': 0, 'project_id_subquery_sum': 3} }}}
with that script:
{{{
project1 = Project.objects.create(
start_date='2024-01-01',
end_date='2024-06-30')

project2 = Project.objects.create(
start_date='2024-07-01',
end_date='2024-12-31')

due_date1 = timezone.make_aware(timezone.datetime(2024, 4, 15, 10, 0, 0))
due_date2 = timezone.make_aware(timezone.datetime(2024, 10, 1, 12, 0, 0))

work_order1 = WorkOrder.objects.create(
project=project1,
due_date=due_date1,
)

work_order2 = WorkOrder.objects.create(
project=project2,
due_date=due_date1,
)

result = WorkOrder.objects.annotate(
project_id_subquery=Subquery(Project.objects.filter(
id=OuterRef("project_id")).values("id")),
).aggregate(
out_of_bound_count=Count("id", filter=Q(due_date__date__range=(
F('project__start_date'), F('project__end_date')))),
project_id_subquery_sum=Sum("project_id_subquery"),
)
print(result)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:6>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------

Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: assigned

Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* has_patch: 0 => 1
* owner: (none) => Simon Charette
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:7>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: ORM | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:8>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette

Type: Bug | Status: closed

Component: Database layer | Version: 4.2
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: ORM | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"089deb82b9ac2d002af36fd36f288368cdac4b53" 089deb82]:
{{{#!CommitTicketReference repository=""
revision="089deb82b9ac2d002af36fd36f288368cdac4b53"
Fixed #36025 -- Fixed re-aliasing of iterable (in/range) lookups rhs.

In order for Expression.relabeled_clone to work appropriately its
get_source_expressions method must return all resolvable which wasn't the
case
for Lookup when its right-hand-side is "direct" (not a compilable).

While refs #22288 added support for non-literals iterable right-hand-side
lookups it predated the subclassing of Lookup(Expression) refs #27021
which
could have been an opportunity to ensure right-hand-sides are always
resolvable
(ValueList and ExpressionList).

Addressing all edge case with non-resolvable right-hand-sides would
require
a significant refactor and deprecation of some parts of the Lookup
interface so
this patch only focuses on FieldGetDbPrepValueIterableMixin (In and Range
lookups) by making sure that a right-hand-side containing resolvables are
dealt
with appropriately during the resolving phase.

Thanks Aashay Amballi for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:9>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: ORM | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"0bac41fc7e4a842e8d20319cba31cc645501c245" 0bac41f]:
{{{#!CommitTicketReference repository=""
revision="0bac41fc7e4a842e8d20319cba31cc645501c245"
Refs #34975 -- Removed unnecessary lookups.In.get_refs().

Now that In.get_source_expression() includes its right-hand-side when it
contains expressions (refs #36025) it no longer requires a specialized
get_refs() method.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:10>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: ORM | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"0690c06013808d92ca8804dd4267adebdba20ca6" 0690c060]:
{{{#!CommitTicketReference repository=""
revision="0690c06013808d92ca8804dd4267adebdba20ca6"
[5.2.x] Refs #34975 -- Removed unnecessary lookups.In.get_refs().

Now that In.get_source_expression() includes its right-hand-side when it
contains expressions (refs #36025) it no longer requires a specialized
get_refs() method.

Backport of 0bac41fc7e4a842e8d20319cba31cc645501c245 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:12>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: ORM | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"d99985bbc121749c5a6bb9eb9a4a9099b6a002eb" d99985b]:
{{{#!CommitTicketReference repository=""
revision="d99985bbc121749c5a6bb9eb9a4a9099b6a002eb"
[5.2.x] Fixed #36025 -- Fixed re-aliasing of iterable (in/range) lookups

rhs.

In order for Expression.relabeled_clone to work appropriately its
get_source_expressions method must return all resolvable which wasn't the
case
for Lookup when its right-hand-side is "direct" (not a compilable).

While refs #22288 added support for non-literals iterable right-hand-side
lookups it predated the subclassing of Lookup(Expression) refs #27021
which
could have been an opportunity to ensure right-hand-sides are always
resolvable
(ValueList and ExpressionList).

Addressing all edge case with non-resolvable right-hand-sides would
require
a significant refactor and deprecation of some parts of the Lookup
interface so
this patch only focuses on FieldGetDbPrepValueIterableMixin (In and Range
lookups) by making sure that a right-hand-side containing resolvables are
dealt
with appropriately during the resolving phase.

Thanks Aashay Amballi for the report.

Backport of 089deb82b9ac2d002af36fd36f288368cdac4b53 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:11>

[Django]

#36025: __range lookup in conditional aggregate with subquery annotation does not
use annotated related fields
-------------------------------------+-------------------------------------
Reporter: Aashay Amballi | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: ORM | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"7b54ddd5e64c96c641b70bb5f0e958a9e2035fb2" 7b54ddd]:
{{{#!CommitTicketReference repository=""
revision="7b54ddd5e64c96c641b70bb5f0e958a9e2035fb2"
Refs #36025 -- Made get_prep_lookup() pass output_field when wrapping
direct values in Value.

Previously, only strings were supplied with an output_field when wrapping
direct value iterable elements in Value expressions for ExpressionList.
This
caused problems for __in lookups on JSONField when using expressions
alongside direct values, as JSONField values can have different types
which
need to be adapted by the field's get_db_prep_value().

Refs #36689.

Thanks Jacob Walls for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36025#comment:13>