[Django] #35011: Queryset union can fail depending on the field type and/or order
Django
-------------------------------------+-------------------------------------
Reporter: Thierry | Owner: nobody
Bastian |
Type: Bug | Status: new
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hello,
I was porting my code to Django 5 and there is an issue with the way
queryset are union'ed.
Essentially it ends up trying to convert datetime to int, which is
strange. I did not get time to investigate the issue myself.
You can note that the order of fields we use in the .values call affects
the behaviour (which is also highly unexpected).
To reproduce the issue, simply untar the attached example, run migrate
then test.
--
Ticket URL: <https://code.djangoproject.com/ticket/35011>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "bugreport.tar.xz" added.
example reproducing the bug
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
* owner: nobody => Simon Charette
* status: new => assigned
* stage: Unreviewed => Accepted
Comment:
It seems that your provide example has been failing since at least 3.2 so
it's not a recent regression in Django 5.
Running your test on Django 3.2 and 4.2 I get
{{{
ValueError: invalid literal for int() with base 10: '2023-12-05
02:46:28.751461'
}}}
The fundamental issue here is that the order of specified fields in
`values` has never been a 1:1 match to the order of the `SELECT` columns
but the latter is a necessity to do proper `UNION` between querysets.
The order is, and has always been, `SELECT *extra, *fields, *annotations`
and within each groups the order of `values` is respected since #28553
which landed in d6b6e5d0fd4e6b6d0183b4cf6e4bd4f9afc7bf67 so I guess this
is what broke things on your side during the attempted upgrade but that
your tests failed to capture. In this sense I guess this could be
considered a release blocker as it changed the ordering from an ambiguous
ordering to the other.
I tried [https://github.com/django/django/pull/16703 solving the
fundamental issue] but I got in a rabbit hole with regards to the
deprecation of `extra(fields)` which
[https://github.com/django/django/pull/16681 I tried to focus on first].
I'd like to find time to get this across the finish line during the
holidays so I'll assign to me in the mean time.
This is really what #28553 should have been about in my opinion; making
sure the order of `values/values_list` is always respected even with it
contains members that cross groups (`extra`, `fields`, `annotations`).
I'm torn on how to proceed here wrt/ to
d6b6e5d0fd4e6b6d0183b4cf6e4bd4f9afc7bf67 as while it fixed some queries it
broke others as described here.
Until we figure out our plan here Thierry your best bet is to stick
entirely to annotations. That means doing something like
{{{#!diff
diff --git a/bugreport/test.py b/bugreport/test.py
index 4f66fa2..f6fc98b 100644
--- a/bugreport/test.py
+++ b/bugreport/test.py
@@ -1,12 +1,12 @@
from django.test import TestCase
-from django.db.models import Value, IntegerField, DateTimeField
+from django.db.models import Value, IntegerField, DateTimeField, F
from bugreport.models import Leaf, Branch
class TreeTest(TestCase):
def test_repro(self):
- column_names = ['size', 'created_date']
+ column_names = ['size', 'created_date_ann']
#column_names = ['created_date', 'size'] # this one works...
leaf = Leaf.objects.create()
@@ -16,13 +16,13 @@ def test_repro(self):
branch_qs
.annotate(
size=Value(1, output_field=IntegerField()),
- created_date=Value(None, output_field=DateTimeField()),
+ created_date_ann=Value(None,
output_field=DateTimeField()),
).values_list(*column_names))
leaf_qs = Leaf.objects.all()
leaf_qs = (
leaf_qs
- .annotate(size=Value(2, output_field=IntegerField()))
+ .annotate(size=Value(2, output_field=IntegerField()),
created_date_ann=F("created_date"))
.values_list(*column_names)
)
print("this works", list(leaf_qs))
}}}
Which will have all the `values` reference in the same `SELECT` group and
due to d6b6e5d0fd4e6b6d0183b4cf6e4bd4f9afc7bf67 the order will be
respected.
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Thierry Bastian | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.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 Thierry Bastian):
Thank you for your detailed answer. Your explanation makes perfect sense.
As you can imaging, I was trying to reproduce with the smallest possible
example. It might be that some detail made it work by chance. In the
original code, I am using .values, so the order of fields does not matter
that much and I reorganised them I the order *fields, *annotations.
That will probably work for me. I do hope that I don't have other places
where this fails silently ;).
thanks again.
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Thierry Bastian | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Jacob Rief (added)
* has_patch: 0 => 1
Comment:
PR to prove that this has been fixed:
https://github.com/django/django/pull/18743
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Thierry Bastian | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
This was most likely a duplicate of #28900.
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Thierry Bastian | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Thierry Bastian | Owner: Simon
| Charette
(models, ORM) |
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"40bfd7b09aa0907b143e96f0b055538f476e544e" 40bfd7b0]:
{{{#!CommitTicketReference repository=""
revision="40bfd7b09aa0907b143e96f0b055538f476e544e"
Fixed #35011, Refs #28900 -- Added tests for QuerySet.union() with
multiple models and DateTimeField annotations.
Ticket was resolved by 65ad4ade74dc9208b9d686a451cd6045df0c9c3a as part of
#28900.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35011#comment:6>