Further fixes for ticket 5470

The fix for #5470 didn't actually take into account that
the "distinct" logic in query was also doubling up the criteria.
Added many more tests.   the 1.3 version here will be different
than 1.4 as the regression is not quite the same.

Fixes: #5470
Change-Id: I16a23917cab175761de9c867d9d9ac55031d9b97

diff --git a/doc/build/changelog/unreleased_13/5470.rst b/doc/build/changelog/unreleased_13/5470.rst
index ed7202df542027125c8422660de647f80c40d2f3..8bf868f3a7ecb0ed7fe74eb432903397302ed178 100644 (file)
--- a/doc/build/changelog/unreleased_13/5470.rst
+++ b/doc/build/changelog/unreleased_13/5470.rst
@@ -5,5 +5,8 @@
     Repaired an issue where the "ORDER BY" clause rendering a label name rather
     than a complete expression, which is particularly important for SQL Server,
     would fail to occur if the expression were enclosed in a parenthesized
-    grouping in some cases.   This case has been added to test support.
+    grouping in some cases.   This case has been added to test support. The
+    change additionally adjusts the "automatically add ORDER BY columns when
+    DISTINCT is present" behavior of ORM query, deprecated in 1.4, to more
+    accurately detect column expressions that are already present.
 

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index a35b2f9fdf7f79b942f5f355568a08404f66b587..0868fb29b4f33ea716c4592fd83270d820a64ced 100644 (file)
--- a/lib/sqlalchemy/orm/context.py
+++ b/lib/sqlalchemy/orm/context.py
@@ -923,7 +923,11 @@ class ORMSelectCompileState(ORMCompileState, SelectState):
 
     def _simple_statement(self):
 
-        if (self.distinct and not self.distinct_on) and self.order_by:
+        if (
+            self.compile_options._use_legacy_query_style
+            and (self.distinct and not self.distinct_on)
+            and self.order_by
+        ):
             to_add = sql_util.expand_column_list_from_order_by(
                 self.primary_columns, self.order_by
             )

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 814253266f112b8f27748ad86a6298ccf8fc0019..b3ead718acca5dfaa149abdab40d2a2b0970d917 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -27,6 +27,7 @@ from .elements import _textual_label_reference
 from .elements import BindParameter
 from .elements import ColumnClause
 from .elements import ColumnElement
+from .elements import Grouping
 from .elements import Label
 from .elements import Null
 from .elements import UnaryExpression
@@ -298,6 +299,9 @@ def unwrap_order_by(clause):
             ):
                 t = t.element
 
+                if isinstance(t, Grouping):
+                    t = t.element
+
                 stack.append(t)
                 continue
             elif isinstance(t, _label_reference):
@@ -310,6 +314,7 @@ def unwrap_order_by(clause):
             if t not in cols:
                 cols.add(t)
                 result.append(t)
+
         else:
             for c in t.get_children():
                 stack.append(c)
@@ -338,11 +343,9 @@ def expand_column_list_from_order_by(collist, order_by):
         ]
     )
 
-    return [
-        col
-        for col in chain(*[unwrap_order_by(o) for o in order_by])
-        if col not in cols_already_present
-    ]
+    to_look_for = list(chain(*[unwrap_order_by(o) for o in order_by]))
+
+    return [col for col in to_look_for if col not in cols_already_present]
 
 
 def clause_is_present(clause, search):

diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index 4663789c4d75b648a2295546554b5a6fc4232b89..99389439ea099de5ef20e47066616927076867ff 100644 (file)
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -3,6 +3,7 @@ import functools
 
 import sqlalchemy as sa
 from sqlalchemy import and_
+from sqlalchemy import asc
 from sqlalchemy import between
 from sqlalchemy import bindparam
 from sqlalchemy import Boolean
@@ -3827,6 +3828,100 @@ class DistinctTest(QueryTest, AssertsCompiledSQL):
             .all(),
         )
 
+    def test_no_automatic_distinct_thing_w_future(self):
+        User = self.classes.User
+
+        stmt = select(User.id).order_by(User.name).distinct()
+
+        self.assert_compile(
+            stmt, "SELECT DISTINCT users.id FROM users ORDER BY users.name"
+        )
+
+    def test_issue_5470_one(self):
+        User = self.classes.User
+
+        expr = (User.id.op("+")(2)).label("label")
+
+        sess = create_session()
+
+        q = sess.query(expr).select_from(User).order_by(desc(expr)).distinct()
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :id_1 AS label "
+            "FROM users ORDER BY label DESC",
+        )
+
+    def test_issue_5470_two(self):
+        User = self.classes.User
+
+        expr = User.id + literal(1)
+
+        sess = create_session()
+        q = sess.query(expr).select_from(User).order_by(asc(expr)).distinct()
+
+        # no double col in the select list,
+        # there's no label so this is the requested SQL
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS anon_1 "
+            "FROM users ORDER BY users.id + :param_1 ASC",
+        )
+
+    def test_issue_5470_three(self):
+        User = self.classes.User
+
+        expr = (User.id + literal(1)).label("label")
+
+        sess = create_session()
+        q = sess.query(expr).select_from(User).order_by(asc(expr)).distinct()
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS label "
+            "FROM users ORDER BY label ASC",
+        )
+
+    def test_issue_5470_four(self):
+        User = self.classes.User
+
+        expr = (User.id + literal(1)).label("label")
+
+        sess = create_session()
+        q = (
+            sess.query(expr)
+            .select_from(User)
+            .order_by(asc("label"))
+            .distinct()
+        )
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS label "
+            "FROM users ORDER BY label ASC",
+        )
+
+    def test_issue_5470_five(self):
+        User = self.classes.User
+
+        expr = (User.id.op("+")(2)).label("label")
+
+        stmt = select(expr).select_from(User).order_by(desc(expr)).distinct()
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            stmt,
+            "SELECT DISTINCT users.id + :id_1 AS label "
+            "FROM users ORDER BY label DESC",
+        )
+
     def test_columns_augmented_roundtrip_one_from_self(self):
         """Test workaround for legacy style DISTINCT on extra column.