#28560 - distinct() on ordered queryset with restricted list of columns returns incorrect result

[Tobias McNulty]

Hi all,

Starting a thread on ticket #28560, "distinct() on ordered queryset with restricted list of columns returns incorrect result." In a nutshell:

$ cat testapp/models.py 

from django.db import models

class School(models.Model):

    name = models.CharField(max_length=255)

    county = models.CharField(max_length=255)

    class Meta:

        ordering = ("name",)

$ python manage.py shell

...

>>> from testapp.models import School

>>> str(School.objects.values("county").distinct().query)

'SELECT DISTINCT "testapp_school"."county", "testapp_school"."name" FROM "testapp_school" ORDER BY "testapp_school"."name" ASC'

Note the "name" column is added implicitly to the SELECT clause due to the default ordering on the model (I believe with good reason, since #7070). This is not specific to a default ordering; it's also possible to generate unintended results with a mismatching list of columns between an explicit order_by() and values().

It's possible to fix with an empty order_by():

>>> str(School.objects.values("county").order_by().distinct().query)

'SELECT DISTINCT "testapp_school"."county" FROM "testapp_school"'

But, this still feels like a case where we could do better. Some potential options:

1. It looks like there was an initial attempt to fix the issue with a subquery, but from what I can tell it was not possible to preserve ordering in the outer query.
2. My colleague Dmitriy pointed out that there may be a precedent for excluding the default ordering for queries like this.
3. An option I suggested on the ticket is to raise an error if the list of columns in values() is insufficient to run the requested query (i.e., never add a column implicitly if the user specified a list of columns via values() or values_list()).

What do others think? What are other potential fixes I'm not thinking of?

Cheers,

Tobias McNulty
Chief Executive Officer

tob...@caktusgroup.com
www.caktusgroup.com

[Adam Johnson]

I’d vote for option 2. I don’t think it can be expected the ordering will be obeyed when not selecting the columns it includes.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMGFDKRQsbUz%3DeYKxX2U%2B892AgQbXr%3DMA9AiWw9vfiPcM%2BnAXw%40mail.gmail.com.

--

Adam

[Tobias McNulty]

On Wed, Oct 28, 2020 at 3:38 PM Adam Johnson <m...@adamj.eu> wrote:

I’d vote for option 2. I don’t think it can be expected the ordering will be obeyed when not selecting the columns it includes.

Thanks! To make sure I understand, are you suggesting we should leave the behavior with an explicit order_by() as-is, or not add "name" to the SELECT in this case either?

>>> str(School.objects.values("county").order_by("name").distinct().query)

'SELECT DISTINCT "testapp_school"."county", "testapp_school"."name" FROM "testapp_school" ORDER BY "testapp_school"."name" ASC'

[Fran Hrženjak]

Personally I would prefer to get a ProgrammingError, so option 3. 

Explicit is better then implicit 😁 

Here is a great explanation of this exact issue, cleared it up for me:

https://blog.jooq.org/2018/07/13/how-sql-distinct-and-order-by-are-related/

On 28.10.2020., at 20:04, Tobias McNulty <tob...@caktusgroup.com> wrote:



--

[Tobias McNulty]

I tend to agree, though I'll note that was also a bug at one point: https://code.djangoproject.com/ticket/5321

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/942B7E2C-7B44-4ECF-9105-6EAB90874E98%40gmail.com.

[charettes]

I'm also a fan of option 3. to require an explicit opt-in or raise an error.

Not a lot of folks are familiar with this requirement imposed by the usage of DISTINCT and even if Model.Meta.ordering is the most common reason but unexpected ordering it can also be caused by the dynamic creation of a queryset through multiple abstractions (e.g. A DRF API filter backend that applies ordering).

I think this is a similar problem to https://code.djangoproject.com/ticket/14357#comment:11

Cheers,

Simon

[Michael Manfre]

Having run in to this issue in the past, automatically changing the meaning and/or breaking the query can turn in to a debug time sink. Option 3 could save people a bit of time.

Cheers,

Michael Manfre

[Adam Johnson]

I misunderstood the bug, the blog post Fran linked to has cleared it up for me. I think option 3 makes sense now.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/691662a4-9ad4-44a7-b2d2-1ecd0d95c30fn%40googlegroups.com.

--

Adam