Skip to content

Commit

Permalink
Fixed #33768 -- Fixed ordering compound queries by nulls_first/nulls_…
Browse files Browse the repository at this point in the history
…last on MySQL.

Columns of the left outer most select statement in a combined query
can be referenced by alias just like by index.

This removes combined query ordering by column index and avoids an
unnecessary usage of RawSQL which causes issues for backends that
specialize the treatment of null ordering.
  • Loading branch information
charettes authored and felixxm committed Oct 5, 2022
1 parent 344d31c commit c58a8ac
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 10 deletions.
1 change: 1 addition & 0 deletions django/db/backends/base/features.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class BaseDatabaseFeatures:
supports_select_difference = True
supports_slicing_ordering_in_compound = False
supports_parentheses_in_compound = True
requires_compound_order_by_subquery = False

# Does the database support SQL 2003 FILTER (WHERE ...) in aggregate
# expressions?
Expand Down
1 change: 1 addition & 0 deletions django/db/backends/oracle/features.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
supports_partial_indexes = False
can_rename_index = True
supports_slicing_ordering_in_compound = True
requires_compound_order_by_subquery = True
allows_multiple_constraints_on_same_fields = False
supports_boolean_expr_in_select_clause = False
supports_comparing_boolean_expr = False
Expand Down
21 changes: 11 additions & 10 deletions django/db/models/sql/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -435,21 +435,18 @@ def get_order_by(self):

for expr, is_ref in self._order_by_pairs():
resolved = expr.resolve_expression(self.query, allow_joins=True, reuse=None)
if self.query.combinator and self.select:
if not is_ref and self.query.combinator and self.select:
src = resolved.expression
expr_src = expr.expression
# Relabel order by columns to raw numbers if this is a combined
# query; necessary since the columns can't be referenced by the
# fully qualified name and the simple column names may collide.
for idx, (sel_expr, _, col_alias) in enumerate(self.select):
if is_ref and col_alias == src.refs:
src = src.source
elif col_alias and not (
for sel_expr, _, col_alias in self.select:
if col_alias and not (
isinstance(expr_src, F) and col_alias == expr_src.name
):
continue
if src == sel_expr:
resolved.set_source_expressions([RawSQL("%d" % (idx + 1), ())])
resolved.set_source_expressions(
[Ref(col_alias if col_alias else src.target.column, src)]
)
break
else:
if col_alias:
Expand Down Expand Up @@ -853,7 +850,11 @@ def as_sql(self, with_limits=True, with_col_aliases=False):
for _, (o_sql, o_params, _) in order_by:
ordering.append(o_sql)
params.extend(o_params)
result.append("ORDER BY %s" % ", ".join(ordering))
order_by_sql = "ORDER BY %s" % ", ".join(ordering)
if combinator and features.requires_compound_order_by_subquery:
result = ["SELECT * FROM (", *result, ")", order_by_sql]
else:
result.append(order_by_sql)

if with_limit_offset:
result.append(
Expand Down
18 changes: 18 additions & 0 deletions tests/queries/test_qs_combinators.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,24 @@ def test_union_none(self):
self.assertSequenceEqual(qs3.none(), [])
self.assertNumbersEqual(qs3, [0, 1, 8, 9], ordered=False)

def test_union_order_with_null_first_last(self):
Number.objects.filter(other_num=5).update(other_num=None)
qs1 = Number.objects.filter(num__lte=1)
qs2 = Number.objects.filter(num__gte=2)
qs3 = qs1.union(qs2)
self.assertSequenceEqual(
qs3.order_by(
F("other_num").asc(nulls_first=True),
).values_list("other_num", flat=True),
[None, 1, 2, 3, 4, 6, 7, 8, 9, 10],
)
self.assertSequenceEqual(
qs3.order_by(
F("other_num").asc(nulls_last=True),
).values_list("other_num", flat=True),
[1, 2, 3, 4, 6, 7, 8, 9, 10, None],
)

@skipUnlessDBFeature("supports_select_intersection")
def test_intersection_with_empty_qs(self):
qs1 = Number.objects.all()
Expand Down

0 comments on commit c58a8ac

Please sign in to comment.