Re: [Django] #33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

Thanks for the quick answer.

Your suggested workaround seems interesting indeed (provided the value of
said column does not affect the exists() return value - for example if
it's 0?)

NB: I didn't mean removing all constants from GROUP BY, rather: removing
the automatic transmission of constants from the SELECT clause to the
GROUP BY clause, since their meaning automatically switches from "constant
number n" to "nth element of the select clause" which imo can never be
intended.

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

> provided the value of said column does not affect the exists() return
value - for example if it's 0?

No, it's not. It's enough for the query to return any rows.

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:3>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | 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):

Maybe we should stop relying on ''extra'' and use `Value(1)` instead
[https://github.com/django/django/blob/ed0a2c3238aa0b9c0d01c436d5bcd70930d696b0/django/db/models/expressions.py#L990-L991
which already disables grouping].

{{{#!diff
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 64e7927f7a..1097de4f79 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -27,6 +27,7 @@
OuterRef,
Ref,
ResolvedOuterRef,
+ Value,
)
from django.db.models.fields import Field
from django.db.models.fields.related_lookups import MultiColSource
@@ -582,8 +583,7 @@ def exists(self, using, limit=True):
q.clear_ordering(force=True)
if limit:
q.set_limits(high=1)
- q.add_extra({"a": 1}, None, None, None, None, None)
- q.set_extra_mask(["a"])
+ q.add_annotation(Value(1), "a")
return q

def has_results(self, using):
}}}

Anything that relies on ''extra'' is a black box to the compiler as it
could be an aggregate function or not.

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:4>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:4 Simon Charette]:

> Maybe we should stop relying on ''extra'' and use `Value(1)` instead
[https://github.com/django/django/blob/ed0a2c3238aa0b9c0d01c436d5bcd70930d696b0/django/db/models/expressions.py#L990-L991
which already disables grouping].

Great idea!

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:5>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

Sounds good indeed!

Replying to [comment:5 Mariusz Felisiak]:

> Replying to [comment:4 Simon Charette]:
> > Maybe we should stop relying on ''extra'' and use `Value(1)` instead
[https://github.com/django/django/blob/ed0a2c3238aa0b9c0d01c436d5bcd70930d696b0/django/db/models/expressions.py#L990-L991
which already disables grouping].
>
> Great idea!

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:6>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Marc, Would you like to prepare a patch?

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:7>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

Yeah, I guess I can do that, with Simon's suggestion the whole thing
should be straightforward enough.
I'll take the ticket - I'll have to look up the contribution guide etc. /
especially about the tests, as I haven't really contributed before.

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:8>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------

Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned

Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Marc Perrin):

* owner: nobody => Marc Perrin
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:9>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:8 Marc Perrin]:

> Yeah, I guess I can do that, with Simon's suggestion the whole thing
should be straightforward enough.
> I'll take the ticket - I'll have to look up the contribution guide etc.
/ especially about the tests, as I haven't really contributed before.

Great, thanks! In this ticket we are forced to check a generated SQL, IMO
it's fine to add assertions to an existing test e.g.
`aggregation.tests.AggregateTestCase.test_aggregation_subquery_annotation_exists`:
{{{#!python
with self.assertNumQueries(1) as ctx:
self.assertTrue(publisher_qs.exists())
sql = ctx[0]["sql"]
self.assertIn("SELECT 1 ", sql)
self.assertNotIn("(1)", sql)
# There is just one GROUP BY clause (zero commas means at most one
# clause).
self.assertEqual(sql[sql.index("GROUP BY") :].count(", "), 0)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:10>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

Yeah, looks like the most relevant existing test case indeed, thanks for
the pointer

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:11>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

Though after debugging this test, it might be a bit more complex than we
want/need (e.g. there are 4 fields in the GROUP BY anyway...) and I might
just add a new test e.g. "test_aggregation_annotation_exists" that gets
close to what I mentioned originally (e.g.

{{{
Book.objects.values('publisher').annotate(cnt=Count('isbn')).filter(cnt__gt=1).exists()
}}}

)

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:12>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: exists group by | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Marc Perrin):

But anyway I've started the PR:
https://github.com/django/django/pull/15630 se we can discuss the details
in there

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:13>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: exists group by | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* has_patch: 0 => 1
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:14>

[Django]

#33655: Unnecessary column in a GROUP BY clause with QuerySet.exists()
-------------------------------------+-------------------------------------
Reporter: Marc Perrin | Owner: Marc
Type: | Perrin

Cleanup/optimization | Status: closed

Component: Database layer | Version: 4.0
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: exists group by | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"4282fd468fd11a7db79307ba1c33a4960c299e4a" 4282fd4]:
{{{
#!CommitTicketReference repository=""
revision="4282fd468fd11a7db79307ba1c33a4960c299e4a"
Fixed #33655 -- Removed unnecessary constant from GROUP BY clause for
QuerySet.exists().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/33655#comment:15>