From 21b858cb6735cdfdc695ff7b076e4cbc1981bc88 Mon Sep 17 00:00:00 2001 From: Josh Smeaton Date: Sat, 10 Jan 2015 02:16:16 +1100 Subject: [PATCH] Fixed #24060 -- Added OrderBy Expressions --- django/db/backends/mysql/base.py | 2 +- django/db/models/expressions.py | 77 ++++++++++++++++++++++++---- django/db/models/sql/compiler.py | 86 ++++++++++++++++++++------------ django/db/models/sql/query.py | 6 +-- docs/ref/models/expressions.txt | 24 ++++++++- docs/ref/models/querysets.txt | 35 ++++++++++--- docs/releases/1.8.txt | 3 +- tests/db_functions/tests.py | 69 +++++++++++++++++++++++++ tests/ordering/tests.py | 62 +++++++++++++++++++++++ 9 files changed, 310 insertions(+), 54 deletions(-) diff --git a/django/db/backends/mysql/base.py b/django/db/backends/mysql/base.py index 73a5acc7d1..ab72aefe09 100644 --- a/django/db/backends/mysql/base.py +++ b/django/db/backends/mysql/base.py @@ -300,7 +300,7 @@ class DatabaseOperations(BaseDatabaseOperations): columns. If no ordering would otherwise be applied, we don't want any implicit sorting going on. """ - return [(None, ("NULL", [], 'asc', False))] + return [(None, ("NULL", [], False))] def fulltext_search_sql(self, field_name): return 'MATCH (%s) AGAINST (%%s IN BOOLEAN MODE)' % field_name diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index 969b8d3acd..0f55545961 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -118,7 +118,7 @@ class CombinableMixin(object): ) -class ExpressionNode(CombinableMixin): +class BaseExpression(object): """ Base class for all query expressions. """ @@ -189,6 +189,10 @@ class ExpressionNode(CombinableMixin): """ c = self.copy() c.is_summary = summarize + c.set_source_expressions([ + expr.resolve_expression(query, allow_joins, reuse, summarize) + for expr in c.get_source_expressions() + ]) return c def _prepare(self): @@ -319,6 +323,22 @@ class ExpressionNode(CombinableMixin): """ return [e._output_field_or_none for e in self.get_source_expressions()] + def asc(self): + return OrderBy(self) + + def desc(self): + return OrderBy(self, descending=True) + + def reverse_ordering(self): + return self + + +class ExpressionNode(BaseExpression, CombinableMixin): + """ + An expression that can be combined with other expressions. + """ + pass + class Expression(ExpressionNode): @@ -412,6 +432,12 @@ class F(CombinableMixin): def refs_aggregate(self, existing_aggregates): return refs_aggregate(self.name.split(LOOKUP_SEP), existing_aggregates) + def asc(self): + return OrderBy(self) + + def desc(self): + return OrderBy(self, descending=True) + class Func(ExpressionNode): """ @@ -526,15 +552,6 @@ class Random(ExpressionNode): return connection.ops.random_function_sql(), [] -class ColIndexRef(ExpressionNode): - def __init__(self, idx): - self.idx = idx - super(ColIndexRef, self).__init__() - - def as_sql(self, compiler, connection): - return str(self.idx), [] - - class Col(ExpressionNode): def __init__(self, alias, target, source=None): if source is None: @@ -678,3 +695,43 @@ class DateTime(ExpressionNode): value = value.replace(tzinfo=None) value = timezone.make_aware(value, self.tzinfo) return value + + +class OrderBy(BaseExpression): + template = '%(expression)s %(ordering)s' + descending_template = 'DESC' + ascending_template = 'ASC' + + def __init__(self, expression, descending=False): + self.descending = descending + if not hasattr(expression, 'resolve_expression'): + raise ValueError('expression must be an expression type') + self.expression = expression + + def set_source_expressions(self, exprs): + self.expression = exprs[0] + + def get_source_expressions(self): + return [self.expression] + + def as_sql(self, compiler, connection): + expression_sql, params = compiler.compile(self.expression) + placeholders = {'expression': expression_sql} + placeholders['ordering'] = 'DESC' if self.descending else 'ASC' + return (self.template % placeholders).rstrip(), params + + def get_group_by_cols(self): + cols = [] + for source in self.get_source_expressions(): + cols.extend(source.get_group_by_cols()) + return cols + + def reverse_ordering(self): + self.descending = not self.descending + return self + + def asc(self): + self.descending = False + + def desc(self): + self.descending = True diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 7d9b3ee609..b091571835 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -1,9 +1,10 @@ from itertools import chain +import re import warnings from django.core.exceptions import FieldError from django.db.models.constants import LOOKUP_SEP -from django.db.models.expressions import RawSQL, Ref, Random, ColIndexRef +from django.db.models.expressions import OrderBy, Random, RawSQL, Ref from django.db.models.query_utils import select_related_descend, QueryWrapper from django.db.models.sql.constants import (CURSOR, SINGLE, MULTI, NO_RESULTS, ORDER_DIR, GET_ITERATOR_CHUNK_SIZE) @@ -28,6 +29,7 @@ class SQLCompiler(object): self.select = None self.annotation_col_map = None self.klass_info = None + self.ordering_parts = re.compile(r'(.*)\s(ASC|DESC)(.*)') def setup_query(self): if all(self.query.alias_refcount[a] == 0 for a in self.query.tables): @@ -105,14 +107,14 @@ class SQLCompiler(object): cols = expr.get_group_by_cols() for col in cols: expressions.append(col) - for expr, _ in order_by: + for expr, (sql, params, is_ref) in order_by: if expr.contains_aggregate: continue # We can skip References to select clause, as all expressions in # the select clause are already part of the group by. - if isinstance(expr, Ref): + if is_ref: continue - expressions.append(expr) + expressions.extend(expr.get_source_expressions()) having = self.query.having.get_group_by_cols() for expr in having: expressions.append(expr) @@ -234,54 +236,75 @@ class SQLCompiler(object): order_by = [] for pos, field in enumerate(ordering): - if field == '?': - order_by.append((Random(), asc, False)) + if hasattr(field, 'resolve_expression'): + if not isinstance(field, OrderBy): + field = field.asc() + if not self.query.standard_ordering: + field.reverse_ordering() + order_by.append((field, False)) continue - if isinstance(field, int): - if field < 0: - field = -field - int_ord = desc - order_by.append((ColIndexRef(field), int_ord, True)) + if field == '?': # random + order_by.append((OrderBy(Random()), False)) continue + col, order = get_order_dir(field, asc) + descending = True if order == 'DESC' else False + if col in self.query.annotation_select: - order_by.append((Ref(col, self.query.annotation_select[col]), order, True)) + order_by.append(( + OrderBy(Ref(col, self.query.annotation_select[col]), descending=descending), + True)) continue + if '.' in field: # This came in through an extra(order_by=...) addition. Pass it # on verbatim. table, col = col.split('.', 1) - expr = RawSQL('%s.%s' % (self.quote_name_unless_alias(table), col), []) - order_by.append((expr, order, False)) + order_by.append(( + OrderBy(RawSQL('%s.%s' % (self.quote_name_unless_alias(table), col), [])), + False)) continue - if not self.query._extra or get_order_dir(field)[0] not in self.query._extra: + + if not self.query._extra or col not in self.query._extra: # 'col' is of the form 'field' or 'field1__field2' or # '-field1__field2__field', etc. - order_by.extend(self.find_ordering_name(field, self.query.get_meta(), - default_order=asc)) + order_by.extend(self.find_ordering_name( + field, self.query.get_meta(), default_order=asc)) else: if col not in self.query.extra_select: - order_by.append((RawSQL(*self.query.extra[col]), order, False)) + order_by.append(( + OrderBy(RawSQL(*self.query.extra[col]), descending=descending), + False)) else: - order_by.append((Ref(col, RawSQL(*self.query.extra[col])), - order, True)) + order_by.append(( + OrderBy(Ref(col, RawSQL(*self.query.extra[col])), descending=descending), + True)) result = [] seen = set() - for expr, order, is_ref in order_by: - sql, params = self.compile(expr) - if (sql, tuple(params)) in seen: + + for expr, is_ref in order_by: + resolved = expr.resolve_expression( + self.query, allow_joins=True, reuse=None) + sql, params = self.compile(resolved) + # Don't add the same column twice, but the order direction is + # not taken into account so we strip it. When this entire method + # is refactored into expressions, then we can check each part as we + # generate it. + without_ordering = self.ordering_parts.search(sql).group(1) + if (without_ordering, tuple(params)) in seen: continue - seen.add((sql, tuple(params))) - result.append((expr, (sql, params, order, is_ref))) + seen.add((without_ordering, tuple(params))) + result.append((resolved, (sql, params, is_ref))) return result def get_extra_select(self, order_by, select): extra_select = [] select_sql = [t[1] for t in select] if self.query.distinct and not self.query.distinct_fields: - for expr, (sql, params, _, is_ref) in order_by: - if not is_ref and (sql, params) not in select_sql: - extra_select.append((expr, (sql, params), None)) + for expr, (sql, params, is_ref) in order_by: + without_ordering = self.ordering_parts.search(sql).group(1) + if not is_ref and (without_ordering, params) not in select_sql: + extra_select.append((expr, (without_ordering, params), None)) return extra_select def __call__(self, name): @@ -392,8 +415,8 @@ class SQLCompiler(object): if order_by: ordering = [] - for _, (o_sql, o_params, order, _) in order_by: - ordering.append('%s %s' % (o_sql, order)) + for _, (o_sql, o_params, _) in order_by: + ordering.append(o_sql) params.extend(o_params) result.append('ORDER BY %s' % ', '.join(ordering)) @@ -514,6 +537,7 @@ class SQLCompiler(object): The 'name' is of the form 'field1__field2__...__fieldN'. """ name, order = get_order_dir(name, default_order) + descending = True if order == 'DESC' else False pieces = name.split(LOOKUP_SEP) field, targets, alias, joins, path, opts = self._setup_joins(pieces, opts, alias) @@ -535,7 +559,7 @@ class SQLCompiler(object): order, already_seen)) return results targets, alias, _ = self.query.trim_joins(targets, joins, path) - return [(t.get_col(alias), order, False) for t in targets] + return [(OrderBy(t.get_col(alias), descending=descending), False) for t in targets] def _setup_joins(self, pieces, opts, alias): """ diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index a613d8eba4..5d4538b2d9 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -1691,14 +1691,14 @@ class Query(object): """ Adds items from the 'ordering' sequence to the query's "order by" clause. These items are either field names (not column names) -- - possibly with a direction prefix ('-' or '?') -- or ordinals, - corresponding to column positions in the 'select' list. + possibly with a direction prefix ('-' or '?') -- or OrderBy + expressions. If 'ordering' is empty, all ordering is cleared from the query. """ errors = [] for item in ordering: - if not ORDER_PATTERN.match(item): + if not hasattr(item, 'resolve_expression') and not ORDER_PATTERN.match(item): errors.append(item) if errors: raise FieldError('Invalid order_by arguments: %s' % errors) diff --git a/docs/ref/models/expressions.txt b/docs/ref/models/expressions.txt index 126455aba4..b36afe5633 100644 --- a/docs/ref/models/expressions.txt +++ b/docs/ref/models/expressions.txt @@ -5,7 +5,7 @@ Query Expressions .. currentmodule:: django.db.models Query expressions describe a value or a computation that can be used as part of -a filter, an annotation, or an aggregation. There are a number of built-in +a filter, order by, annotation, or aggregate. There are a number of built-in expressions (documented below) that can be used to help you write queries. Expressions can be combined, or in some cases nested, to form more complex computations. @@ -58,6 +58,10 @@ Some examples # Aggregates can contain complex computations also Company.objects.annotate(num_offerings=Count(F('products') + F('services'))) + # Expressions can also be used in order_by() + Company.objects.order_by(Length('name').asc()) + Company.objects.order_by(Length('name').desc()) + Built-in Expressions ==================== @@ -428,6 +432,24 @@ calling the appropriate methods on the wrapped expression. nested expressions. ``F()`` objects, in particular, hold a reference to a column. + .. method:: asc() + + Returns the expression ready to be sorted in ascending order. + + .. method:: desc() + + Returns the expression ready to be sorted in descending order. + + .. method:: reverse_ordering() + + Returns ``self`` with any modifications required to reverse the sort + order within an ``order_by`` call. As an example, an expression + implementing ``NULLS LAST`` would change its value to be + ``NULLS FIRST``. Modifications are only required for expressions that + implement sort order like ``OrderBy``. This method is called when + :meth:`~django.db.models.query.QuerySet.reverse()` is called on a + queryset. + Writing your own Query Expressions ---------------------------------- diff --git a/docs/ref/models/querysets.txt b/docs/ref/models/querysets.txt index 397eea0782..e7d293fc89 100644 --- a/docs/ref/models/querysets.txt +++ b/docs/ref/models/querysets.txt @@ -322,17 +322,28 @@ identical to:: Entry.objects.order_by('blog__name') +It is also possible to order a queryset by a related field, without incurring +the cost of a JOIN, by referring to the ``_id`` of the related field:: + + # No Join + Entry.objects.order_by('blog_id') + + # Join + Entry.objects.order_by('blog__id') + .. versionadded:: 1.7 - Note that it is also possible to order a queryset by a related field, - without incurring the cost of a JOIN, by referring to the ``_id`` of the - related field:: + The ability to order a queryset by a related field, without incurring + the cost of a JOIN was added. - # No Join - Entry.objects.order_by('blog_id') +You can also order by :doc:`query expressions ` by +calling ``asc()`` or ``desc()`` on the expression:: - # Join - Entry.objects.order_by('blog__id') + Entry.objects.order_by(Coalesce('summary', 'headline').desc()) + +.. versionadded:: 1.8 + + Ordering by query expressions was added. Be cautious when ordering by fields in related models if you are also using :meth:`distinct()`. See the note in :meth:`distinct` for an explanation of how @@ -367,6 +378,16 @@ There's no way to specify whether ordering should be case sensitive. With respect to case-sensitivity, Django will order results however your database backend normally orders them. +You can order by a field converted to lowercase with +:class:`~django.db.models.functions.Lower` which will achieve case-consistent +ordering:: + + Entry.objects.order_by(Lower('headline').desc()) + +.. versionadded:: 1.8 + + The ability to order by expressions like ``Lower`` was added. + If you don't want any ordering to be applied to a query, not even the default ordering, call :meth:`order_by()` with no parameters. diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index cd9a5de682..7a70a8ec2c 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -100,7 +100,8 @@ Query Expressions and Database Functions customize, and compose complex SQL expressions. This has enabled annotate to accept expressions other than aggregates. Aggregates are now able to reference multiple fields, as well as perform arithmetic, similar to ``F()`` -objects. +objects. :meth:`~django.db.models.query.QuerySet.order_by` has also gained the +ability to accept expressions. A collection of :doc:`database functions ` is also included with functionality such as diff --git a/tests/db_functions/tests.py b/tests/db_functions/tests.py index 2ab18b61e0..00a5e258de 100644 --- a/tests/db_functions/tests.py +++ b/tests/db_functions/tests.py @@ -68,6 +68,37 @@ class FunctionTests(TestCase): lambda a: a.headline ) + def test_coalesce_ordering(self): + Author.objects.create(name='John Smith', alias='smithj') + Author.objects.create(name='Rhonda') + + authors = Author.objects.order_by(Coalesce('alias', 'name')) + self.assertQuerysetEqual( + authors, [ + 'Rhonda', + 'John Smith', + ], + lambda a: a.name + ) + + authors = Author.objects.order_by(Coalesce('alias', 'name').asc()) + self.assertQuerysetEqual( + authors, [ + 'Rhonda', + 'John Smith', + ], + lambda a: a.name + ) + + authors = Author.objects.order_by(Coalesce('alias', 'name').desc()) + self.assertQuerysetEqual( + authors, [ + 'John Smith', + 'Rhonda', + ], + lambda a: a.name + ) + def test_concat(self): Author.objects.create(name='Jayden') Author.objects.create(name='John Smith', alias='smithj', goes_by='John') @@ -184,6 +215,22 @@ class FunctionTests(TestCase): self.assertEqual(authors.filter(alias_length__lte=Length('name')).count(), 1) + def test_length_ordering(self): + Author.objects.create(name='John Smith', alias='smithj') + Author.objects.create(name='John Smith', alias='smithj1') + Author.objects.create(name='Rhonda', alias='ronny') + + authors = Author.objects.order_by(Length('name'), Length('alias')) + + self.assertQuerysetEqual( + authors, [ + ('Rhonda', 'ronny'), + ('John Smith', 'smithj'), + ('John Smith', 'smithj1'), + ], + lambda a: (a.name, a.alias) + ) + def test_substr(self): Author.objects.create(name='John Smith', alias='smithj') Author.objects.create(name='Rhonda') @@ -230,3 +277,25 @@ class FunctionTests(TestCase): with six.assertRaisesRegex(self, ValueError, "'pos' must be greater than 0"): Author.objects.annotate(raises=Substr('name', 0)) + + def test_nested_function_ordering(self): + Author.objects.create(name='John Smith') + Author.objects.create(name='Rhonda Simpson', alias='ronny') + + authors = Author.objects.order_by(Length(Coalesce('alias', 'name'))) + self.assertQuerysetEqual( + authors, [ + 'Rhonda Simpson', + 'John Smith', + ], + lambda a: a.name + ) + + authors = Author.objects.order_by(Length(Coalesce('alias', 'name')).desc()) + self.assertQuerysetEqual( + authors, [ + 'John Smith', + 'Rhonda Simpson', + ], + lambda a: a.name + ) diff --git a/tests/ordering/tests.py b/tests/ordering/tests.py index 5d323e006b..abb30f43f1 100644 --- a/tests/ordering/tests.py +++ b/tests/ordering/tests.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals from datetime import datetime from operator import attrgetter +from django.db.models import F from django.test import TestCase from .models import Article, Author @@ -203,3 +204,64 @@ class OrderingTests(TestCase): ], attrgetter("headline") ) + + def test_order_by_f_expression(self): + self.assertQuerysetEqual( + Article.objects.order_by(F('headline')), [ + "Article 1", + "Article 2", + "Article 3", + "Article 4", + ], + attrgetter("headline") + ) + self.assertQuerysetEqual( + Article.objects.order_by(F('headline').asc()), [ + "Article 1", + "Article 2", + "Article 3", + "Article 4", + ], + attrgetter("headline") + ) + self.assertQuerysetEqual( + Article.objects.order_by(F('headline').desc()), [ + "Article 4", + "Article 3", + "Article 2", + "Article 1", + ], + attrgetter("headline") + ) + + def test_order_by_f_expression_duplicates(self): + """ + A column may only be included once (the first occurrence) so we check + to ensure there are no duplicates by inspecting the SQL. + """ + qs = Article.objects.order_by(F('headline').asc(), F('headline').desc()) + sql = str(qs.query).upper() + fragment = sql[sql.find('ORDER BY'):] + self.assertEqual(fragment.count('HEADLINE'), 1) + self.assertQuerysetEqual( + qs, [ + "Article 1", + "Article 2", + "Article 3", + "Article 4", + ], + attrgetter("headline") + ) + qs = Article.objects.order_by(F('headline').desc(), F('headline').asc()) + sql = str(qs.query).upper() + fragment = sql[sql.find('ORDER BY'):] + self.assertEqual(fragment.count('HEADLINE'), 1) + self.assertQuerysetEqual( + qs, [ + "Article 4", + "Article 3", + "Article 2", + "Article 1", + ], + attrgetter("headline") + )