Re: [Django] #33975: __in doesn't clear selected fields on the RHS when QuerySet.alias() is used after annotate().
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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):
Whether or not the `In` lookups defaults a `Query` right-hand-side to
`.values('pk')` is based of `rhs.has_select_fields`
([https://github.com/django/django/blob/cd1afd553f9c175ebccfc0f50e72b43b9604bd97/django/db/models/lookups.py#L421-L425
source])
This dynamic property is based of `self.select or
self.annotation_select_mask` so I suspect that there might be some bad
logic in `Query.add_annotation`
([https://github.com/django/django/blob/b3db6c8dcb5145f7d45eff517bcd96460475c879/django/db/models/sql/query.py#L1109-L1119
source]) from the introduction of `QuerySet.alias` in
f4ac167119e8897c398527c392ed117326496652.
The
`self.set_annotation_mask(set(self.annotation_select).difference({alias}))`
[https://github.com/django/django/commit/f4ac167119e8897c398527c392ed117326496652
#diff-
fd2300283d1546e36141373b0621f142ed871e3e8856e07efe5a22ecc38ad620R1025
line] seems to be what causes the problem here. If `annotate` and `alias`
are used then `annotation_select_mask` will be materialized as a ''mask''
is required to keep track of which entries in `annotations` must be
selected and whatnot (not necessary if only using `alias` or `annotate` as
the first doesn't require any selecting and the second selects all
annotations)
The `add_annotation` logic relied on to implement `QuerySet.alias` is not
wrong per se it just happens to piggy back on the annotation masking logic
that only `Query.set_values` relied on previously an now happens to break
the heuristics in `Query.has_select_fields`.
The proper solutions is likely to replace `Query.has_select_fields` by a
class attribute that defaults to `False` and have `set_values` set it to
`True` and make sure `Query.clone` carries the attribute over. This seems
like a more durable options as that avoids trying to peek into internals
to determine if a `values` call was performed.
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
after annotate().
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 => Bhuvnesh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:6>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Bhuvnesh):
hi, I am new to Django contribution, i tried to implement the solution
proposed by simon, replaced the Query.has_select_fields with
has_select_fields attribute with default value False and have set_values
set it to true, but now i am stuck at carrying over the attribute to
Query.clone , can I get a bit more explanation for the same?
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:7>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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):
If you're still struggling with the clone part of the patch it basically
means that `Query.clone` must assign `obj.has_select_fields = True` when
`if self.has_select_fields`
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:8>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Bhuvnesh):
Thanks, that worked! Opening a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:9>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Bhuvnesh):
PR [https://github.com/django/django/pull/16032]
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:10>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(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/33975#comment:11>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:12>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:13>
Django
after annotate().
-------------------------------------+-------------------------------------
Reporter: Gabriel Muj | Owner: Bhuvnesh
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"32797e7fbfb28c4cd2210aae37157267d237364f" 32797e7f]:
{{{
#!CommitTicketReference repository=""
revision="32797e7fbfb28c4cd2210aae37157267d237364f"
Fixed #33975 -- Fixed __in lookup when rhs is a queryset with annotate()
and alias().
This fixes clearing selected fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33975#comment:14>