Opened 7 years ago

Last modified 4 years ago

#28560 new Bug

distinct() on ordered queryset with restricted list of columns returns incorrect result — at Version 15

Reported by: Mariusz Felisiak Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: distinct values
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tobias McNulty)

When distinct() is used with values() (or values_list()) on ordered queryset and a list of fields in values() doesn't contain all fields from ORDER BY, then it doesn't return correct result because columns from ORDER BY clause must be included in SELECT.

After some discussion on the mailing list (https://groups.google.com/g/django-developers/c/DNVRFqVBsfk/m/xDUvaq3DAAAJ) it looks like the consensus is to require an explicit opt-in or raise an error (i.e., never add a column implicitly if the user specified a list of columns via values() or values_list()).

Change History (14)

comment:2 by Mariusz Felisiak, 7 years ago

Summary: distinct() on None valuesdistinct() on ordered queryset with restricted list of columns returns incorrect result

comment:3 by Mariusz Felisiak, 7 years ago

Description: modified (diff)

comment:4 by Simon Charette, 7 years ago

Couldn't we wrap the query in a subquery like we did in #24254 in this case?

comment:5 by Mariusz Felisiak, 7 years ago

Good idea! it should be feasible. I will try to prepare patch in this week.

comment:6 by Simon Charette, 7 years ago

By the way Mariusz, do you have an opinion on https://code.djangoproject.com/ticket/14357#comment:11?

comment:7 by Mariusz Felisiak, 7 years ago

Description: modified (diff)

comment:8 by Mariusz Felisiak, 7 years ago

Description: modified (diff)

comment:9 by Mariusz Felisiak, 7 years ago

Has patch: set

comment:10 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:11 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:12 by Mariusz Felisiak, 7 years ago

Has patch: unset
Owner: Mariusz Felisiak removed
Patch needs improvement: unset
Status: assignednew

comment:13 by Tobias McNulty, 4 years ago

Should/could we force the user to include in values() any ordering columns required in the SELECT per #7070?

While this doesn't fix the issue per se (though I'm not sure fixing it is possible...), it seems most explicit for the user and consistent with the Postgres error noted in #5321.

Last edited 4 years ago by Tobias McNulty (previous) (diff)

comment:14 by Tobias McNulty, 4 years ago

After some discussion on the mailing list (https://groups.google.com/g/django-developers/c/DNVRFqVBsfk/m/xDUvaq3DAAAJ) it looks like the consensus is to require an explicit opt-in or raise an error (i.e., never add a column implicitly if the user specified a list of columns via values() or values_list()).

I am not too familiar with the ORM. I believe these columns are added in get_extra_select() (django/db/models/sql/compiler.py), which could be adapted initially to raise a deprecation warning and eventually an exception. Does that sound right?

I may not have time right away so if anyone feels like picking this up, go for it.

comment:15 by Tobias McNulty, 4 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top