#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat,coalese | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by David:
Old description:
> Currently `ConcatPair` (which is used by `Concat`) when invoking the
> `.coalesce` method it will force the `COALESCE` on every argument
> provided to the expression.
>
> https://github.com/django/django/blob/3851601b2e080df34fb9227fe5d2fd43af604263/django/db/models/functions/text.py#L112-L122
>
> This does not take into account:
>
> - if any involved expression have nullable output (which I understand can
> be hard to check)
> - if any involved expression is a fixed value (which should easier to
> detect)
>
> The case in which I am more interested is the latter, because it is
> completely useless to invoke `COALESCE` on a fixed value (unless it was
> provided `NULL` from the beginning, which should be easy to check).
>
> I came along this while writing a query to detect custom permissions on
> the database:
>
> {{{
> from django.contrib.auth.models import Permission
> from django.db.models import Value, F
> from django.db.models.functions import Concat
>
> custom_permissions = Permission.objects.exclude(
> codename__regex=Concat(
> Value("^(add|change|delete|view)_"),
> F("content_type__model"),
> Value("$"),
> )
> )
> }}}
>
> I was expecting a WHERE clause like:
>
> {{{
> "auth_permission"."codename"::text ~ ('^(add|change|delete|view)_' ||
> "django_content_type"."model" || '$')
> }}}
>
> And I was stunned when I saw the following (which is hard to read):
>
> {{{
> "auth_permission"."codename"::text ~
> (COALESCE('^(add|change|delete|view)_', '') ||
> COALESCE((COALESCE("django_content_type"."model", '') || COALESCE('$',
> '')), ''))
> }}}
>
> My proposal is to change how `ConcatPair.coalesce` behaves to detect if
> `COALESCE` can be skipped (ie: fixed value, already enforced coalesce on
> previous expression, etc) before putting that function in the output.
> This could improve readability for generated queries and may reduce the
> overhad of building the `Coalesce` expression.
New description:
Currently `ConcatPair` (which is used by `Concat`) when invoking the
`.coalesce` method it will force the `COALESCE` on every argument provided
to the expression.
https://github.com/django/django/blob/3851601b2e080df34fb9227fe5d2fd43af604263/django/db/models/functions/text.py#L112-L122
This does not take into account:
- if any involved expression have nullable output (which I understand can
be hard to check)
- if any involved expression is a fixed value (which should easier to
detect)
The case in which I am more interested is the latter, because it is
completely useless to invoke `COALESCE` on a fixed value (unless it was
provided `NULL` from the beginning, which should be easy to check).
I came along this while writing a query to detect custom permissions on
the database:
{{{
from django.contrib.auth.models import Permission
from django.db.models import Value, F
from django.db.models.functions import Concat
custom_permissions = Permission.objects.exclude(
codename__regex=Concat(
Value("^(add|change|delete|view)_"),
F("content_type__model"),
Value("$"),
)
)
}}}
I was expecting a WHERE clause like:
{{{
"auth_permission"."codename"::text ~ ('^(add|change|delete|view)_' ||
"django_content_type"."model" || '$')
}}}
And I was stunned when I saw the following (which is hard to read):
{{{
"auth_permission"."codename"::text ~
(COALESCE('^(add|change|delete|view)_', '') ||
COALESCE((COALESCE("django_content_type"."model", '') || COALESCE('$',
'')), ''))
}}}
My proposal is to change how `ConcatPair.coalesce` behaves to detect if
`COALESCE` can be skipped (ie: fixed value, already enforced coalesce on
previous expression, etc) before putting that function in the output.
This could improve readability for generated queries and may reduce the
overhad of building the `Coalesce` expression.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019be134db04-c1dd16a1-d8e5-437c-a8cf-626c2d79b74d-000000%40eu-central-1.amazonses.com.
