include nulls_first, nulls_last in order_by_label_element

Fixed bug where the :meth:`.Operators.nulls_first()` and
:meth:`.Operators.nulls_last()` modifiers would not be treated the same way
as :meth:`.Operators.desc()` and :meth:`.Operators.asc()` when determining
if an ORDER BY should be against a label name already in the statement. All
four modifiers are now treated the same within ORDER BY.

Fixes: #11592
Change-Id: I1de1aff679c56af1abfdfd07f9bcbc45ecc5a8cc

diff --git a/doc/build/changelog/unreleased_20/11592.rst b/doc/build/changelog/unreleased_20/11592.rst
new file mode 100644 (file)
index 0000000..616eb1e
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/11592.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, sql
+    :tickets: 11592
+
+    Fixed bug where the :meth:`.Operators.nulls_first()` and
+    :meth:`.Operators.nulls_last()` modifiers would not be treated the same way
+    as :meth:`.Operators.desc()` and :meth:`.Operators.asc()` when determining
+    if an ORDER BY should be against a label name already in the statement. All
+    four modifiers are now treated the same within ORDER BY.

diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index 56b937726e089008ba4702bab6894da6709e2d79..3271acd60d9aad1656b27db72f7c3883a70ff353 100644 (file)
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -3699,7 +3699,7 @@ class UnaryExpression(ColumnElement[_T]):
 
     @property
     def _order_by_label_element(self) -> Optional[Label[Any]]:
-        if self.modifier in (operators.desc_op, operators.asc_op):
+        if operators.is_order_by_modifier(self.modifier):
             return self.element._order_by_label_element
         else:
             return None

diff --git a/lib/sqlalchemy/sql/operators.py b/lib/sqlalchemy/sql/operators.py
index a5390ad6d0fa20353889e50fa90c3ea76cc22b22..65b94f32b54dcfe46d2b44314bac2b564450458d 100644 (file)
--- a/lib/sqlalchemy/sql/operators.py
+++ b/lib/sqlalchemy/sql/operators.py
@@ -2474,6 +2474,12 @@ def is_associative(op: OperatorType) -> bool:
     return op in _associative
 
 
+def is_order_by_modifier(op: Optional[OperatorType]) -> bool:
+    return op in _order_by_modifier
+
+
+_order_by_modifier = {desc_op, asc_op, nulls_first_op, nulls_last_op}
+
 _natural_self_precedent = _associative.union(
     [getitem, json_getitem_op, json_path_getitem_op]
 )

diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py
index 9d9f69bdb9b04a4cb6f55da69db45c9db16d0ec6..3e8fca59a887a9bd56cadd94edb38faa058f584d 100644 (file)
--- a/test/sql/test_compiler.py
+++ b/test/sql/test_compiler.py
@@ -44,6 +44,10 @@ from sqlalchemy import literal_column
 from sqlalchemy import MetaData
 from sqlalchemy import not_
 from sqlalchemy import null
+from sqlalchemy import nulls_first
+from sqlalchemy import nulls_last
+from sqlalchemy import nullsfirst
+from sqlalchemy import nullslast
 from sqlalchemy import Numeric
 from sqlalchemy import or_
 from sqlalchemy import outerjoin
@@ -1668,44 +1672,85 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL):
             "foo || :param_1",
         )
 
-    def test_order_by_labels_enabled(self):
+    def test_order_by_labels_enabled_negative_cases(self):
+        """test order_by_labels enabled but the cases where we expect
+        ORDER BY the expression without the label name"""
+
         lab1 = (table1.c.myid + 12).label("foo")
         lab2 = func.somefunc(table1.c.name).label("bar")
         dialect = default.DefaultDialect()
 
+        # binary expressions render as the expression without labels
         self.assert_compile(
-            select(lab1, lab2).order_by(lab1, desc(lab2)),
+            select(lab1, lab2).order_by(lab1 + "test"),
             "SELECT mytable.myid + :myid_1 AS foo, "
             "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY foo, bar DESC",
+            "ORDER BY mytable.myid + :myid_1 + :param_1",
             dialect=dialect,
         )
 
-        # the function embedded label renders as the function
+        # labels within functions in the columns clause render
+        # with the expression
         self.assert_compile(
-            select(lab1, lab2).order_by(func.hoho(lab1), desc(lab2)),
+            select(lab1, func.foo(lab1)).order_by(lab1, func.foo(lab1)),
             "SELECT mytable.myid + :myid_1 AS foo, "
-            "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY hoho(mytable.myid + :myid_1), bar DESC",
+            "foo(mytable.myid + :myid_1) AS foo_1 FROM mytable "
+            "ORDER BY foo, foo(mytable.myid + :myid_1)",
             dialect=dialect,
         )
 
-        # binary expressions render as the expression without labels
+        # here, 'name' is implicitly available, but w/ #3882 we don't
+        # want to render a name that isn't specifically a Label elsewhere
+        # in the query
         self.assert_compile(
-            select(lab1, lab2).order_by(lab1 + "test"),
+            select(table1.c.myid).order_by(table1.c.name.label("name")),
+            "SELECT mytable.myid FROM mytable ORDER BY mytable.name",
+        )
+
+        # as well as if it doesn't match
+        self.assert_compile(
+            select(table1.c.myid).order_by(
+                func.lower(table1.c.name).label("name")
+            ),
+            "SELECT mytable.myid FROM mytable ORDER BY lower(mytable.name)",
+        )
+
+    @testing.combinations(
+        (desc, "DESC"),
+        (asc, "ASC"),
+        (nulls_first, "NULLS FIRST"),
+        (nulls_last, "NULLS LAST"),
+        (nullsfirst, "NULLS FIRST"),
+        (nullslast, "NULLS LAST"),
+        (lambda c: c.desc().nulls_last(), "DESC NULLS LAST"),
+        (lambda c: c.desc().nullslast(), "DESC NULLS LAST"),
+        (lambda c: c.nulls_first().asc(), "NULLS FIRST ASC"),
+    )
+    def test_order_by_labels_enabled(self, operator, expected):
+        """test positive cases with order_by_labels enabled.  this is
+        multipled out to all the ORDER BY modifier operators
+        (see #11592)
+
+
+        """
+        lab1 = (table1.c.myid + 12).label("foo")
+        lab2 = func.somefunc(table1.c.name).label("bar")
+        dialect = default.DefaultDialect()
+
+        self.assert_compile(
+            select(lab1, lab2).order_by(lab1, operator(lab2)),
             "SELECT mytable.myid + :myid_1 AS foo, "
             "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY mytable.myid + :myid_1 + :param_1",
+            f"ORDER BY foo, bar {expected}",
             dialect=dialect,
         )
 
-        # labels within functions in the columns clause render
-        # with the expression
+        # the function embedded label renders as the function
         self.assert_compile(
-            select(lab1, func.foo(lab1)).order_by(lab1, func.foo(lab1)),
+            select(lab1, lab2).order_by(func.hoho(lab1), operator(lab2)),
             "SELECT mytable.myid + :myid_1 AS foo, "
-            "foo(mytable.myid + :myid_1) AS foo_1 FROM mytable "
-            "ORDER BY foo, foo(mytable.myid + :myid_1)",
+            "somefunc(mytable.name) AS bar FROM mytable "
+            f"ORDER BY hoho(mytable.myid + :myid_1), bar {expected}",
             dialect=dialect,
         )
 
@@ -1713,62 +1758,49 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL):
         ly = (func.lower(table1.c.name) + table1.c.description).label("ly")
 
         self.assert_compile(
-            select(lx, ly).order_by(lx, ly.desc()),
+            select(lx, ly).order_by(lx, operator(ly)),
             "SELECT mytable.myid + mytable.myid AS lx, "
             "lower(mytable.name) || mytable.description AS ly "
-            "FROM mytable ORDER BY lx, ly DESC",
+            f"FROM mytable ORDER BY lx, ly {expected}",
             dialect=dialect,
         )
 
         # expression isn't actually the same thing (even though label is)
         self.assert_compile(
             select(lab1, lab2).order_by(
-                table1.c.myid.label("foo"), desc(table1.c.name.label("bar"))
+                table1.c.myid.label("foo"),
+                operator(table1.c.name.label("bar")),
             ),
             "SELECT mytable.myid + :myid_1 AS foo, "
             "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY mytable.myid, mytable.name DESC",
+            f"ORDER BY mytable.myid, mytable.name {expected}",
             dialect=dialect,
         )
 
         # it's also an exact match, not aliased etc.
         self.assert_compile(
             select(lab1, lab2).order_by(
-                desc(table1.alias().c.name.label("bar"))
+                operator(table1.alias().c.name.label("bar"))
             ),
             "SELECT mytable.myid + :myid_1 AS foo, "
             "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY mytable_1.name DESC",
+            f"ORDER BY mytable_1.name {expected}",
             dialect=dialect,
         )
 
         # but! it's based on lineage
         lab2_lineage = lab2.element._clone()
         self.assert_compile(
-            select(lab1, lab2).order_by(desc(lab2_lineage.label("bar"))),
+            select(lab1, lab2).order_by(operator(lab2_lineage.label("bar"))),
             "SELECT mytable.myid + :myid_1 AS foo, "
             "somefunc(mytable.name) AS bar FROM mytable "
-            "ORDER BY bar DESC",
+            f"ORDER BY bar {expected}",
             dialect=dialect,
         )
 
-        # here, 'name' is implicitly available, but w/ #3882 we don't
-        # want to render a name that isn't specifically a Label elsewhere
-        # in the query
-        self.assert_compile(
-            select(table1.c.myid).order_by(table1.c.name.label("name")),
-            "SELECT mytable.myid FROM mytable ORDER BY mytable.name",
-        )
-
-        # as well as if it doesn't match
-        self.assert_compile(
-            select(table1.c.myid).order_by(
-                func.lower(table1.c.name).label("name")
-            ),
-            "SELECT mytable.myid FROM mytable ORDER BY lower(mytable.name)",
-        )
-
     def test_order_by_labels_disabled(self):
+        """test when the order_by_labels feature is disabled entirely"""
+
         lab1 = (table1.c.myid + 12).label("foo")
         lab2 = func.somefunc(table1.c.name).label("bar")
         dialect = default.DefaultDialect()