Failing test for an ORDER BY that is INNER JOINed in a subquery #1267
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3519 We use Jira to track the state of pull requests and the versions they got |
|
Thanks - I will take a look at this. |
|
@Ocramius @stof I'm not entirely sure how best to fix this. I think if an orderBy clause references an identifier for a joined table, the simple solution would be to also ensure that the query is fetch joining it. But that could have performance implications. The alternative I think would be to add any orderBy expressions not matched in the select list as hidden aliased columns... But I am not sure how best to approach that either. Thoughts? |
| // current output is: | ||
| // SELECT DISTINCT id_0, a1_.name FROM (SELECT b0_.id AS id_0, b0_.author_id AS author_id_1, b0_.category_id AS category_id_2 FROM BlogPost b0_ INNER JOIN Author a1_ ON b0_.author_id = a1_.id) dctrn_result ORDER BY a1_.name ASC | ||
| // | ||
| // The problem is a1_.name is not output from the subquery, so it cannot be included in the ORDER BY clause. |
There was a problem hiding this comment.
Did you pin down which bit strips a1_.name?
There was a problem hiding this comment.
a1_.name is not being output in the select list when LimitSubqueryOutputWalker calls $innerSql = parent::walkSelectStatement($AST);
So either it would need to be added to the select clause in the AST somehow, or it would have to be inserted after the AST is materialized to SQL.
|
@stof agree. Functional tests around ordering make totally sense IMO as it gives us more safety that the rather complex SQL also works in the end. 👍 |
|
@stof @deeky666 I think the functional tests are a great idea. We can see from the generated SQL that it won't work, but it was our DB crying at @austinsmorris and myself that tipped us off to the problem in the first place. |
|
Can we revert the original PR that caused this in the meantime? Usage in MySQL and maybe PostgreSQL + Oracle is now broke for a change that was meant to fix SQL Server. I'm sure SQL Server has a pretty small market share of Doctrine usage. |
|
Agreed with that. Please clean up the test code and open a new PR with the actual "final" If you want, you can include the merge revert commit yourself: otherwise Marco Pivetta On 20 January 2015 at 16:21, Kristopher Wilson notifications@github.com
|
austinsmorris
force-pushed
the
subquery-orderby
branch
from
2537ae6
to
6ac1c42
Compare
|
Moved to #1283 - please review it. |
A failing test case to demonstrate a problem with #1220.
cc: @Ocramius @zeroedin-bill, @mvrhov, @stof, @brianium, @mrkrstphr