Fixed #20600 -- ordered distinct(*fields) in subqueries #1722

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
1 participant
Member

akaariai commented Oct 10, 2013

No description provided.

@akaariai akaariai commented on the diff Oct 10, 2013

django/db/models/sql/compiler.py
ordering_aliases.append(elt)
result.append('%s %s' % (elt, order))
group_by.append((elt, []))
else:
elt = qn2(col)
if col not in self.query.extra_select:
- sql = "(%s) AS %s" % (self.query.extra[col][0], elt)
- ordering_aliases.append(sql)
- ordering_params.extend(self.query.extra[col][1])
+ if must_append_to_select:
+ sql = "(%s) AS %s" % (self.query.extra[col][0], elt)
+ ordering_aliases.append(sql)
+ ordering_params.extend(self.query.extra[col][1])
+ result.append('%s %s' % (elt, order))
+ else:
+ result.append("(%s) %s" % (self.query.extra[col][0], order))
+ params.extend(self.query.extra[col][1])
@akaariai

akaariai Oct 10, 2013

Member

Previously extra clauses where unconditionally appended to ordering_aliases (the stuff that ends up in the select) - but that isn't necessary, we can skip the append if this isn't plain distinct query.

@akaariai akaariai commented on the diff Oct 10, 2013

django/db/models/sql/compiler.py
@@ -364,6 +364,10 @@ def get_ordering(self):
params = []
ordering_params = []
+ # For plain DISTINCT queries any ORDER BY clause must appear
+ # in SELECT clause.
+ # http://www.postgresql.org/message-id/27009.1171559417@sss.pgh.pa.us
+ must_append_to_select = distinct and not self.query.distinct_fields
@akaariai

akaariai Oct 10, 2013

Member

We can't append the ordering clause to select for distinct(*fields) - that would mean having more than one field in the subquery's SELECT clause and that doesn't work. I also added a link to PostgreSQL mailing list to clarify why exactly we need to add ordering SQL into select clause in DISTINCT case. This was the best reference I could find.

@akaariai akaariai commented on the diff Oct 10, 2013

django/db/models/sql/compiler.py
@@ -164,7 +164,7 @@ def as_nested_sql(self):
Used when nesting this query inside another.
"""
obj = self.query.clone()
- if obj.low_mark == 0 and obj.high_mark is None:
+ if obj.low_mark == 0 and obj.high_mark is None and not self.query.distinct_fields:
@akaariai

akaariai Oct 10, 2013

Member

The original issue of this ticket - ordering needed for distinct(*fields) subqueries.

@akaariai akaariai commented on the diff Oct 10, 2013

django/db/models/sql/compiler.py
else:
- if distinct and col not in select_aliases:
- ordering_aliases.append(elt)
- ordering_params.extend(params)
@akaariai

akaariai Oct 10, 2013

Member

This branch is actually impossible to hit - to get here we need to have extra clause that isn't in self.query.extra_select and isn't in select_aliases - but everything in extra_select gets added to select_aliases.

@akaariai akaariai commented on the diff Oct 10, 2013

tests/extra_regress/tests.py
+
+ def test_extra_values_distinct_ordering(self):
+ t1 = TestObject.objects.create(first='a', second='a', third='a')
+ t2 = TestObject.objects.create(first='a', second='b', third='b')
+ qs = TestObject.objects.extra(
+ select={'second_extra': 'second'}
+ ).values_list('id', flat=True).distinct()
+ self.assertQuerysetEqual(
+ qs.order_by('second_extra'), [t1.pk, t2.pk], lambda x: x)
+ self.assertQuerysetEqual(
+ qs.order_by('-second_extra'), [t2.pk, t1.pk], lambda x: x)
+ # Note: the extra ordering must appear in select clause, so we get two
+ # non-distinct results here (this is on purpose, see #7070).
+ self.assertQuerysetEqual(
+ qs.order_by('-second_extra').values_list('first', flat=True),
+ ['a', 'a'], lambda x: x)
@akaariai

akaariai Oct 10, 2013

Member

This tests the col in extra_select + must_append_to_select branch.

Member

akaariai commented Nov 7, 2013

Merged manually.

akaariai closed this Nov 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment