DBAL-834: SQLServer modifyLimitQuery does not work with aggregate functions in ORDER BY #2069

Closed
doctrinebot opened this Issue Mar 10, 2014 · 21 comments

2 participants

@doctrinebot

Jira issue originally created by user osvi:

Starting with Doctrine 2.4, the modifyLimitQuery method does not work anymore with query using ORDER BY MAX(...)
See this example:

$sql = "SELECT MAX(heading_id) aliased, code
    FROM operator*model*operator
    GROUP BY code
    ORDER BY MAX(heading_id) DESC
";
$sql = $this->em->getConnection()->getDatabasePlatform()->modifyLimitQuery(
    $sql, 1, 0
);

Doctrine generates this SQL, which is invalid:

SELECT * FROM (SELECT MAX(heading_id) aliased, code
, ROW*NUMBER() OVER (ORDER BY MAX(heading_id) AS doctrine_rownum FROM operator_model*operator GROUP BY code) DESC
) AS doctrine*tbl WHERE doctrine*rownum BETWEEN 1 AND 1```
The ORDER BY in moved into the OVER(), but the `preg_replace` in SQLServerPlatform.php stops to replace at the closing ")".
@doctrinebot

Comment created by osvi:

It is not possible to write ORDER BY aliased because it leads to a syntax error in SQL Server.

@doctrinebot

Comment created by @deeky666:

[~osvi] There have been some modifications to the modifyLimitQuery() method in SQL Server lately for 2.5 which address some problems with subqueries and aggregate functions. Not sure if that might already solve your issue. Can you please check if the problem also exists in the current master branch of DBAL?
See commit: 9f3cb43

@doctrinebot

Comment created by osvi:

  - Removing doctrine/common (v2.4.1)
  - Installing doctrine/common (2.4.x-dev 9a7e20e)
    Cloning 9a7e20e779360f3b8a02c27a89d47d5a6fdce8d1

  - Removing doctrine/dbal (v2.4.2)
  - Installing doctrine/dbal (dev-master c61361d)
    Cloning c61361d8fcf65a977d8610ba78eb542a1d2f44b4

  - Removing doctrine/orm (v2.4.2)
  - Installing doctrine/orm (2.4.x-dev a949e87)
    Cloning a949e87ca88299cde368d2b574740753526b62c9

Same issue.

@doctrinebot

Comment created by flip101:

on this line here https://github.com/doctrine/dbal/blob/9f3cb437c0f491599de4e1bd847235965f98ffd4/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php#L1164

try the following stuffs:

Puts "DESC" in a second capturing group (closer to the original regex, but not sure why you want to do this)
/ORDER\s<ins>BY\s</ins>([<sup>)(]**(?:\(\w<ins>\))?)(.**)/

Includes "DESC" in the first capturing group
/ORDER\s</ins>BY\s<ins>([</sup>)(]**(?:\(\w</ins>\))?.**)/

Same as last one, except this one stops capturing when it hits a ")" after "DESC"
/ORDER\s<ins>BY\s</ins>([<sup>)(]**(?:\(\w+\))?[</sup>)]**)/
@doctrinebot

Comment created by mklocke:

This not only affects Queries with aggregate functions, but every Query that uses a limit and order by an entity-field alias.

Try this Testcase:

$query = $this->em->createQuery("
    SELECT a.id AS test
    FROM prim\\entity\\Article a
    ORDER BY test ASC
");
$query->setMaxResults(10);
echo "<h3>DQL</h3>";
var_dump($query->getDQL());
echo "<br><h3>SQL</h3>";
var_dump($query->getSQL());
echo "<br><h3>Result</h3>";
var_dump($query->getResult());
@doctrinebot

Comment created by flip101:

The test case is incomplete as we don't have kare\\entity\\Article. Please try the 3 proposed solutions and show the results from those adjustments.

@doctrinebot

Comment created by mklocke:

This is just a sample Query for illustration. Replace it with whatever Entity you like.

@doctrinebot

Comment created by @deeky666:

Patch supplied in PR: #573

[osvi] [flip101] [~mklocke] please review.

@doctrinebot

Comment created by osvi:

Hi Steve,

I tried 2.5.*@dev now and everything works for me.
About the code, I am not a regexp guru.

Just for review purpose, can you explain this regexp?
deeky666@df1f3e4#diff-8a544d213159863ef39497f4b139b420R1155

Thank you.

@doctrinebot

Comment created by @deeky666:

The regex replaces the ORDER BY including nested parentheses expressions (uses recursion for that) until a terminating sequence is detected (for example a closing parenthesis that has no corresponding opening parenthesis from a previous ORDER BY expression.
This might not be perfect and complete but it is an improvement. It does not stop matching after the first closing parenthesis found.
Please note that the PR has not been merged yet so I am not sure whether you had the patch applied in your 2.5.*@dev version constraint.

@doctrinebot

Comment created by osvi:

Thank you Steve. I confirm you I tested your code (manually copied and pasted your patch to SQLServerPlatform in 2.5.*@dev :-)).
Maybe it is not perfect, but surely a huge improvement.

@doctrinebot

Comment created by @deeky666:

Alright. Thanks for testing.

@doctrinebot

Comment created by mklocke:

I've had no time for testing this yet, but i read your code changes and i think this is definitely a big step forward. But it would be a lot nicer if you could always ORDER BY an alias in DQL. DBAL's goal is abstract away database specific language and for now users still have to worry about the Platform while writing DQL queries with ORDER BY :/

@doctrinebot

Comment created by @deeky666:

I'm not sure if I understand correctly. Can you please give an example of what you mean? It sounds like it's another issue?

@doctrinebot

Comment created by mklocke:

SELECT MAX(heading_id) aliased, code
FROM operator*model*operator
GROUP BY code
ORDER BY aliased DESC

This Query won't work with modifyLimitQuery

@doctrinebot

Comment created by @deeky666:

[~mklocke] Okay I think I get what you mean but that is another issue IMO. There should be a new ticket for this.

@doctrinebot

Comment created by osvi:

Can we merge this in master/2.5?

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-573] was closed:
#573

@doctrinebot

Comment created by @deeky666:

Fixed in commit: 4a7ff71

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot doctrinebot added the Bug label Dec 7, 2015
@deeky666 deeky666 was assigned by doctrinebot Dec 7, 2015
@doctrinebot doctrinebot added this to the 2.5 milestone Dec 7, 2015
@doctrinebot doctrinebot closed this Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment