Repository: tobymao/sqlglot
Pull request: Feat(tsql): transpile LIMIT with OFFSET properly
Fixes #3144

Reference:
- https://www.microsoftpressstore.com/articles/article.aspx?p=2314819
- https://learn.microsoft.com/en-us/sql/t-sql/queries/select-order-by-clause-transact-sql?view=sql-server-ver16

Problem statement:
Concerns about Translating SQLite Queries to TSQL
1. **Offset Issue:**
   
   When attempting to convert an SQLite query to TSQL using the provided code snippet:

   ```python
   import sqlglot

   sql_a = "SELECT name, (SELECT COUNT(*) FROM orders WHERE orders.customer_id = customers.id) AS order_count FROM customers LIMIT 5, 10"

   sql_b = sqlglot.transpile(sql_a, read="sqlite", write="tsql")[0]

   print(sql_b)
   ```

   The resulting TSQL output is as follows:

   ```sql
   SELECT TOP 10 name, (SELECT COUNT(*) FROM orders WHERE orders.customer_id = customers.id) AS order_count FROM customers
   ```

   It appears that the translated TSQL does not include information about the offset of 5. Instead, it only selects the top 10 rows, neglecting the fact that it should select 10 rows after an offset of 5. Could you please verify if this behavior is intended?

2. **Completeness of Translation:**

   I had a question about the thoroughness of the translation process from SQLite to TSQL. Does the transpiler cover all possible cases of SQLite queries that are incompatible with TSQL syntax? Are there any known clauses or scenarios where the transpiler fails to convert the syntax accurately?

Hints from issue discussion:
Hey, thanks for the report. I'll take a look, this seems like a bug.

> I had a question about the thoroughness of the translation process from SQLite to TSQL. Does the transpiler cover all possible cases of SQLite queries that are incompatible with TSQL syntax? Are there any known clauses or scenarios where the transpiler fails to convert the syntax accurately?

The transpiler probably does not cover all possible cases as both dialects are huge and there may even be features that are not transpilable at all. It does cover a very large surface though, so I expect that for more "standard" queries you won't have any issues. There's no documentation about missing functionality, but feel free to file issues and / or PRs if you come across bugs in the future so we can improve.

Commit message: Feat(tsql): transpile LIMIT with OFFSET properly

Patch to review:
diff --git a/sqlglot/dialects/tsql.py b/sqlglot/dialects/tsql.py
index 45855772cb..c4e946012d 100644
--- a/sqlglot/dialects/tsql.py
+++ b/sqlglot/dialects/tsql.py
@@ -328,6 +328,21 @@ def _json_extract_sql(
     return self.func("ISNULL", json_query, json_value)
 
 
+def _replace_limit_with_offset_fetch(expression: exp.Expression) -> exp.Expression:
+    limit = expression.args.get("limit")
+    if (
+        isinstance(expression, exp.Select)
+        and isinstance(limit, exp.Limit)
+        and expression.args.get("offset")
+    ):
+        limit.replace(exp.Fetch(direction="NEXT", count=limit.expression))
+
+        if not expression.args.get("order"):
+            expression.order_by(exp.select(exp.null()).subquery(), copy=False)
+
+    return expression
+
+
 class TSQL(Dialect):
     NORMALIZATION_STRATEGY = NormalizationStrategy.CASE_INSENSITIVE
     TIME_FORMAT = "'yyyy-mm-dd hh:mm:ss'"
@@ -785,6 +800,7 @@ class Generator(generator.Generator):
                     transforms.eliminate_distinct_on,
                     transforms.eliminate_semi_and_anti_joins,
                     transforms.eliminate_qualify,
+                    _replace_limit_with_offset_fetch,
                 ]
             ),
             exp.StrPosition: lambda self, e: self.func(
diff --git a/tests/dialects/test_tsql.py b/tests/dialects/test_tsql.py
index ed474fd1af..e8ea41e34c 100644
--- a/tests/dialects/test_tsql.py
+++ b/tests/dialects/test_tsql.py
@@ -272,6 +272,18 @@ def test_tsql(self):
             "SELECT [x].[y] FROM foo",
         )
 
+        self.validate_all(
+            "SELECT * FROM t ORDER BY (SELECT NULL) OFFSET 5 ROWS FETCH NEXT 10 ROWS ONLY",
+            read={
+                "duckdb": "SELECT * FROM t LIMIT 10 OFFSET 5",
+                "sqlite": "SELECT * FROM t LIMIT 5, 10",
+                "tsql": "SELECT * FROM t ORDER BY (SELECT NULL) OFFSET 5 ROWS FETCH NEXT 10 ROWS ONLY",
+            },
+            write={
+                "duckdb": "SELECT * FROM t ORDER BY (SELECT NULL) NULLS FIRST LIMIT 10 OFFSET 5",
+                "sqlite": "SELECT * FROM t ORDER BY (SELECT NULL) LIMIT 10 OFFSET 5",
+            },
+        )
         self.validate_all(
             "SELECT CAST([a].[b] AS SMALLINT) FROM foo",
             write={


Code review comments:
diff_hunk: @@ -328,6 +328,21 @@ def _json_extract_sql(
     return self.func("ISNULL", json_query, json_value)
 
 
+def _replace_limit_with_offset_fetch(expression: exp.Expression) -> exp.Expression:
path: sqlglot/dialects/tsql.py
text: @user1:
doesn't this happen in the generator? why do we need this?

@author:
I'll try to refactor the generator to simplify the logic there. The issue is that the generation of `TOP` happens too [early](https://github.com/tobymao/sqlglot/blob/d6bac3e54c6445c52daa04015b1b2e4a6933e682/sqlglot/generator.py#L2194) if the `limit` arg is a `Limit`, and so we will end up with both a `FETCH` and a `TOP` in the same query, which is problematic. Also, the `OFFSET` is ignored for some reason currently in the generator if there's a `Limit`, which is a separate issue.

@author:
For context, `TOP` can't be used with `OFFSET`, so we need to produce the `FETCH` clause if we need to offset the rowset.