Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix wrong offset in SQL Server when not using ORDER BY #1318

Open
wants to merge 5 commits into from

5 participants

Diego Rocha resurtm Carsten Schmitz Qiang Xue gabistan
Diego Rocha

fix issue #166

Simple SQL test script:

--test table
CREATE TABLE test_offset;
--test data
INSERT INTO test_offset VALUES(2);
INSERT INTO test_offset VALUES(4);
INSERT INTO test_offset VALUES(6);
INSERT INTO test_offset VALUES(8);
INSERT INTO test_offset VALUES(10);
INSERT INTO test_offset VALUES(12);
INSERT INTO test_offset VALUES(14);

--which LIMIT = 3
--and OFFSET = 2
--expected values are: 6,8,10

--current implementation
SELECT * FROM (
SELECT TOP 3 * FROM (
SELECT TOP 5 [id] FROM [test_offset]
) as [inner]
) as [outer];
--returned values: 2,4,6
--issue: wrong range of values

--proposed implementation
SELECT * FROM (
SELECT
ROW_NUMBER() OVER(ORDER BY (SELECT 1)) AS [RowNumber],
*
FROM (
SELECT [id] FROM [test_offset]
) as [inner]
) as [outer]
WHERE [RowNumber] BETWEEN 3 AND 5;
--returned values: 6,8,10
--issue: the column "RowNumber" is added to result

Diego Rocha Diego-Rocha Update framework/db/schema/mssql/CMssqlCommandBuilder.php
Fix wrong offset in SQL Server.

Simple SQL test script:

--test table
CREATE TABLE [test_offset]([id] INTEGER);
--test data
INSERT INTO [test_offset]([id]) VALUES(2);
INSERT INTO [test_offset]([id]) VALUES(4);
INSERT INTO [test_offset]([id]) VALUES(6);
INSERT INTO [test_offset]([id]) VALUES(8);
INSERT INTO [test_offset]([id]) VALUES(10);
INSERT INTO [test_offset]([id]) VALUES(12);
INSERT INTO [test_offset]([id]) VALUES(14);

--which LIMIT = 3
--and OFFSET = 2
--expected values are: 6,8,10

--current implementation
SELECT * FROM (
	SELECT TOP 3 * FROM (
		SELECT TOP 5 [id] FROM [test_offset]
	) as [__inner__]
) as [__outer__];
--returned values: 2,4,6
--issue: wrong range of values

--proposed implementation
SELECT * FROM (
	SELECT
		ROW_NUMBER() OVER(ORDER BY (SELECT 1)) AS [__RowNumber__],
		*
	FROM (
		SELECT [id] FROM [test_offset]
	) as [__inner__]
) as [__outer__]
WHERE [__RowNumber__] BETWEEN 3 AND 5;
--returned values: 6,8,10
--issue: the column "__RowNumber__" is added to result
d319d16
resurtm
Collaborator

@Diego-Rocha By the way, if you're familiar with MSSQL it would be nice if you tried to run updated MSSQL unit tests. This would help improve MSSQL support. Apparently i'm alone who tried to execute them. :-)

Diego Rocha

Done!

Diego Rocha

The problem with the current implementation showed in issue #166 is the necessity to use a ORDER BY to be consistent with the native OFFSET implementations of postgresql and mysql.

Another Advantage with the proposed solution is the performance:

--old OFFSET implementation
SELECT * FROM (SELECT TOP 2 * FROM (SELECT TOP 5 id, title FROM [dbo].[posts] [t] ORDER BY title) as [__inner__] ORDER BY title DESC) as [__outer__] ORDER BY title ASC;

--proposed OFFSET implementation
SELECT * FROM (SELECT ROW_NUMBER() OVER(ORDER BY (SELECT 1)) AS [__RowNumber__], * FROM (SELECT id, title FROM [dbo].[posts] [t]) as [__inner__]) as [__outer__] WHERE [__RowNumber__] BETWEEN 4 AND 5 ORDER BY title ASC;

Running the execution plan (the EXPLAIN of SQL Server)

I got the following Results:

  1. Query 1: Query Cost (relative to the batch): 72%
  2. Query 2: Query Cost (relative to the batch): 28%

A great performance boost!!!

The only drawbacks are:

  • The ROW_NUMBER() is only avaliable in SQL Server 2005 and above, breaking the compatibility with SQL Server 2000;
  • The additional field "__RowNumber__" returned in the final query (can be removed with a replace between the first occurrence of "SELECT *" with the original field list)
Carsten Schmitz

I can reproduce this. A fix would be awesome. Can we somehow determine the SQL version number and apply it the solution accordingly? Use the old 'buggy' solution automatically for older versions and the newer one for 2005 and above?

Carsten Schmitz

@qiangxue I cannot find it anywhere in the Yii specs: What minimum version of MSSQL is Yii supposed to support? (considering the fact that MSSQL 2000 is now more than 12 years old)

Qiang Xue
Owner

I think we can bring up the minimum mssql version to a prevalent one (you have better knowledge than me on this aspect.)

Diego Rocha

Refactor and fix for placing the right "ORDER BY" inside the "OVER"

