Fixed SQLServer ORDER BY problem in paginator CountOutputWalker #572

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@norzechowicz
Contributor

Code explains everything :)

@doctrinebot

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2282

@Ocramius
Member
Ocramius commented Feb 7, 2013

@norzechowicz such kind of fix needs a test that was failing before the patch was applied.

@mvrhov
Contributor
mvrhov commented Feb 7, 2013

Well the orderBy on count is not needed on any database, but I had crashes on 5.4 if set to null.

@norzechowicz
Contributor

@mvrhov there is a comment above walkSelectStatement() method. https://github.com/norzechowicz/doctrine2/blob/d04dc7421430a493cdb35035b36c2b777a4f2ad2/lib/Doctrine/ORM/Tools/Pagination/CountOutputWalker.php#L71 I think this is the reasony why ORDER BY is not removed from count query.

@Ocramius Any suggestions how should I write this test? This is sql server engine problem and without connection to database I can't prove that the current query is failing.

Below comment comes from database engine.

The ORDER BY clause is invalid in views, inline functions, derived tables, subqueries, and common table expressions, unless TOP or FOR XML is also specified

Btw. This is the tests result with SQL Server 2008 R2 database:

FAILURES! Tests: 1878, Assertions: 6555, Failures: 2, Errors: 20, Skipped: 33.

@Ocramius
Member
Ocramius commented Feb 8, 2013

@norzechowicz if you configured the test suite to use a db on your SQL Server, then you're already good to go (check phpunit.xml.dist)

@norzechowicz
Contributor

@Ocramius At the moment pagination tests operate on ConnectionMock, should this be changed?

@Ocramius
Member
Ocramius commented Feb 8, 2013

@norzechowicz you can do assertions on the SQL thrown at the connection mock

@norzechowicz
Contributor

@Ocramius sorry but I dont understand how assertion on connection mock can prove that something is failing on a specific db engine :/ I'm afraid I will not be able to write this test.

@mrsushiface

Any movement on this? SQLServer user here encountering issue, confirmed code fixed my issue.

@norzechowicz
Contributor

@Ocramius Test added, can you check it now?

@Ocramius Ocramius and 1 other commented on an outdated diff Feb 26, 2013
lib/Doctrine/ORM/Tools/Pagination/CountOutputWalker.php
@@ -79,6 +80,13 @@ public function __construct($query, $parserResult, array $queryComponents)
*/
public function walkSelectStatement(SelectStatement $AST)
{
+ // The ORDER BY clause is invalid in views, inline functions, derived
+ // tables, subqueries, and common table expressions, unless TOP or
+ // FOR XML is also specified
+ if ($this->platform instanceof SQLServerPlatform) {
@Ocramius
Ocramius Feb 26, 2013 Member

How about nulling it every time? That doesn't change the result, right?

@norzechowicz
norzechowicz Feb 26, 2013 Contributor

This should not change the result but there is a comment above that function

Note that the ORDER BY clause is not removed. Many SQL implementations (e.g. MySQL)
are able to cache subqueries. By keeping the ORDER BY clause intact, the limitSubQuery
that will most likely be executed next can be read from the native SQL cache.

Also @mvrhov tried this before but it crashed at php 5.4
I think it will be safe to remove it only when SQLServerPlatform is used, what do you think?

@Ocramius
Ocramius Feb 26, 2013 Member

@norzechowicz If the result is not affected (since we're just counting) I'd go with removing the orderByClause overall instead of introducing it as a hack for a single platform. The comment can still live.

@norzechowicz
norzechowicz Feb 26, 2013 Contributor

@Ocramius fair enough, sholud I do it?

@Ocramius
Ocramius Feb 26, 2013 Member

Yes please

@norzechowicz
Contributor

@Ocramius done :)

@norzechowicz
Contributor

@beberlei is there any chance to merge this into 2.3 branch?

@norzechowicz
Contributor

@doctrine I know u guys are very busy but this is rly big issue for SQL Server 2008 users.

@beberlei beberlei added a commit that referenced this pull request Mar 12, 2013
@beberlei beberlei Merge branch 'GH-572' 3ba0562
@beberlei beberlei added a commit that referenced this pull request Mar 12, 2013
@beberlei beberlei Merge branch 'GH-572' into 2.3 550c1cf
@beberlei
Member

Adjusted to affect SQL Server only, squashed, Merged into master and into 2.3

@beberlei beberlei closed this Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment