[Django] #30961: Use proper whitespace in CREATE INDEX statements
Django
-------------------------------------+-------------------------------------
Reporter: Hannes | Owner: nobody
Ljungberg |
Type: Bug | Status: assigned
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords: db-indexes
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Creating an index through:
{{{
index = Index(
fields=['-name’],
name='idx'
)
}}}
Will generate the valid but not so pretty `CREATE INDEX` statement:
{{{
CREATE INDEX "idx" ON "schema_author" ("name"DESC)
}}}
The following would be expected:
{{{
CREATE INDEX "idx" ON "schema_author" ("name" DESC)
}}}
This was partially fixed for indexes using `opclasses` in
https://code.djangoproject.com/ticket/30903#ticket but it introduced a new
quirk when `opclasses` is used:
{{{
index = Index(
fields=['name’],
name='idx'
opclasses=['text_pattern_ops’]
)
}}}
Will result in:
{{{
CREATE INDEX "idx" ON "schema_author" (“name” text_pattern_ops )
}}}
Note the whitespace after `text_pattern_ops`. When used with a descending
order it will look correct.
Unfortunately in the fix in #30903 it was assumed that the `col_suffixes`
passed to `django.db.backends.ddl_references.Columns` would be empty for
ascending order but instead it will contain empty strings and thus causing
this bug. See:
https://github.com/django/django/blob/master/django/db/backends/ddl_references.py#L87
The expected output would be:
{{{
CREATE INDEX "idx" ON "schema_author" (“name” text_pattern_ops)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30961>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Ljungberg
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Hannes Ljungberg
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:1>
Django
-------------------------------------+-------------------------------------
| Ljungberg
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/12039
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:2>
Django
-------------------------------------+-------------------------------------
| Ljungberg
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
Creating an index through:
{{{
index = Index(
fields=['-name’],
name='idx'
)
}}}
Will generate the valid but not so pretty `CREATE INDEX` statement:
{{{
CREATE INDEX "idx" ON "schema_author" ("name"DESC)
}}}
The following would be expected:
{{{
CREATE INDEX "idx" ON "schema_author" ("name" DESC)
}}}
This was partially fixed for indexes using `opclasses` in
https://code.djangoproject.com/ticket/30903#ticket but it introduced a new
quirk when `opclasses` is used without explicit ordering:
{{{
index = Index(
fields=['name’],
name='idx'
opclasses=['text_pattern_ops’]
)
}}}
Will result in:
{{{
CREATE INDEX "idx" ON "schema_author" (“name” text_pattern_ops )
}}}
Note the whitespace after `text_pattern_ops`. When used with a descending
order it will look correct.
Unfortunately in the fix in #30903 it was assumed that the `col_suffixes`
passed to `django.db.backends.ddl_references.Columns` would be empty for
ascending order but instead it will contain empty strings and thus causing
this bug. See:
https://github.com/django/django/blob/master/django/db/backends/ddl_references.py#L87
The expected output would be:
{{{
CREATE INDEX "idx" ON "schema_author" (“name” text_pattern_ops)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
OK, I'll Accept this as a clean up (there's not actually an error right?).
The patch looks small/clean enough at first glance.
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hannes Ljungberg):
Correct, no error. But since it's not documented as valid syntax to not
have whitespace between the column and the ordering it could break in
future database versions.
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* version: 2.2 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d5af43c8d1de7bc21e6e01f31e711c02a5059a5a" d5af43c8]:
{{{
#!CommitTicketReference repository=""
revision="d5af43c8d1de7bc21e6e01f31e711c02a5059a5a"
Refs #30961 -- Added tests for columns list SQL generated for indexes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Component: Database layer | Version: master
(models, ORM) |
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6d590bcf1ff1d66990d03f8ee288e18110bdd995" 6d590bcf]:
{{{
#!CommitTicketReference repository=""
revision="6d590bcf1ff1d66990d03f8ee288e18110bdd995"
Fixed #30961 -- Fixed spaces in columns list SQL generated for indexes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: db-indexes | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Thanks for the fix. The lack of whitespace is invalid SQL on Google's
Cloud Spanner.
--
Ticket URL: <https://code.djangoproject.com/ticket/30961#comment:10>