[Django] #36875: Avoid unnecessary Coalesce in Concat/ConcatPair

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Normal
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
-------------------------------------+-------------------------------------
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>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#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:

New description:

--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:1>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------

Reporter: David | Owner: Rusheel
Type: | Chandra Reddy
Cleanup/optimization | Status: assigned

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
-------------------------------------+-------------------------------------

Changes (by Rusheel Chandra Reddy):

* owner: (none) => Rusheel Chandra Reddy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:2>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: Rusheel
Type: | Chandra Reddy
Cleanup/optimization | Status: assigned
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
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Before making any changes here we should consider the context from #25517,
#29582, #30385, #34955.

> if any involved expression have nullable output (which I understand can
be hard to check)

This is particularly relevant as the ORM doesn't do a good job at
differentiaing nullable expression, we can't use `expr.output_field.null`
as we don't update this flag transitively when performing outer joins.
That's the main reason why we systematically `Coalesce` each expression.
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:3>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: Rusheel
Type: | Chandra Reddy
Cleanup/optimization | Status: assigned
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
-------------------------------------+-------------------------------------

Comment (by David):

Hi Simon, I understand that there are other things which relies on
`COALESCE` to work, expecially in cross-database context where things can
break quite easy.

My proposal is to start by addressing the case in which it is involved a
`Value`, where it is simple to detect if it is/not NULL, thus avoiding the
`Coalesce(Value("a"), Value(""))` path.

Yet I realize that this may be a breaking change for people who relies con
`Concat` in their index/constraints and could be affected, however I
belive that it can be an improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:4>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: Rusheel
Type: | Chandra Reddy

Cleanup/optimization | Status: closed

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

Severity: Normal | Resolution: fixed

Keywords: concat,coalese | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

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

Changes (by Rusheel Chandra Reddy):

* has_patch: 0 => 1
* resolution: => fixed
* status: assigned => closed

Comment:

the link for the submitted pr is as follows:
https://github.com/django/django/pull/20567
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:5>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: Rusheel
Type: | Chandra Reddy

Cleanup/optimization | Status: new
Component: Database layer | Version: dev

(models, ORM) |
Severity: Normal | Resolution:

Keywords: concat,coalese | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: fixed =>
* status: closed => new
* version: 5.2 => dev

Comment:

We close tickets when merging fixes. Also, submitting a PR is premature at
this point as the triage is tending against acceptance so far.
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:6>

[Django]

#36875: Avoid unnecessary Coalesce in Concat/ConcatPair
-------------------------------------+-------------------------------------
Reporter: David | Owner: Rusheel
Type: | Chandra Reddy
Cleanup/optimization | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat,coalese | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rusheel Chandra Reddy):

Thank you for the clarification regarding the workflow; I will leave the
ticket status open in the future.Regarding the acceptance: I understood
this to be a valid optimization/issue .Could you clarify the concerns
regarding acceptance? I am happy to close the PR if we decide this change
isn't beneficial for the project, but I'd love to understand the reasoning
first.
--
Ticket URL: <https://code.djangoproject.com/ticket/36875#comment:7>