[Django] #29229: QuerySet.values_list() combined with .extra() or .annotate() may produce wrong .union()
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) |
Severity: Release | Keywords: union values_list
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
Regression introduced in 2.0, still present in 2.0.3.
Easy context to reproduce the problem: suppose a Message model, in a
postman app. Only one ordinary field is enough for the demo.
{{{#!python
qs1 = Message.objects.extra(select={'count': 0}).values_list('id',
'count').order_by()
print(qs1.query) # as expected:
# SELECT (0) AS "count", "postman_message"."id"
# FROM "postman_message"
qs2 =
Message.objects.values('somefield').annotate(count=models.Count('pk')).annotate(id=models.Max('pk')).values_list('id',
'count').order_by()
print(qs2.query) # as expected:
# SELECT COUNT("postman_message"."id") AS "count",
MAX("postman_message"."id") AS "id"
# FROM "postman_message" GROUP BY "postman_message"."somefield"
print(qs1.union(qs2).query) # !! WRONG !! the qs2 part is truncated:
# SELECT (0) AS "count", "postman_message"."id" FROM "postman_message"
# UNION
# SELECT MAX("postman_message"."id") AS "id" FROM "postman_message" GROUP
BY "postman_message"."somefield"
}}}
Compared to version 1.11, it comes from this addition in
db/models/sql/compiler.py/get_combinator_sql():
{{{#!python
if not compiler.query.values_select and self.query.values_select:
compiler.query.set_values(self.query.values_select)
}}}
Here, self.query.values_select is ('id',).
For qs2, values_select is indeed empty, as coded in
db/models/sql/query.py/set_values(),
because in this case the two values_list() arguments are managed in
annotation_names[].
--
Ticket URL: <https://code.djangoproject.com/ticket/29229>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: union values_list | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "regression_test_29229.txt" added.
regression test result
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* version: master => 2.0
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/9796 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:1>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
* version: 2.0 => 1.11
* stage: Accepted => Ready for checkin
Comment:
This affects Django 1.11.8 and later, due to
67316720603821ebb64dfe8fa592ba6edcef5f3e.
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:2>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Old description:
New description:
Regression introduced in 2.0, still present in 2.0.3 [Edit: and backported
in 1.11.8].
Easy context to reproduce the problem: suppose a Message model, in a
postman app. Only one ordinary field is enough for the demo.
{{{#!python
qs1 = Message.objects.extra(select={'count': 0}).values_list('id',
'count').order_by()
print(qs1.query) # as expected:
# SELECT (0) AS "count", "postman_message"."id"
# FROM "postman_message"
qs2 =
Message.objects.values('somefield').annotate(count=models.Count('pk')).annotate(id=models.Max('pk')).values_list('id',
'count').order_by()
print(qs2.query) # as expected:
# SELECT COUNT("postman_message"."id") AS "count",
MAX("postman_message"."id") AS "id"
# FROM "postman_message" GROUP BY "postman_message"."somefield"
print(qs1.union(qs2).query) # !! WRONG !! the qs2 part is truncated:
# SELECT (0) AS "count", "postman_message"."id" FROM "postman_message"
# UNION
# SELECT MAX("postman_message"."id") AS "id" FROM "postman_message" GROUP
BY "postman_message"."somefield"
}}}
Compared to version 1.11 [Edit: I run 1.11.7 precisely], it comes from
this addition in db/models/sql/compiler.py/get_combinator_sql():
{{{#!python
if not compiler.query.values_select and self.query.values_select:
compiler.query.set_values(self.query.values_select)
}}}
Here, self.query.values_select is ('id',).
For qs2, values_select is indeed empty, as coded in
db/models/sql/query.py/set_values(),
because in this case the two values_list() arguments are managed in
annotation_names[].
--
Comment (by master):
Precisions about versions.
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:3>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"a0c03c62a8ac586e5be5b21393c925afa581efaf" a0c03c62]:
{{{
#!CommitTicketReference repository=""
revision="a0c03c62a8ac586e5be5b21393c925afa581efaf"
Fixed #29229 -- Fixed column mismatch crash when combining two annotated
values_list() querysets with union(), difference(), or intersection().
Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:4>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"9123fd75caa595c018d4121575cbada80226b4f2" 9123fd7]:
{{{
#!CommitTicketReference repository=""
revision="9123fd75caa595c018d4121575cbada80226b4f2"
[2.0.x] Fixed #29229 -- Fixed column mismatch crash when combining two
annotated values_list() querysets with union(), difference(), or
intersection().
Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.
Backport of a0c03c62a8ac586e5be5b21393c925afa581efaf from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:5>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"c5bb472095f02df364f9a0ce64f0b97bfd4a8c63" c5bb472]:
{{{
#!CommitTicketReference repository=""
revision="c5bb472095f02df364f9a0ce64f0b97bfd4a8c63"
[1.11.x] Fixed #29229 -- Fixed column mismatch crash when combining two
annotated values_list() querysets with union(), difference(), or
intersection().
Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.
Backport of a0c03c62a8ac586e5be5b21393c925afa581efaf from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:6>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
It seems that the regression test added here doesn't match the original
steps to reproduce closely enough as the original issue has reoccurred
after #29286 and a new ticket (#29694) is created.
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:7>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by master):
The regression test added by the PR
(test_union_with_two_annotated_values_list()) seems to have an
unnecessarily complex combination, just to comply with a mandatory
ordering of fields imposed by the union.
Instead of:
{{{
.annotate(count=F('num'),).annotate(num=Value(1, IntegerField()),
}}}
This writing is more understandable:
{{{
.annotate(num=F('num'),).annotate(count=Value(1, IntegerField()),
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29229#comment:8>