From 5c8bc517ee2d1d9bbb343cf664a522f24d59f4d2 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 11 Oct 2023 15:36:24 -0400 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_20/10458.rst | 11 ++++ lib/sqlalchemy/dialects/mssql/base.py | 8 ++- test/dialect/mssql/test_compiler.py | 68 +++++++++++++++++++++ 3 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/10458.rst diff --git a/doc/build/changelog/unreleased_20/10458.rst b/doc/build/changelog/unreleased_20/10458.rst new file mode 100644 index 0000000000..a8c0fbe6b8 --- /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 6d46687e43..6ebad7fcbe 100644 --- 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 de7d85afc0..74867ccbe2 100644 --- 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" -- 2.47.3