Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#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 Mariusz Felisiak

Cc: Simon Charette added
Summary: SearchVector lookup is generating redundent Coalesce wrapperSearchVector lookup is generating redundent Coalesce wrapper.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Thanks for the report. It looks that we resolve expression twice.

comment:2 Changed 5 years ago by Simon Charette

Owner: set to Simon Charette
Status: newassigned

comment:3 in reply to:  2 Changed 5 years ago by T Lee

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 Simon Charette

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 T Lee

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 Mariusz Felisiak

Has patch: set

comment:7 Changed 5 years ago by Mariusz Felisiak

Version: master2.2

comment:8 Changed 5 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In c38e7a79:

Fixed #30488 -- Removed redundant Coalesce call in SQL generated by SearchVector.

Regression in 405c8363362063542e9e79beac53c8437d389520.

comment:9 Changed 5 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 3d4e53bc:

[2.2.x] Fixed #30488 -- Removed redundant Coalesce call in SQL generated by SearchVector.

Regression in 405c8363362063542e9e79beac53c8437d389520.

Backport of c38e7a79f4354ee831f92deb7a658fc0387e3bec from master

Note: See TracTickets for help on using tickets.
Back to Top