-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix DISTINCT
queries with LIMIT
and no ORDER
on SQLServer 2012
#827
Fix DISTINCT
queries with LIMIT
and no ORDER
on SQLServer 2012
#827
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-1187 We use Jira to track the state of pull requests and the versions they got |
…ery - determined unnecessary.
// In another DBMS, we could do ORDER BY 0, but SQL Server gets angry if you use constant expressions in | ||
// the order by list. | ||
$query .= " ORDER BY (SELECT 0)"; | ||
if (strtoupper(substr($query, 0, 15)) == 'SELECT DISTINCT') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: stripos($query, 'SELECT DISTINCT') === 0
$query .= " ORDER BY (SELECT 0)"; | ||
if (stripos($query, 'SELECT DISTINCT') === 0) { | ||
// SQL Server won't let us order by a non-selected column in a DISTINCT query, | ||
// so we have to do this madness. This says, order by the first column in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's quite the madness: is there a way to integration-test this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is, but there is no existing functional test suite for DBAL that covers this kind of stuff, and I don't have time to write one. Ideally we should have a testsuite that covers actually executing these queries on all platforms, but we don't :(
I've been using the ORM as my integration testsuite for DBAL, bad as that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this PR as is for now. We should add the integration tests for the limit/offset modifications in seperate PR anyways.
…itquery Fix DISTINCT queries with limit and no order on SQL Server 2012
Thanks @zeroedin-bill |
DISTINCT
queries with LIMIT
and no ORDER
on SQLServer 2012
On SQLServer2012Platform, doModifyLimitQuery adds a do-nothing ORDER BY clause, because this is required in order to use OFFSET...FETCH NEXT N ROWS ONLY.
DISTINCT queries run via the paginator without an ORDER BY clause on SQL Server 2012 were failing with this error:
This PR fixes the error by adding 0 to the select list and changing the generated ORDER BY clause to read ORDER BY 1.
So a query that would have been generated as:
Will now be generated as: