Skip to content

fix(sqlserver): add TOP/ROW_NUMBER pagination for SQL Server < 2012 (#1693) - #1895

Closed
HandSonic wants to merge 1 commit into
OtterMind:mainfrom
HandSonic:fix/1693-mssql-pagination
Closed

fix(sqlserver): add TOP/ROW_NUMBER pagination for SQL Server < 2012 (#1693)#1895
HandSonic wants to merge 1 commit into
OtterMind:mainfrom
HandSonic:fix/1693-mssql-pagination

Conversation

@HandSonic

Copy link
Copy Markdown
Contributor

Problem

SqlServerSqlBuilder.buildPageLimit() 对版本号 < 11 的 SQL Server(即 < 2012)返回空字符串。DefaultSQLExecutor 检查到空字符串后完全跳过分页,返回全部数据行。

Fix

添加分页降级策略:

  • SQL Server 2012+ (v11+): 使用 OFFSET/FETCH NEXT(原有行为不变)
  • SQL Server 2005-2008 R2 (v9-10): 首页用 SELECT TOP (pageSize) *,后续页用 ROW_NUMBER() OVER() 子查询实现偏移
  • v9 以下: 返回空(无安全的分页方法)

Related

Fixes #1693

…tterMind#1693)

buildPageLimit() returned empty string for SQL Server versions below
11 (< 2012), causing DefaultSQLExecutor to skip pagination entirely
and return all rows.

Add fallback pagination:
- SQL Server 2012+ (v11+): OFFSET/FETCH NEXT (existing behavior)
- SQL Server 2005-2008 R2 (v9-10): TOP for first page, ROW_NUMBER()
  OVER() for subsequent pages
- Below v9: returns empty (no safe pagination method available)
@HandSonic
HandSonic force-pushed the fix/1693-mssql-pagination branch from dc357ce to af91751 Compare July 21, 2026 08:16
@openai0229

Copy link
Copy Markdown
Contributor

This change needs focused tests before it can be merged, and the current fallback has correctness issues that those tests should expose.

buildPageLimit is used by DefaultSQLExecutor for general SELECT statements, but the SQL Server 2005-2008 wrapper is not semantics-preserving:

  1. Later pages return _row_num as an additional result column because the outer query uses SELECT *. That changes the result-set schema and can also collide with a user column named _row_num.
  2. A source query containing ORDER BY is placed inside a derived table, where SQL Server rejects ORDER BY unless it is paired with a permitted construct such as TOP/OFFSET/FOR XML. The original ordering is also not transferred into ROW_NUMBER().
  3. ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) is nondeterministic, so repeated page requests can skip or duplicate rows. Offset pagination needs a stable ordering contract.
  4. CTE queries and projections with duplicate column names are additional common cases that cannot safely be wrapped as this implementation does.

The linked issue #1693 is a historical report about AI generating TOP/LIMIT, not a current reproduction of buildPageLimit returning all rows for SQL Server < 2012, and it has already been closed. Please link a current Issue that defines the affected SQL Server versions and the expected behavior.

There is already a SqlServerSqlBuilderTest. Please add at least version-boundary coverage for major versions 8/9/10/11, first and later pages, exact page boundaries, explicit ORDER BY, no ordering, CTEs, duplicate projection names, and preservation of the original result columns. The v11+ SQL should remain byte-for-byte compatible. The old-version SQL also needs execution evidence against the intended SQL Server version; string assertions alone will not prove that the derived-table forms are valid.

If robust rewriting is only possible for simple table-browse queries, please scope the fallback to that path rather than applying it to arbitrary editor SQL.

@HandSonic

Copy link
Copy Markdown
Contributor Author

Closing this PR per review feedback. The review identified fundamental correctness issues:

  1. column leaks into user-visible results
  2. ORDER BY in derived table is not valid in SQL Server
  3. ROW_NUMBER() without ORDER BY is nondeterministic
  4. CTE usage has issues

The reviewer correctly noted that #1693 is a historical issue, and the fallback approach of wrapping any arbitrary SQL with TOP/ROW_NUMBER pagination has too many edge cases. A proper fix would need to scope the fallback to simple table-browse queries only, which requires a more thorough design.

Thank you for the detailed review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Persistent MSSQL Query Error - TOP/LIMIT

2 participants