Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#28038 closed Bug (fixed)

Cast regression in PostgresSQL using built-in lookups

Reported by: Peter J. Farrell Owned by: Simon Charette <charette.s@…>
Component: Database layer (models, ORM) Version: 1.11
Severity: Release blocker Keywords: postgres postgresql array_agg
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The following query works in Django 1.10 or less but fails on Django 1.11 with the following error message:

function upper(character varying[]) does not exist... No function matches the given name and argument types. You might need to add explicit type casts.
query = self.filter(
    category=category
).annotate(
    arm_types=ArrayAgg('assets__arm_type')
).filter(
    arm_types__icontains=arm_type
)

The change is that the filter using the icontains no longer casts to ::text. It appears this is regression that is caused by this commit:

https://github.com/django/django/commit/658f1e81a7ea29acf30bbf8f61ffec2beff24639#diff-8e3817dff3483fe3eaecb84d56803e32

The SQL generated from Django 1.10 looks like this (wheres 1.11 no longer includes the ::text cast):

UPPER(ARRAY_AGG("asset_furnitureasset"."arm_type")::text) LIKE
UPPER('%Without Arms%') ORDER BY "asset_furniture"."make_model" ASC

Change History (7)

comment:1 Changed 2 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

I don't have the time to look at this right but it looks like a bug in your code IMHO. Wouldn't using StringAgg be more appropriate here?

comment:2 in reply to:  1 Changed 2 years ago by Peter J. Farrell

Replying to Simon Charette:

I don't have the time to look at this right but it looks like a bug in your code IMHO. Wouldn't using StringAgg be more appropriate here?

It's definitely a regression from the behavior in 1.10 and a blocker to upgrade apps. We would have to find all the usages of ArrayAgg and convert them. In this case, we haven't found any lookups work in conjunction with ArrayAgg in Django 1.11 whereas they worked in 1.10. It would have to be documented that which lookups work with ArrayAgg in Django 1.11.

Plus, it much easier to iterator over the array/list of arm_types than having to convert a delimited string into something that can be iterated over.

comment:3 Changed 2 years ago by Simon Charette

Has patch: set
Owner: Simon Charette deleted
Severity: NormalRelease blocker
Status: assignednew
Triage Stage: UnreviewedAccepted

It's a shame we expose builtin lookups such as __icontains on these kind of fields as it produces really inefficient SQL, a real footgun API if you ask me.

I think we'd be doing developers a favor in erroring instead and forcing them to choose an implementation that best fit their need. That is using something along EXISTS(SELECT 1 FROM unnest(array_field) key WHERE UPPER(key) = UPPER('lookup')) on small datasets or relying on a GIN indexed functional lookup instead on large ones.

Anyway, here's the PR.

Last edited 2 years ago by Simon Charette (previous) (diff)

comment:4 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:5 in reply to:  3 Changed 2 years ago by Peter J. Farrell

Replying to Simon Charette:

Anyway, here's the PR.

Thanks for the fix.

comment:6 Changed 2 years ago by Simon Charette <charette.s@…>

Owner: set to Simon Charette <charette.s@…>
Resolution: fixed
Status: newclosed

In a354c690:

Fixed #28038 -- Restored casting to text of builtin lookups on PostgreSQL.

Reverted 658f1e8 which broke code using icontains's implicit cast to ::text
on ArrayField.

Thanks Peter J. Farrell for the report.

comment:7 Changed 2 years ago by Simon Charette <charette.s@…>

In 5d35e8eb:

[1.11.x] Fixed #28038 -- Restored casting to text of builtin lookups on PostgreSQL.

Reverted 658f1e8 which broke code using icontains's implicit cast to ::text
on ArrayField.

Thanks Peter J. Farrell for the report.

Backport of a354c69055fd818e612ce22eaa2da0576a4b89ee from master

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