include ORDER BY in subquery that has TOP via FETCH

Fixed bug where the rule that prevents ORDER BY from emitting within
subqueries on SQL Server was not being disabled in the case where the
:meth:`.select.fetch` method were used to limit rows in conjunction with
WITH TIES or PERCENT, preventing valid subqueries with TOP / ORDER BY from
being used.

Fixes: #10458
Change-Id: I5bfab485241ee54de50cc03e222d9d1736089ea8

diff --git a/doc/build/changelog/unreleased_20/10458.rst b/doc/build/changelog/unreleased_20/10458.rst
new file mode 100644 (file)
index 0000000..a8c0fbe
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/10458.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, mssql
+    :tickets: 10458
+
+    Fixed bug where the rule that prevents ORDER BY from emitting within
+    subqueries on SQL Server was not being disabled in the case where the
+    :meth:`.select.fetch` method were used to limit rows in conjunction with
+    WITH TIES or PERCENT, preventing valid subqueries with TOP / ORDER BY from
+    being used.
+
+

diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py
index 6d46687e43b797bc012673480b9755fb20f0d3a3..6ebad7fcbe02e5d70009ee592209dceb2f4b8885 100644 (file)
--- a/lib/sqlalchemy/dialects/mssql/base.py
+++ b/lib/sqlalchemy/dialects/mssql/base.py
@@ -2125,6 +2125,7 @@ class MSSQLCompiler(compiler.SQLCompiler):
             or (
                 # limit can use TOP with is by itself. fetch only uses TOP
                 # when it needs to because of PERCENT and/or WITH TIES
+                # TODO: Why?  shouldn't we use TOP always ?
                 select._simple_int_clause(select._fetch_clause)
                 and (
                     select._fetch_clause_options["percent"]
@@ -2385,10 +2386,13 @@ class MSSQLCompiler(compiler.SQLCompiler):
         return ""
 
     def order_by_clause(self, select, **kw):
-        # MSSQL only allows ORDER BY in subqueries if there is a LIMIT
+        # MSSQL only allows ORDER BY in subqueries if there is a LIMIT:
+        # "The ORDER BY clause is invalid in views, inline functions,
+        # derived tables, subqueries, and common table expressions,
+        # unless TOP, OFFSET or FOR XML is also specified."
         if (
             self.is_subquery()
-            and not select._limit
+            and not self._use_top(select)
             and (
                 select._offset is None
                 or not self.dialect._supports_offset_fetch

diff --git a/test/dialect/mssql/test_compiler.py b/test/dialect/mssql/test_compiler.py
index de7d85afc0459c2dce0ff039baaffc300992e50f..74867ccbe21ab80cb645f6f879b290ccdcbb7d4e 100644 (file)
--- a/test/dialect/mssql/test_compiler.py
+++ b/test/dialect/mssql/test_compiler.py
@@ -485,6 +485,74 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL):
             "foo.myid = mytable.myid",
         )
 
+    @testing.variation("style", ["plain", "ties", "percent"])
+    def test_noorderby_insubquery_fetch(self, style):
+        """test "no ORDER BY in subqueries unless TOP / LIMIT / OFFSET"
+        present; test issue #10458"""
+
+        table1 = table(
+            "mytable",
+            column("myid", Integer),
+            column("name", String),
+            column("description", String),
+        )
+
+        if style.plain:
+            q = (
+                select(table1.c.myid)
+                .order_by(table1.c.myid)
+                .fetch(count=10)
+                .alias("foo")
+            )
+        elif style.ties:
+            q = (
+                select(table1.c.myid)
+                .order_by(table1.c.myid)
+                .fetch(count=10, with_ties=True)
+                .alias("foo")
+            )
+        elif style.percent:
+            q = (
+                select(table1.c.myid)
+                .order_by(table1.c.myid)
+                .fetch(count=10, percent=True)
+                .alias("foo")
+            )
+        else:
+            style.fail()
+
+        crit = q.c.myid == table1.c.myid
+
+        if style.plain:
+            # the "plain" style of fetch doesnt use TOP right now, so
+            # there's an order_by implicit in the row_number part of it
+            self.assert_compile(
+                select("*").where(crit),
+                "SELECT * FROM (SELECT anon_1.myid AS myid FROM "
+                "(SELECT mytable.myid AS myid, ROW_NUMBER() OVER "
+                "(ORDER BY mytable.myid) AS mssql_rn FROM mytable) AS anon_1 "
+                "WHERE mssql_rn <= :param_1) AS foo, mytable "
+                "WHERE foo.myid = mytable.myid",
+            )
+        elif style.ties:
+            # issue #10458 is that when TIES/PERCENT were used, and it just
+            # generates TOP, ORDER BY would be omitted.
+            self.assert_compile(
+                select("*").where(crit),
+                "SELECT * FROM (SELECT TOP __[POSTCOMPILE_param_1] WITH "
+                "TIES mytable.myid AS myid FROM mytable "
+                "ORDER BY mytable.myid) AS foo, mytable "
+                "WHERE foo.myid = mytable.myid",
+            )
+        elif style.percent:
+            self.assert_compile(
+                select("*").where(crit),
+                "SELECT * FROM (SELECT TOP __[POSTCOMPILE_param_1] "
+                "PERCENT mytable.myid AS myid FROM mytable "
+                "ORDER BY mytable.myid) AS foo, mytable "
+                "WHERE foo.myid = mytable.myid",
+            )
+
     @testing.combinations(10, 0)
     def test_noorderby_insubquery_offset_oldstyle(self, offset):
         """test "no ORDER BY in subqueries unless TOP / LIMIT / OFFSET"