[Django] #34176: Annotation's original field-name can clash with result field name over aggregation
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai | Owner: nobody
Berger |
Type: Bug | Status: new
Component: Database | Version: 4.1
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is a bit of a continuation of #34123
With current Django main, if one aggregates over a model, while adding
annotations of related fields; and an annotation comes from a field with
the same name as a result field, it breaks.
This is easier to explain by example; consider this test, added to the
{{{aggregation_regress}}} tests, so it uses their models; {{{Book}}}
carries a FK to {{{Publisher}}}, a M2M to {{{Author}}} named
{{{authors}}}, and a FK to {{{Author}}} (supposedly picking one of the
related authors) named {{{contact}}}.
{{{#!python
def test_aggregate_and_annotate_duplicate_columns(self):
results = Book.objects.values('isbn').annotate(
name=F('publisher__name'),
contact_name=F('contact__name'),
num_authors=Count('authors'),
)
self.assertTrue(results)
}}}
The field {{{isbn}}} is not defined as unique, it was chosen in order to
make sure there's an aggregation on the main table and not some sort of
subquery -- and also, to get the {{{Book}}} model's own {{{name}}} field
out of the way.
This blows up on Sqlite:
{{{
======================================================================
ERROR: test_aggregate_and_annotate_duplicate_columns
(aggregation_regress.tests.AggregationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/django/django/django/db/backends/utils.py", line 89, in
_execute
return self.cursor.execute(sql, params)
File "/home/django/django/django/db/backends/sqlite3/base.py", line 378,
in execute
return super().execute(query, params)
sqlite3.OperationalError: ambiguous column name: name
}}}
and on Postgres
{{{
======================================================================
ERROR: test_aggregate_and_annotate_duplicate_columns
(aggregation_regress.tests.AggregationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/django/django/django/db/backends/utils.py", line 89, in
_execute
return self.cursor.execute(sql, params)
psycopg2.errors.AmbiguousColumn: column reference "name" is ambiguous
LINE 1: ..._id") GROUP BY "aggregation_regress_book"."isbn", "name", "c...
^
}}}
Note that if we change the {{{name}}} above to something else -- e.g.
{{{#!python
pub_name=F('publisher__name'),
}}}
then things seem to be working fine. Conversely, if we pick another
related field for the name annotation:
{{{#!python
name=F('publisher__num_awards'),
}}}
then things stay broken.
This used to work, and now it doesn't. I've bisected this regression to
b7b28c7c189615543218e81319473888bc46d831.
Thanks [https://www.matific.com Matific] for letting me work on this.
--
Ticket URL: <https://code.djangoproject.com/ticket/34176>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
aggregation
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 4.1 => dev
* stage: Unreviewed => Accepted
Comment:
Thanks for the report!
Reproduced at 744a1af7f943106e30d538e6ace55c2c66ccd791.
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:1>
Django
aggregation
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | 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 => Simon Charette
* status: new => assigned
Comment:
Thanks for the report Shai.
I stumbled upon something similar in #34123 when trying to have
`get_select` use the `values` aliases instead of `colN` ones for aesthetic
reasons.
Changes are required in `SQLCompiler.get_select` that are similar to
[https://github.com/django/django/blob/7d5329852f19c6ae78c6f6f3d3e41835377bf295/django/db/models/sql/query.py#L2221-L2245
the ones] in `sql.Query.set_group_by(allow_aliases=True)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:2>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* has_patch: 0 => 1
Comment:
I didn't know that
{{{#!python
Book.objects.values('isbn').annotate(
name=F('publisher__name'),
)
}}}
was allowed but
{{{#!python
> Book.objects.values('isbn', name=F('publisher__name'))
ValueError: The annotation 'name' conflicts with a field on the model.
}}}
isn't which seems like a bug in #25871?
Anyway, I submitted [https://github.com/django/django/pull/16321 this MR]
for review which avoids a complex refactor of `get_select` that would have
necessitated reference repointing.
This means that group by reference will be disabled for annotations that
conflict will a selected column name but I think that's an acceptable
compromise given it should only have an incidence for with complex
annotations that can't be collapsed assigned with ambiguous names. This a
compromise we already took in #31377 as well and has a workaround for
users that must get the group be reference; use an annotation name that
doesn't conflict with an already selected column name.
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:3>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
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/34176#comment:4>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
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
-------------------------------------+-------------------------------------
* cc: Paolo Melchiorre (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:5>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:6>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | 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/34176#comment:7>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Component: Database layer | Version: dev
(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:"dd68af62b2b27ece50d434f6a351877212e15c3f" dd68af62]:
{{{
#!CommitTicketReference repository=""
revision="dd68af62b2b27ece50d434f6a351877212e15c3f"
Fixed #34176 -- Fixed grouping by ambiguous aliases.
Regression in b7b28c7c189615543218e81319473888bc46d831.
Refs #31377.
Thanks Shai Berger for the report and reviews.
test_aggregation_subquery_annotation_values_collision() has been
updated as queries that are explicitly grouped by a subquery should
always be grouped by it and not its outer columns even if its alias
collides with referenced table columns. This was not possible to
accomplish at the time 10866a10 landed because we didn't have compiler
level handling of colliding aliases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:8>
Django
aggregation
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution: fixed
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
-------------------------------------+-------------------------------------
Comment (by Simon Charette <charette.s@…>):
In [changeset:"f91e085c3054240dbb581c1db9b4a9063c26525c" f91e085]:
{{{
#!CommitTicketReference repository=""
revision="f91e085c3054240dbb581c1db9b4a9063c26525c"
Refs #34176 -- Adjusted group by position variables naming to follow SQL
spec.
This avoids conceptual collisions with the notion of indices.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34176#comment:9>