don't base compilation off the int value of offset/limit part II

Fixed an additional regression in the same area as that of :ticket:`6184`,
where using a value of 0 for OFFSET in conjunction with LIMIT with SQL
Server would create a statement using "TOP", as was the behavior in 1.3,
however due to caching would then fail to respond accordingly to other
values of OFFSET. If the "0" wasn't first, then it would be fine. For the
fix, the "TOP" syntax is now only emitted if the OFFSET value is omitted
entirely, that is, :meth:`_sql.Select.offset` is not used. Note that this
change now requires that if the "with_ties" or "percent" modifiers are
used, the statement can't specify an OFFSET of zero, it now needs to be
omitted entirely.

Fixes: #6265
Change-Id: If30596b8dcd9f2ce4221cd87c5407fa81f5f9a90

diff --git a/doc/build/changelog/unreleased_14/6265.rst b/doc/build/changelog/unreleased_14/6265.rst
new file mode 100644 (file)
index 0000000..113a4cc
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/6265.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, mssql, regression
+    :tickets: 6265
+
+    Fixed an additional regression in the same area as that of :ticket:`6173`,
+    :ticket:`6184`, where using a value of 0 for OFFSET in conjunction with
+    LIMIT with SQL Server would create a statement using "TOP", as was the
+    behavior in 1.3, however due to caching would then fail to respond
+    accordingly to other values of OFFSET. If the "0" wasn't first, then it
+    would be fine. For the fix, the "TOP" syntax is now only emitted if the
+    OFFSET value is omitted entirely, that is, :meth:`_sql.Select.offset` is
+    not used. Note that this change now requires that if the "with_ties" or
+    "percent" modifiers are used, the statement can't specify an OFFSET of
+    zero, it now needs to be omitted entirely.

diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py
index e0932248702da8abe75f71a2716d8c43d61fb3b3..e26548750474c1878769f796f6dbf6a05cfaa5fb 100644 (file)
--- a/lib/sqlalchemy/dialects/mssql/base.py
+++ b/lib/sqlalchemy/dialects/mssql/base.py
@@ -1759,13 +1759,7 @@ class MSSQLCompiler(compiler.SQLCompiler):
             return select._fetch_clause
 
     def _use_top(self, select):
-        return (
-            select._offset_clause is None
-            or (
-                select._simple_int_clause(select._offset_clause)
-                and select._offset == 0
-            )
-        ) and (
+        return (select._offset_clause is None) and (
             select._simple_int_clause(select._limit_clause)
             or (
                 # limit can use TOP with is by itself. fetch only uses TOP

diff --git a/lib/sqlalchemy/testing/suite/test_select.py b/lib/sqlalchemy/testing/suite/test_select.py
index 7318a4f33a994498b19b31afb06254246a5210e2..7b35dc3fa360a9b82df7415af8a0e05f7a8db7c9 100644 (file)
--- a/lib/sqlalchemy/testing/suite/test_select.py
+++ b/lib/sqlalchemy/testing/suite/test_select.py
@@ -265,19 +265,26 @@ class FetchLimitOffsetTest(fixtures.TablesTest):
             [(4, 4, 5), (5, 4, 6)],
         )
 
+    @testing.combinations(
+        ([(2, 0), (2, 1), (3, 2)]),
+        ([(2, 1), (2, 0), (3, 2)]),
+        ([(3, 1), (2, 1), (3, 1)]),
+        argnames="cases",
+    )
     @testing.requires.offset
-    def test_simple_limit_offset(self, connection):
+    def test_simple_limit_offset(self, connection, cases):
         table = self.tables.some_table
-        self._assert_result(
-            connection,
-            select(table).order_by(table.c.id).limit(2).offset(1),
-            [(2, 2, 3), (3, 3, 4)],
-        )
-        self._assert_result(
-            connection,
-            select(table).order_by(table.c.id).limit(3).offset(2),
-            [(3, 3, 4), (4, 4, 5), (5, 4, 6)],
-        )
+        connection = connection.execution_options(compiled_cache={})
+
+        assert_data = [(1, 1, 2), (2, 2, 3), (3, 3, 4), (4, 4, 5), (5, 4, 6)]
+
+        for limit, offset in cases:
+            expected = assert_data[offset : offset + limit]
+            self._assert_result(
+                connection,
+                select(table).order_by(table.c.id).limit(limit).offset(offset),
+                expected,
+            )
 
     @testing.requires.fetch_first
     def test_simple_fetch_offset(self, connection):

diff --git a/test/dialect/mssql/test_compiler.py b/test/dialect/mssql/test_compiler.py
index 582205ea106720ab8b39cc5a577cc1462fac523e..a0127fa57b998458cde01ecc6ff6e938e1e4c7da 100644 (file)
--- a/test/dialect/mssql/test_compiler.py
+++ b/test/dialect/mssql/test_compiler.py
@@ -1118,6 +1118,21 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL):
 
         s = select(t).where(t.c.x == 5).order_by(t.c.y).limit(0).offset(0)
 
+        # offset is zero but we need to cache a compatible statement
+        self.assert_compile(
+            s,
+            "SELECT anon_1.x, anon_1.y FROM (SELECT t.x AS x, t.y AS y, "
+            "ROW_NUMBER() OVER (ORDER BY t.y) AS mssql_rn FROM t "
+            "WHERE t.x = :x_1) AS anon_1 WHERE mssql_rn > :param_1 "
+            "AND mssql_rn <= :param_2 + :param_1",
+            checkparams={"x_1": 5, "param_1": 0, "param_2": 0},
+        )
+
+    def test_limit_zero_using_window(self):
+        t = table("t", column("x", Integer), column("y", Integer))
+
+        s = select(t).where(t.c.x == 5).order_by(t.c.y).limit(0)
+
         # render the LIMIT of zero, but not the OFFSET
         # of zero, so produces TOP 0
         self.assert_compile(
@@ -1420,8 +1435,12 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL):
         else:
             sel = "SELECT t.a FROM t ORDER BY t.a " + exp
 
+        stmt = select(t).order_by(t.c.a).fetch(fetch, **fetch_kw)
+        if "with_ties" not in fetch_kw and "percent" not in fetch_kw:
+            stmt = stmt.offset(offset)
+
         self.assert_compile(
-            select(t).order_by(t.c.a).fetch(fetch, **fetch_kw).offset(offset),
+            stmt,
             sel,
             checkparams=params,
             dialect=dialect_2012,
@@ -1512,8 +1531,12 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL):
                 + exp
             )
 
+        stmt = select(t).order_by(t.c.a).fetch(fetch, **fetch_kw)
+        if "with_ties" not in fetch_kw and "percent" not in fetch_kw:
+            stmt = stmt.offset(offset)
+
         self.assert_compile(
-            select(t).order_by(t.c.a).fetch(fetch, **fetch_kw).offset(offset),
+            stmt,
             sel,
             checkparams=params,
         )