fix(mssql): preserve outer CTEs and ORDER BY when flattening pagination wrap - #2579
Conversation
…on wrap _flatten_pagination_limit collapses SELECT * FROM (inner) LIMIT n into the inner select, but returned only the inner node — silently dropping an outer WITH (invalid object errors for CTE references) and an outer ORDER BY (TOP-n then returns arbitrary rows). Bail out of flattening when the outer select carries either clause; both are valid T-SQL next to TOP, so the original query runs correctly as-is. The bare paginate-wrap shape keeps collapsing. Adds container-free unit tests for all three shapes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughMSSQL pagination flattening now avoids collapsing queries with top-level ChangesMSSQL pagination rewrite
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/connector/mssql.py`:
- Around line 134-135: Update _flatten_pagination_limit() so guarded LIMIT
queries with outer WITH or ORDER BY are re-emitted through the T-SQL dialect
instead of returning raw SQL containing LIMIT. In
core/wren/tests/connectors/test_mssql.py lines 315-335, add equivalent LIMIT
regression cases for both CTE and ORDER BY scenarios, verifying the generated
queries are valid T-SQL.
In `@core/wren/tests/connectors/test_mssql.py`:
- Around line 315-335: Extend test_flatten_pagination_preserves_outer_cte and
test_flatten_pagination_preserves_outer_order_by with equivalent LIMIT 3 inputs,
asserting the outer WITH or ORDER BY remains present and the output uses valid
T-SQL pagination (TOP or FETCH) without any LIMIT clause. Keep the existing
TOP-based coverage intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff967499-ff2d-4698-9aab-3e707c46d366
📒 Files selected for processing (2)
core/wren/src/wren/connector/mssql.pycore/wren/tests/connectors/test_mssql.py
…ll becomes TOP/FETCH Addresses review feedback: returning the raw query on the WITH/ORDER BY guard path would leave a literal LIMIT unrewritten (invalid T-SQL). Re-emit the parsed statement via the tsql dialect instead — WITH and ORDER BY survive and LIMIT is rewritten to TOP/FETCH. Adds two unit tests covering LIMIT with an outer CTE and with an outer ORDER BY.
|
Addressed the review feedback: the guard path now re-emits the parsed statement through the tsql dialect instead of returning the raw query, so a literal Verified locally: all 5 flatten unit tests pass, and |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/tests/connectors/test_mssql.py`:
- Line 343: Update the pagination assertions in the affected MSSQL connector
tests to verify the numeric limit as an exact token, not a substring that can
match larger values such as 30. Apply this to both TOP and NEXT forms at the
assertions around the referenced cases, using token-boundary matching or parsed
SQL while preserving the existing expected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c0a7f96-ac1b-4e5f-8a92-af59d2ca644c
📒 Files selected for processing (2)
core/wren/src/wren/connector/mssql.pycore/wren/tests/connectors/test_mssql.py
🚧 Files skipped from review as they are similar to previous changes (1)
- core/wren/src/wren/connector/mssql.py
Substring checks like 'TOP 3 in out' would also match TOP 30; use a regex helper that verifies the numeric limit exactly for both the TOP and FETCH NEXT forms.
|
Also addressed the remaining minor comment in 08479f6: the pagination assertions now use a token-boundary-aware helper ( |
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Thanks @abdullahharunozturk, nice catch 👍
2 participants
Problem
MSSqlConnector._flatten_pagination_limitcollapses the v4 paginate-wrap shapeSELECT * FROM (inner) LIMIT ninto the inner select, but it returns only the inner node. Two outer clauses are silently dropped:WITH(e.g. semantic-layer model CTEs emitted bydry_plan) — the flattened query then references CTEs that no longer exist and fails with Invalid object name;ORDER BY—TOP nthen returns arbitrary rows instead of the requested top-n, a silent correctness bug.Repro (both shapes are produced by wrapping a cube/aggregate query for ordering + limit):
flattens to
SELECT TOP 3 a, SUM(b) AS s FROM m GROUP BY a→Invalid object name 'm'; without the CTE the ORDER BY loss makes TOP 3 nondeterministic.Fix
Bail out of flattening when the outer select carries
WITHorORDER BY— both are valid T-SQL next toTOP, so the original query runs correctly as-is. The bare paginate-wrap shape still collapses as before.Tests
Three container-free unit tests in
tests/connectors/test_mssql.py: CTE preserved, ORDER BY preserved, bare wrap still collapses. Verified end-to-end against SQL Server 2022 (real data): before the fix the CTE shape errors and the ORDER BY shape returns wrong rows; after the fix both return correct results.🤖 Generated with Claude Code
Summary by CodeRabbit
WITH(CTEs) andORDER BYwhen present.LIMITis correctly rewritten into valid T-SQL while keeping supported outer clauses intact.WITHand outerORDER BYscenarios, plus standard limit rewriting behavior.