* LimitSubqueryOutputWalker should not duplicate orderBy clauses

[billschaller]

When using the paginator, the LimitSubqueryOutputWalker uses the passed query as a subquery in a generated query.

Ex:

SELECT a0_.id AS id_0, a0_.name AS name_1, a0_.name AS name_2 FROM Author a0_ ORDER BY a0_.name

Becomes:

SELECT DISTINCT id_0, name_2 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1, a0_.name AS name_2 FROM Author a0_ ORDER BY a0_.name) dctrn_result ORDER BY name_2 DESC

This transformation duplicates the ORDER BY clause unnecessarily -- In SQL Server, this is not valid - it will cause an error unless combined with the TOP clause. In other DBMS, the inner ORDER BY clause is meaningless unless combined with a limit/offset clause, and even then there is no guarantee that the order prescribed in the inner subquery will be maintained in the outer expression. ORM does not generate any subqueries where the order by clause is meaningful. The duplicate inner ORDER BY clause can safely be dropped, without any side effects.

I have modified LimitSubqueryOutputWalker to drop ORDER BY clauses in the subquery generated in the walkSelectStatement method. I have also modified the tests to remove the ORDER BY clauses inside subqueries.

[Ocramius]

Change looks good, but it may only land in 2.5 because of the test changes, which may lead to unforseen breakages.

[Ocramius]

Need feedback from @glen-84 on this.

[mvrhov]

Afaik there is comment inside a file, that dropping inner order by may cause segmentation faults in some php versions.

[Ocramius]

may cause segmentation faults in some php versions.

Regex bug? I would just suggest a php version upgrade in those cases.

[mvrhov]

AFAIR NO. And I've tried a numerous versions at that time.

[Ocramius]

@mvrhov since the travis versions seem to be fine, I'll still merge and wait for reported segfaults, if there will be any.

[billschaller]

Work in progress.

[billschaller]

@Ocramius I rebuilt LimitSubqueryOutputWalker::preserveSqlOrdering to handle all complex expressions involving columns.

It seemed cleaner to handle all order by items with the same code block than to detect PathExpression nodes and handle them in a separate codepath.

I think in a future release it might be better to handle this by running logic against the AST before the SQL is generated, but for now this works for all test cases.

On Oracle and Mysql, it's not necessary to include OrderBy items in the select list of a SELECT DISTINCT query, but since it doesn't change the way the query runs, I didn't write special-case logic for those platforms. All platforms get the OrderBy items in the select list.

If DQL had provisions for aggregate functions in order by clauses, this code would need to be revised to create a group by expression as well. Luckily DQL does not allow aggregates in order by 😅

[mvrhov]

On Oracle and Mysql, it's not necessary to include OrderBy items in the select list of a SELECT DISTINCT query, but since it doesn't change the way the query runs, I didn't write special-case logic for those platforms. All platforms get the OrderBy items in the select list.

Unfortunately you are wrong about this. Please take a look at numerous bugfixes regarding that.

[billschaller]

@mvrhov The existing logic in master does the same thing as the new logic. I tested queries on newer versions of Oracle and Mysql, so older versions may behave differently... What am I wrong about? Can you link me some references?

[mvrhov]

PR #939 is one of them..

[billschaller]

@mrrhov I think we're in agreement on what the functionality should be, but I want to be completely certain we understand each other...

There are 2 problems with the logic currently in master:

1. When it copies the order by items out of the query to build the order by clause in the wrapping query, it copies only simple path expression items referencing a single column. It should instead copy and transform all items in the order by clause. Do you disagree with this point?
2. An order by clause inside a subquery without TOP or LIMIT effectively does nothing. I do not know of any DBMS where this is not the case. SQL Server returns an error in this case. The existing logic in master does not remove the order by clause when it wraps the query. It should instead effectively move and transform the order by clause from the subquery to the outer SELECT DISTINCT query. I believe this will behave properly on all platforms. Do you disagree with this point?

[stof]

I think in a future release it might be better to handle this by running logic against the AST before the SQL is generated, but for now this works for all test cases.

We already have an implementation working on the AST. However, each implementation has some limitations (some cases cannot be handled by the AST walker, for instance when the query already has other walkers). This is why we have 2 implementations to cover more cases

[Ocramius]

Merged into master at 8f097ab

Thanks @zeroedin-bill!

[billschaller]

Yay Thanks! :)

[austinsmorris]

@Ocramius, @zeroedin-bill, this broke ordering for pagination. Here's what's happening:

If the inner subquery did a JOIN and ordering was meant to be done on a column in the joined table, the actual orderby column is not selected in the subquery. This results in the outer query (where the ordering is meant to take place) selecting a column that is not available to it 💥!

Still trying to make sense of the changes here so I can submit a fix, so if you know how to fix, let me know!

Thanks!

cc @brianium, @mrkrstphr

[billschaller]

@austinsmorris Can you PR a failing testcase?

[mvrhov]

Well. as I said. The ORDER BY is important.! We are going through this every couple of months.

[stof]

This is precisely why providing a testcase is important, to avoid regressions

[austinsmorris]

See #1267. Let me know if you have any questions. Thanks for your help!

[ghost]

@zeroedin-bill could you be so kind and have a look at http://www.doctrine-project.org/jira/browse/DDC-3688 ? I believe it might be caused by this change or at least related to it?