FIX: emit FETCH clause for qualify queries sliced with offset=0 (#525) - #580
FIX: emit FETCH clause for qualify queries sliced with offset=0 (#525)#580Muhammad Faseeh (Faseeh06) wants to merge 1 commit into
Conversation
…osoft#525) A Window-function .filter() (e.g. .annotate(...).filter(row_number=1)) routes SQLCompiler.as_sql() through the get_qualify_sql() branch, which wraps the query in a subquery and entirely bypasses the plain-SELECT TOP %d insertion. The only place a limit/offset clause was appended afterward was the do_offset block, gated solely on low_mark != 0. So a qualify query sliced with offset=0 (qs[:50]) compiled with do_limit true but do_offset false: no limit clause was emitted at all, and the query returned every matching row instead of the requested slice. Non-zero offsets (qs[1:50]) already worked, since do_offset was true for them. Emit the FETCH-only form of the offset/limit clause for this qualify + limit-only case too. Adds regression tests that build the SQL text directly (no live database needed - the one call that would otherwise require a connection, sql_server_version, is pre-seeded via DatabaseWrapper's own version cache).
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a SQL compilation bug in the SQL Server backend where qualified (window-function-filtered) querysets sliced with offset=0 (e.g. qs[:50]) could lose their row limit and return an unbounded result set. The change updates mssql/compiler.py to emit the OFFSET 0 ROWS FETCH FIRST N ROWS ONLY clause for the qualify + limit-only case, and adds regression tests in testapp/ to prevent recurrence.
Changes:
- Fix
SQLCompiler.as_sql()to appendlimit_offset_sql()for qualify queries whendo_limitis true anddo_offsetis false (i.e.low_mark == 0). - Add regression tests covering
[:N],[1:N], and the unsliced qualify query SQL output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mssql/compiler.py |
Ensures qualify queries sliced with offset=0 still emit an outer limit via OFFSET 0 ... FETCH .... |
testapp/tests/test_compiler_qualify_limit.py |
Adds regression coverage validating SQL generation for qualify queries with/without slicing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@microsoft-github-policy-service agree |
📊 Code Coverage Report
Diff CoverageDiff: dev...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)- mssql/client.py: 19.5% (41 lines)
- mssql/__init__.py: 50.0% (2 lines)
- mssql/creation.py: 56.8% (74 lines)
- mssql/operations.py: 78.8% (396 lines)
- mssql/compiler.py: 82.3% (646 lines)
- mssql/functions.py: 84.1% (428 lines)
- mssql/base.py: 84.8% (514 lines)
- mssql/schema.py: 88.3% (719 lines)
- mssql/introspection.py: 90.2% (133 lines)
- mssql/features.py: 98.9% (88 lines)🔗 Quick Links
|
Fixes #525
A Window-function
.filter()(e.g..annotate(row_number=Window(...)).filter(row_number=1)) routesSQLCompiler.as_sql()through theget_qualify_sql()branch, which wraps the query in a subquery and entirely bypasses the plain-SELECTTOP %dinsertion further down in the method. The only place a limit/offset clause got appended afterward was the finalif do_offset:block — gated solely onlow_mark != 0.So a qualify query sliced with
offset=0(qs[:50]) compiles withdo_limittrue butdo_offsetfalse: no limit clause is emitted anywhere, and the query returns every matching row instead of the requested slice. Non-zero offsets (qs[1:50]) already worked correctly, sincedo_offsetwas true for them — that's exactly the asymmetry described in the issue.Changes
mssql/compiler.py: emit the FETCH-only form of the offset/limit clause (self.connection.ops.limit_offset_sql(...), which already producesOFFSET 0 ROWS FETCH FIRST N ROWS ONLYforlow_mark=0) for thequalify and do_limit and not do_offsetcase.testapp/tests/test_compiler_qualify_limit.pywith regression coverage for: the previously-brokenoffset=0slice, the already-working non-zero-offset slice (guard against regressing it), and the unsliced case (confirming no limit clause is added when none was requested).Test plan
dev, confirmed theFETCHclause was missing before and is emitted correctly afterstr(queryset.query), which only invokesSQLCompiler.as_sql()and never opens a cursor — the one call that would otherwise need a live connection,sql_server_version, is pre-seeded throughDatabaseWrapper's own version cache, sopython manage.py test testapp.tests.test_compiler_qualify_limitpasses with no database configured (Skipping setup of unused database(s): default, other, sqlite.)devwithout this change and pass with itflake8clean on both changed files