The rewriteLimitOffsetSql function is different in Yii 1.1.14 release, but this seems to work better for me. It fixes the issues with changing pages when a column is sorted, but it doesn't work on related columns. Do you know why that is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 31, 2012
  1. Diego Rocha

    Update framework/db/schema/mssql/CMssqlCommandBuilder.php

    Diego-Rocha authored
    Fix wrong offset in SQL Server.
    
    Simple SQL test script:
    
    --test table
    CREATE TABLE [test_offset]([id] INTEGER);
    --test data
    INSERT INTO [test_offset]([id]) VALUES(2);
    INSERT INTO [test_offset]([id]) VALUES(4);
    INSERT INTO [test_offset]([id]) VALUES(6);
    INSERT INTO [test_offset]([id]) VALUES(8);
    INSERT INTO [test_offset]([id]) VALUES(10);
    INSERT INTO [test_offset]([id]) VALUES(12);
    INSERT INTO [test_offset]([id]) VALUES(14);
    
    --which LIMIT = 3
    --and OFFSET = 2
    --expected values are: 6,8,10
    
    --current implementation
    SELECT * FROM (
    	SELECT TOP 3 * FROM (
    		SELECT TOP 5 [id] FROM [test_offset]
    	) as [__inner__]
    ) as [__outer__];
    --returned values: 2,4,6
    --issue: wrong range of values
    
    --proposed implementation
    SELECT * FROM (
    	SELECT
    		ROW_NUMBER() OVER(ORDER BY (SELECT 1)) AS [__RowNumber__],
    		*
    	FROM (
    		SELECT [id] FROM [test_offset]
    	) as [__inner__]
    ) as [__outer__]
    WHERE [__RowNumber__] BETWEEN 3 AND 5;
    --returned values: 6,8,10
    --issue: the column "__RowNumber__" is added to result
  2. Diego Rocha

    Update tests/framework/db/schema/CMssqlTest.php

    Diego-Rocha authored
    Updated test case to new offset find
  3. Diego Rocha

    Update framework/db/schema/mssql/CMssqlCommandBuilder.php

    Diego-Rocha authored
    Fixed implementation of rewriteLimitOffsetSql
  4. Diego Rocha

    Update framework/db/schema/mssql/CMssqlCommandBuilder.php

    Diego-Rocha authored
    fixed white space bug with special querys
Commits on Feb 4, 2013
  1. Diego Rocha
This page is out of date. Refresh to see the latest.
12 framework/db/schema/mssql/CMssqlCommandBuilder.php
View
@@ -190,22 +190,22 @@ public function applyLimit($sql, $limit, $offset)
/**
* Rewrite sql to apply $limit > and $offset > 0 for MSSQL database.
- * See http://troels.arvin.dk/db/rdbms/#select-limit-offset
* @param string $sql sql query
* @param integer $limit $limit > 0
* @param integer $offset $offset > 0
* @return string modified sql query applied with limit and offset.
*
- * @author Wei Zhuo <weizhuo[at]gmail[dot]com>
+ * @author Diego Rocha <diego[dot]rocha[dot]br[at]gmail[dot]com>
*/
protected function rewriteLimitOffsetSql($sql, $limit, $offset)
{
- $fetch = $limit+$offset;
- $sql = preg_replace('/^([\s(])*SELECT( DISTINCT)?(?!\s*TOP\s*\()/i',"\\1SELECT\\2 TOP $fetch", $sql);
+ $end = $limit + $offset;
+ $start = $end - $limit + 1;
$ordering = $this->findOrdering($sql);
$orginalOrdering = $this->joinOrdering($ordering, '[__outer__]');
- $reverseOrdering = $this->joinOrdering($this->reverseDirection($ordering), '[__inner__]');
- $sql = "SELECT * FROM (SELECT TOP {$limit} * FROM ($sql) as [__inner__] {$reverseOrdering}) as [__outer__] {$orginalOrdering}";
+ $rowNumberOrdering = !empty($orginalOrdering) ? $orginalOrdering : 'ORDER BY (SELECT 1)';
+ $sqlWithoutOrder = preg_replace('/([ ]ORDER BY)[\s"\[](.*)(ASC|DESC)?(?:[\s"\[]|$|COMPUTE|FOR)/i','',$sql);
+ $sql = "SELECT * FROM (SELECT ROW_NUMBER() OVER({$rowNumberOrdering}) AS [__RowNumber__], * FROM ({$sqlWithoutOrder}) as [__inner__]) as [__outer__] WHERE [__RowNumber__] BETWEEN {$start} AND {$end}";
return $sql;
}
3  tests/framework/db/schema/CMssqlTest.php
View
@@ -212,7 +212,8 @@ public function testCommandBuilder()
'order'=>'title',
'limit'=>2,
'offset'=>3)));
- $this->assertEquals('SELECT * FROM (SELECT TOP 2 * FROM (SELECT TOP 5 id, title FROM [dbo].[posts] [t] ORDER BY title) as [__inner__] ORDER BY title DESC) as [__outer__] ORDER BY title ASC',$c->text);
+ $expectedSql = 'SELECT * FROM (SELECT ROW_NUMBER() OVER(ORDER BY (SELECT 1)) AS [__RowNumber__], * FROM (SELECT id, title FROM [dbo].[posts] [t]) as [__inner__]) as [__outer__] WHERE [__RowNumber__] BETWEEN 4 AND 5 ORDER BY title ASC';
+ $this->assertEquals($expectedSql,$c->text);
$rows=$c->query()->readAll();
$this->assertEquals(2,count($rows));
$this->assertEquals('post 4',$rows[0]['title']);
Something went wrong with that request. Please try again.