#30488 closed Cleanup/optimization (fixed)
SearchVector lookup is generating redundent Coalesce wrapper.
Reported by: | T Lee | Owned by: | T Lee |
---|---|---|---|
Component: | contrib.postgres | Version: | 2.2 |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In Django 2.2.1, in the SearchVector if we do:
>>> from django.contrib.postgres.search import SearchVector >>> Entry.objects.annotate( ... search=SearchVector('body_text', 'summary_text'), ... ).filter(search='Cheese')
The generated SQL currently looks something like
SELECT id /*......*/ FROM "app_entry" WHERE to_tsvector(COALESCE(COALESCE("app_entry"."body_text", ''), '') || ' ' || COALESCE(COALESCE("app_entry"."summary_text", ''), '')) @@ (plainto_tsquery('Cheese')) = true
i.e. in the WHERE
clause, the fields is wrapped twice with COALESCE
, .e.g. COALESCE(COALESCE("app_entry"."body_text", ), )
While it is still possible to create a functional index with this, the generated SQL does not feel optimal at the very least.
It will be great if the generated SQL will keep to one level of COALESCE
, .e.g. COALESCE("app_entry"."body_text", ), )
Change History (9)
comment:1 Changed 5 years ago by
Cc: | Simon Charette added |
---|---|
Summary: | SearchVector lookup is generating redundent Coalesce wrapper → SearchVector lookup is generating redundent Coalesce wrapper. |
Triage Stage: | Unreviewed → Accepted |
Version: | 2.2 → master |
comment:2 follow-up: 3 Changed 5 years ago by
Owner: | set to Simon Charette |
---|---|
Status: | new → assigned |
comment:3 Changed 5 years ago by
Sorry I didn't know I need to claim the ticket first. Anyhow here is a pull request https://github.com/django/django/pull/11380 if it can be used. Thanks.
comment:4 Changed 5 years ago by
No worries, feel free to assign the ticket to you. The patch seems good but you'll probably want to add an addition regression test which might require building a queryset and asserting against self.assertNotIn('COALESCE(COALESCE', str(qs.query))
. You'll want to add a mention in the 2.2.2 release notes as well.
comment:5 Changed 5 years ago by
Owner: | changed from Simon Charette to T Lee |
---|
Thanks. I have claimed the ticket, and have added both regression test and 2.2.2 release mention in PR
comment:6 Changed 5 years ago by
Has patch: | set |
---|
comment:7 Changed 5 years ago by
Version: | master → 2.2 |
---|
Thanks for the report. It looks that we resolve expression twice.