Paginator fixes take3

[billschaller]

This PR fixes issues reported in #1347 and #1351.

• Ordering a query by a column from an association which uses SINGLE_TABLE inheritance no longer throws an error.
• Limiting a query which is ordered by fields from a joined -To-Many association now works as expected.

This patch converts LimitSubqueryOutputWalker to use the ROW_NUMBER window function for queries on platforms that support it. Other platforms use a modified version of the previous method. The modification is to not select the ORDER BY columns in the wrapping query. The reason those columns were selected like that in the first place is that ORDER BY on columns not in the select list is unsupported on many platforms. MySQL and SQLite are fine with it, and don't support ROW_NUMBER.

The ROW_NUMBER solution is actually much simpler.

LimitSubqueryOutputWalker SQL generation tests for PG and Oracle have been changed.

There are 2 failing expectedException tests right now. The LimitSubqueryWalker needs to be modified to throw an exception when it detects a query that does the following things together:

• Joins a -To-Many association
• Orders by a column from the associated entity
• Limits the result

As with the previous patch, this depends on changes to SQLServerPlatform2008 that have not yet been merged. SQLServerPlatform2008 in DBAL/master currently mangles the generated queries when trying to apply LIMIT/OFFSET.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3645

We use Jira to track the state of pull requests and the versions they got
included in.

[billschaller]

@vaheshadunts @alexander-orabey I believe this fixes both of your issues... Please test.

[billschaller]

@mrkrstphr Does this fix your issue?

[mrkrstphr]

@zeroedin-bill no, it does not. I will try to write a failing test tomorrow.

[billschaller]

@mrkrstphr Ok, thanks - the functional testsuite is taking shape, and that should help prevent regressions like this in the future.

What platform are you running on?

[mrkrstphr]

Test suite runs in sqlite.

[vaheshadunts]

@zeroedin-bill as I see its ok in travis, I'll merge and test it later, thanks.

[antanas-arvasevicius]

Hi, I don't know is it related but now this query is throwing exception

        $q = $this->entityManager->createQuery(
            '
SELECT entity FROM System\Entity\SimpleEntity entity
WHERE  ((SELECT COUNT(simple.id) FROM System\Entity\SimpleEntity AS simple) = 1)
ORDER BY entity.id DESC
        '
        );
        $q->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');
        $list = $q->getScalarResult();

Exception:

An exception occurred while executing 'SELECT DISTINCT id_0 FROM (SELECT s0_.id AS id_0, s0_.name AS name_1, s1_.id AS id_2 FROM simple_entity s0_ WHERE ((SELECT COUNT(s1_.id) AS dctrn__1 FROM simple_entity s1_) = 1)) dctrn_result ORDER BY id_0 DESC':

SQLSTATE[42S22]: Column not found: 1054 Unknown column 's1_.id' in 'field list'

If I remove ORDER BY entity.id DESC from query then it runs fine.

Now its impossible to use Paginator when where is ordering and subquery with count()

SimpleEntity - is just an entity with id and name columns.

[mrkrstphr]

@zeroedin-bill it looks like our issue is cases that use table inheritance with an inheritance type of JOINED.

For a test case, should I put that in PaginatorTest or in Functional\Ticket somewhere?

[billschaller]

Since most of PaginationTest is new, and it hasn't been merged, please put
it into PaginationTest - I'll merge it into my PR branch.

Thanks!
On Mar 30, 2015 8:52 AM, "Kristopher Wilson" [email protected]
wrote:

@zeroedin-bill https://github.com/zeroedin-bill it looks like our issue
is cases that use table inheritance with an inheritance type of JOINED.

For a test case, should I put that in PaginatorTest or in
Functional\Ticket somewhere?

—
Reply to this email directly or view it on GitHub
#1353 (comment).

[billschaller]

@antanas-arvasevicius - fixed and added a testcase, thanks

[mrkrstphr]

@zeroedin-bill here is a failing test case: https://github.com/doctrine/doctrine2/compare/master...mrkrstphr:paginator-test-for-joined-inheritance?expand=1

[billschaller]

Great, thanks :)

Bill Schaller
Software and Support Engineer
Zeroed-In Technologies LLC
m: +1.336.601.0632 o: +1.410.242.6611 x2122
w: *http://www.zeroedin.com http://www.zeroedin.com/ *

On Mon, Mar 30, 2015 at 11:19 AM, Kristopher Wilson <
[email protected]> wrote:

@zeroedin-bill https://github.com/zeroedin-bill here is a failing test
case:
https://github.com/doctrine/doctrine2/compare/master...mrkrstphr:paginator-test-for-joined-inheritance?expand=1

—
Reply to this email directly or view it on GitHub
#1353 (comment).

[billschaller]

@mrkrstphr - merged and fixed.

[stof]

please don't add the underscore prefix. We are getting rid of it, not adding new ones

[billschaller]

@stof - I actually ran into some really strange behavior with this. These properties were hiding the private ones from the parent class, but when I overrode walkSelectStatement, I was getting a notice saying that these private variables were undefined. It was really odd. I'll try backing the underscores out and see if I can reproduce.

Ideally these variables should just be protected on SqlWalker, but I didn't want to push changes too far out from LimitSubqueryOutputWalker.

[billschaller]

Of course, when I try to reproduce, the error is gone...

[billschaller]

@stof Please see http://www.doctrine-project.org/jira/browse/DDC-3688 - this is the issue I was talking about... Any ideas? Reference: #1220. This user is trying to use FOSUserBundle with the new code and is seeing this issue and others.

[stof]

should be typehinted

[billschaller]

@stof resolved comments, thanks

[antanas-arvasevicius]

@zeroedin-bill thank you very much!

[mrkrstphr]

@zeroedin-bill found another issue: billschaller#1

[mrkrstphr]

@zeroedin-bill commenting out this function call (https://github.com/zeroedin-bill/doctrine2/blob/paginator-fixes-take3/lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php#L424) seems to fix it, and doesn't cause any other errors. Is this call necessary?

[billschaller]

@mrkrstphr fixed, thanks :)

[mrkrstphr]

Our test suite now passes with this branch. Thanks!

[billschaller]

@mrkrstphr Thanks for your help and testing 👍

[Ocramius]

Merged into master at 82230cc - now tagging 2.5.0-RC2