igor-tkachev / bltoolkit

Business Logic Toolkit for .NET
MIT License
297 stars 112 forks source link

OFFSET used by mistake for SQL Server 2012. #365

Open ghost opened 9 years ago

ghost commented 9 years ago

The following SQL query is not valid from point of view of syntax:

SELECT DISTINCT ColumnA, ColumnB, ColumnC
FROM SomeTable
ORDER BY ColumnD, ColumnC
OFFSET 0 ROWS FETCH NEXT 50 ROWS ONLY 

because ColumnD is not in distinct select list.

But such pagination (Skip and Take) and ordering (OrderBy) are valid and possible. This issue is addressed only to OFFSET syntax available in SQL Server 2012 (and some others).

MsSql2008SqlProvider works fine because BuildAlternativeSql property is set to true. True for that property will force using of AlternativeBuildSql method. So this method is used to create another version of select query with row number. "ROW_NUMBER() OVER" syntax allows ordering with DISTINCT queries and pagination works fine. Of course row number is slower than offset. So we need to create some solution to consider both methods.

Potential way to do it is overriding of the following method for MsSql2012SqlProvider:

protected override void BuildSql(StringBuilder sb)
        {
            if (BuildAlternativeSql)
                AlternativeBuildSql(sb, true, base.BuildSql);
            else
                base.BuildSql(sb);
        }

There we need to detect distinct query and compare columns in select and in order by. But I am not sure how it is better to do... project structure. Maybe somebody who is more experienced in the project structure can suggest detection logic or refactoring of BuildAlternativeSql property.

Fast temp fix is related to disabling of OFFSET syntax. There are 2 ways: 1) Return "true" instead of "false":

protected override bool   BuildAlternativeSql { get { return false;             } }

2) Change database compatibility version to 100 (SQL Server 2008):

ALTER DATABASE [My Database] SET COMPATIBILITY_LEVEL = 100

In 2nd case SqlDataProviderVersionResolver will instantiate 2008 version of sql provider.

ili commented 9 years ago

I see... SQL Server demand to include order by columns into select closure.. may be we can check and if it is not so use old syntax....

ghost commented 9 years ago

SQL Server demands to include all columns from ORDER BY to SELECT DISTINCT clause. But LINQ expressions can be made in the way when we do not need to include it. In my example we need to sort items by ColumnD and after that perform distinct selection of top 50 rows using columns not related to ColumnD. From point of view of Linq experssion it is possible and it works if ROW_NUMBER syntax is used. I guess we need to switch to ROW_NUMBER instead of OFFSET only for queries with DISTINCT, ORDER BY and pagination at the same time and only in case when select and order by columns are different in expression. In all other cases we can use OFFSET because it works a bit faster.