[Django] #34963: Recursive and other "combinator" queries broken in django-cte
Django
-----------------------------------------+------------------------
Reporter: Daniel Miller | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
[https://code.djangoproject.com/changeset/70499b25c708557fb9ee2264686cd172f4b2354e
70499b2] in #34123 [https://github.com/dimagi/django-cte/issues/66 broke
django-cte]. The problem is caused by
`compiler.as_sql(with_col_aliases=True)` in `get_combinator_sql`:
`with_col_aliases=True` breaks the ability to reference CTE query columns
by name on Django 4.2.
--
Ticket URL: <https://code.djangoproject.com/ticket/34963>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Daniel Miller | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Sanders):
* cc: David Sanders (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34963#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: Daniel Miller | Owner: nobody
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* cc: Mariusz Felisiak, Simon Charette (added)
* status: new => closed
* resolution: => invalid
Comment:
Hello Daniel, thanks for your report.
I read the referenced issues and skimmed the related PRs. As far as I
know, there is no much that can be done in the Django side, this ticket
tracker is to track bugs and issues with Django itself and from the
provided description and related links, it's unclear how/if Django has a
bug in its code. I understand that the referenced revision adding the
column aliases broke `django-cte`, but it legitimately fixed confirmed
issues *within* Django, so following the current
[https://docs.djangoproject.com/en/4.2/internals/contributing/triaging-
tickets/#closing-tickets ticket triaging guidelines], I think we need to
close this report as invalid.
I'll cc Mariusz and Simon who were involved in fixing ticket #34123 in
case they disagree with this resolution, or in case they could provide
insights about possible fixes for `django-cte` (though the latter would be
more appropriate to be [https://forum.djangoproject.com/c/internals/5 a
new post in the Django Internals forum]).
--
Ticket URL: <https://code.djangoproject.com/ticket/34963#comment:2>
Django
-------------------------------+--------------------------------------
Reporter: Daniel Miller | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Daniel Miller):
Hi Natalia, thanks for your prompt response.
I could have done a better job explaining why I think this is a bug in
Django. As of
[https://code.djangoproject.com/changeset/70499b25c708557fb9ee2264686cd172f4b2354e
70499b2] all "combinator" queries (queries using `UNION`, etc.) now have
compiler-generated aliases `col1`, `col2`, ... applied to unaliased^1^
columns. This change was made to support a small subset of queries that
use a combinator with `ORDER BY` and `select_related`. However, the fix as
implemented also unnecessarily affects all other combinator queries as
well. Frankly I am surprised that it did not break anything else outside
of django-cte. Is there a way to implement a fix so that it only affects
queries where aliases are needed?
In my attempts to fix the issue in django-cte I have been unable to find a
way to cleanly disable compiler-level aliasing just for CTE queries where
it breaks the ability to reference columns by name. One reason is that
many ORM methods like `get_combinator_sql` are large and overriding them
is difficult or impossible to do in a maintainable way. I am open to
suggestions for how to accomplish this within django-cte if there is a
good way to do that.
I think it is in Django's interest to make it possible for projects like
django-cte to extend the functionality of the ORM with new features, since
the alternative is to have all such features implemented internally and
maintained as part of Django or have them be repeatedly broken as Django
evolves over time.
^1^ The definition of "unaliased" here is a bit murky. There are at least
two ways a column can be aliased: explicitly at the Query level, or
implicitly at the Compiler level when `with_col_aliases=True`. Compiler-
level aliases do not mask or override Query-level aliases. My
understanding of "unaliased" is all columns without a Query-level alias.
--
Ticket URL: <https://code.djangoproject.com/ticket/34963#comment:3>
Django
-------------------------------+--------------------------------------
Reporter: Daniel Miller | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Natalia Bidart):
Thank you Daniel for the extra rationale, that definitely helps. Mariusz
is currently returning from DjangoCon Africa, but I'm sure he'll share his
opinion next week when catching up with Django things. Simon also reads
issues periodically, so I'm very keen to wait and read their thoughts
since they are the experts of this Django area.
--
Ticket URL: <https://code.djangoproject.com/ticket/34963#comment:4>
Django
-------------------------------+--------------------------------------
Reporter: Daniel Miller | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Simon Charette):
Hello Daniel!
I sympathize with the desire to avoid duplicating large chunks of code in
django-cte and I appreciate that you maintain such a solution but asking
for the development team to maintain compatibility with its internal
interface when adjusting bugs of the interface it maintains is not
sustainable particularly for regressions in versions that have been
released for a few months already. When making these changes we try to
think of ways that are not too invasive but there's only so much we can do
to account for all cases.
I'm very much open to reviewing patches that would reduce the maintenance
burden that you face (is there a kwarg that can be passed? a different
function breakdown be used?) in `main` or that implements smarter way of
doing things that avoids unnecessary aliasing such as proper `ORDER BY
$index` support but stability of the ORM internals cannot be guaranteed by
team in its current state.
> Is there a way to implement a fix so that it only affects queries where
aliases are needed?
I believe that if you build and maintain a package that relies on the
stability of ORM internals you must be prepared to deal with regressions
against `main` and suggest alternative solutions when they happen. I'm
afraid the ship has already sailed for the 4.2 backport support window and
I'm not even sure it would qualify given the internal nature of SQL
compilation.
--
Ticket URL: <https://code.djangoproject.com/ticket/34963#comment:5>