Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqlsrvDriver: offset must have a order by #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rumcais
Copy link

@rumcais rumcais commented Jul 27, 2018

  • bug fix

@rumcais
Copy link
Author

rumcais commented Jul 30, 2018

Limitations in Using OFFSET-FETCH
ORDER BY is mandatory to use OFFSET and FETCH clause.

OFFSET clause is mandatory with FETCH. You can never use, ORDER BY … FETCH.

TOP cannot be combined with OFFSET and FETCH in the same query expression.

The OFFSET/FETCH rowcount expression can be any arithmetic, constant, or parameter expression that will return an integer value. The rowcount expression does not support scalar sub-queries.

https://technet.microsoft.com/en-us/library/gg699618(v=sql.110).aspx

@dg
Copy link
Owner

dg commented Aug 1, 2018

I think ORDER BY must be added by programmer, because he knows how to order rows. ORDER BY 1 solves nothing.

@rumcais
Copy link
Author

rumcais commented Aug 2, 2018

But the queries you have in the tests do not pass to MSSQL. "SELECT 1 OFFSET 10 ROWS FETCH NEXT 10 ROWS ONLY" ends with MSSQL error. Furthermore, FetchSingle() does not pass a simple query, and it does problems with addons like datagrid, etc.

@dg
Copy link
Owner

dg commented Aug 3, 2018

But the queries you have in the tests do not pass to MSSQL. "SELECT 1 OFFSET 10 ROWS FETCH NEXT 10 ROWS ONLY" ends with MSSQL error.

So tests should be fixed, or not?

Furthermore, FetchSingle() does not pass a simple query, and it does problems with addons like datagrid, etc.

Can you send here example code?

@dg dg force-pushed the master branch 4 times, most recently from e5e6a36 to 7a49609 Compare September 17, 2018 11:46
@dg dg force-pushed the master branch 2 times, most recently from 4e171c7 to 0535d57 Compare August 30, 2019 16:55
@dg dg force-pushed the master branch 5 times, most recently from 6b34262 to 34e1603 Compare February 28, 2020 15:41
@dg dg force-pushed the master branch 2 times, most recently from 680e3f6 to ed2a827 Compare March 26, 2020 03:10
@ghost
Copy link

ghost commented Apr 10, 2020

Perhaps we could throw an exception instead from SqlsrvDriver and PdoDriver (the sqlsrv case of applyLimit()) for SQL Server >= 2012, explaining that order must be specified in order to apply the limit? It fails with Incorrect syntax near '0' anyway, so it wouldn't cause a BC break.

I think that would be a good hint for simple cases like:

$dibi->select('*')->from('table')->fetch();

When I read this, I assume it does something like this:

SELECT TOP 1 * FROM [table]

ORDER BY 1 doesn't seem right to me, that might end up in getting an unexpected result, as you are sorting by first column of your query (from SELECT clause), which might be different from the default order (primary key).

If we don't want to force the programmer to order the query, fallback to SELECT TOP 1 ... might be a better option and might actually work without a subquery, since we don't need to offset the result.

@dg dg force-pushed the master branch 6 times, most recently from 4c708e6 to bc0332a Compare October 8, 2020 15:30
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.

3 participants