Pagination using SQL walkers #298

Merged
merged 8 commits into from Mar 12, 2012

Projects

None yet

5 participants

@sandermarechal
Contributor

A CountSqlWalker and LimitSubquerySqlWalker have been implemented. By
default the Paginator will use these SQL walkers. When a query already
uses custom SQL walkers, the Paginator will fall back to the existing
TreeWalker implementations. Improvements:

  • Support for more complex DQL queries using named mixed results with
    GROUP BY and HAVING. For example:

    SELECT g, u, COUNT(u.id) AS userCount
    FROM Entity\Group g LEFT JOIN g.users u
    GROUP BY g.id
    HAVING userCount > 0

  • Support for entities with composite primary keys in the CountSqlWalker
    and LimitSubquerySqlWalker. Only the WhereInWalker still needs to be
    updated for full composite primary key support. But someone smarter
    than me needs to look at that and figure out how to build a WHERE IN
    query that can select rows based on multiple columns.

@sandermarechal sandermarechal Pagination using SQL walkers
A CountSqlWalker and LimitSubquerySqlWalker have been implemented. By
default the Paginator will use these SQL walkers. When a query already
uses custom SQL walkers, the Paginator will fall back to the existing
TreeWalker implementations. Improvements:

* Support for more complex DQL queries using named mixed results with
  GROUP BY and HAVING. For example:

  SELECT g, u, COUNT(u.id) AS userCount
      FROM Entity\Group g LEFT JOIN g.users u
      GROUP BY g.id
      HAVING userCount > 0

* Support for entities with composite primary keys in the CountSqlWalker
  and LimitSubquerySqlWalker. Only the WhereInWalker still needs to be
  updated for full composite primary key support. But someone smarter
  than me needs to look at that and figure out how to build a WHERE IN
  query that can select rows based on multiple columns.
edd5d14
@asm89
Member
asm89 commented Mar 6, 2012

This is an improvement on whiteoctober/Pagerfanta#46 if I'm right?

@stof stof commented on an outdated diff Mar 6, 2012
tests/Doctrine/Tests/ORM/Functional/PaginationTest.php
@@ -39,15 +40,27 @@ public function testCountWithFetchJoin()
$query = $this->_em->createQuery($dql);
$paginator = new Paginator($query);
+ $paginator->setUseSqlWalkers(false);
$this->assertEquals(3, count($paginator));
@stof
stof Mar 6, 2012 Member

for all queries that are supported by both implementations, please use a dataProvider to run the test with both of them

@stof stof and 1 other commented on an outdated diff Mar 6, 2012
tests/Doctrine/Tests/ORM/Functional/PaginationTest.php
+ public function testDetectSqlWalker()
+ {
+ // This query works using the SQL walkers but causes an exception using the TreeWalker
+ $dql = "SELECT g, COUNT(u.id) AS userCount FROM Doctrine\Tests\Models\CMS\CmsGroup g LEFT JOIN g.users u GROUP BY g.id HAVING userCount > 0";
+ $query = $this->_em->createQuery($dql);
+
+ // If the Paginator detects the custom SQL walker it should fall back to using the
+ // Tree walkers for pagination, which leads to an exception. If the query works, the SQL walkers were used
+ $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Query\SqlWalker');
+ $paginator = new Paginator($query);
+
+ try {
+ count($paginator);
+ $this->fail('Paginator did not detect custom SQL walker');
+ } catch (\PHPUnit_Framework_Error_Notice $e) {
+ $this->assertEquals('Undefined index: userCount', $e->getMessage());
@stof
stof Mar 6, 2012 Member

WTF ? You should never catch the notices and even less assert that a Notice occurred as the code should never throw a notice.

@sandermarechal
sandermarechal Mar 6, 2012 Contributor

The existing Paginator code that uses the TreeWalkers already throws a notice when you feed it a query it doesn't understand.

Would it be acceptable if I fixed that bug in the TreeWalker so it throws a \RuntimeException instead of a Notice, then use that exception to detect if the correct walker has been chosen in this unittest?

A bigger problem might be how to make the TreeWalker reliable detect when it is fed a query it cannot handle. It already detects queries that have multiple FROM clauses, and I could add the code to detect mixed results. But you could probably come up with many queries that would still cause trouble form the TreeWalker and have it generate notices. That's why I cane up with the SqlWalkers in the first place. More robust :-)

@stof
stof Mar 6, 2012 Member

well, if it is a case not supported by the TreeWalker, it should indeed throw an exception instead of a notice

@stof stof commented on an outdated diff Mar 6, 2012
...ine/Tests/ORM/Tools/Pagination/CountSqlWalkerTest.php
@@ -0,0 +1,48 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Tools\Pagination;
+
+use Doctrine\ORM\Query;
+use Doctrine\ORM\Tools\Pagination\CountWalker;
+
+class CountSqlWalkerTest extends PaginationTestCase
+{
+ public function testCountQuery()
+ {
+ $query = $this->entityManager->createQuery(
+ 'SELECT p, c, a FROM Doctrine\Tests\ORM\Tools\Pagination\BlogPost p JOIN p.category c JOIN p.author a');
@stof
stof Mar 6, 2012 Member

the indentation should only use 4 spaces per level. You have too many here

@sandermarechal
Contributor

@asm89: Yes. Both whiteoctober/Pagerfanta#46 and beberlei/DoctrineExtensions#59. And I implemented the suggestion from the PagerFanta PR to have the SqlWalkers as an alternative of the TreeWalkers instead of just replacing them.

@sandermarechal
Contributor

I fixed all the issues that @stof pointed out. I took the liberty of fixing the existing incorrect indentation in the Paginate unittests as well.

@beberlei
Member
beberlei commented Mar 7, 2012

Very good, i like this one and will merge it when i had time to play with it a bit.

@stof stof and 1 other commented on an outdated diff Mar 7, 2012
lib/Doctrine/ORM/Tools/Pagination/Paginator.php
use Doctrine\ORM\Tools\Pagination\CountWalker;
+use Doctrine\ORM\Tools\Pagination\CountSqlWalker;
@stof
stof Mar 7, 2012 Member

all these use statements are useless as the classes are in the same namespace

@sandermarechal
sandermarechal Mar 7, 2012 Contributor

What are the Doctrine coding guidelines on use statements? I did this to keep in line with the rest of the file. Related: I've seen other parts of Doctrine use a single multiline use statement instead of separate statements like this.

@stof stof commented on an outdated diff Mar 7, 2012
lib/Doctrine/ORM/Tools/Pagination/Paginator.php
@@ -103,7 +133,16 @@ public function count()
$countQuery->setHint(CountWalker::HINT_DISTINCT, true);
}
- $countQuery->setHint(Query::HINT_CUSTOM_TREE_WALKERS, array('Doctrine\ORM\Tools\Pagination\CountWalker'));
+ if ($this->useSqlWalker($countQuery)) {
+ $rsm = new ResultSetMapping();
+ $rsm->addScalarResult('_dctrn_count', 'count');
+
+ $countQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\CountSqlWalker');
+ $countQuery->setResultSetMapping($rsm);
+ } else {
+ $countQuery->setHint(Query::HINT_CUSTOM_TREE_WALKERS, array('Doctrine\ORM\Tools\Pagination\CountWalker'));
@stof
stof Mar 7, 2012 Member

this should be improved to merge the CountWalker with the existing tree walkers instead of overwriting the array totally. @beberlei thoughts ?

@stof stof and 1 other commented on an outdated diff Mar 7, 2012
tests/Doctrine/Tests/ORM/Functional/PaginationTest.php
@@ -84,6 +114,40 @@ public function testIterateWithFetchJoin()
$this->assertEquals(3, count($data));
}
+ public function testIterateComplexWithSqlWalker()
+ {
+ $dql = "SELECT g, COUNT(u.id) AS userCount FROM Doctrine\Tests\Models\CMS\CmsGroup g LEFT JOIN g.users u GROUP BY g.id HAVING userCount > 0";
+ $query = $this->_em->createQuery($dql);
+
+ $paginator = new Paginator($query);
+ $paginator->setUseSqlWalkers(true);
+
+ $data = array();
+ foreach ($paginator as $user) {
+ $data[] = $user;
+ }
+ $this->assertEquals(9, count($data));
@stof
stof Mar 7, 2012 Member

$this->assertCount(9, $paginator->getPaginator()) could do the job without the need to do the foreach loop in each test (ArrayIterator implements Countable)

@sandermarechal
sandermarechal Mar 7, 2012 Contributor

I assume you mean ->getIterator(), not ->getPaginator()?

@stof
stof Mar 7, 2012 Member

yeah sorry

@sandermarechal
Contributor

I've been thinking, maybe some thought should be given to the names (and namespaces?) of all those walkers. I don't know if the existing walkers are part of the API and should remain the same. Perhaps it would also be better to name the new *SqlWalker classes *OutputWalker instead. Thoughts?

@beberlei
Member

They are not public API, they are hidden behind the Paginator Iterator. But i would name them "CountWalker" + "CountOutputWalker" and so on.

@sandermarechal
Contributor

I have renamed *SqlWalker to *OutputWalker.

@beberlei beberlei merged commit 3788d0e into doctrine:master Mar 12, 2012
@beberlei
Member

Merged, thank you for providing this awesome extension :)

@l3pp4rd
l3pp4rd commented Mar 12, 2012

yea, great work!

@beberlei
Member

It seems this broke PGSQL Pagination tests: http://travis-ci.org/#!/doctrine/doctrine2/jobs/845061

@l3pp4rd
l3pp4rd commented Mar 12, 2012

yes, psql requires all components used in having order by and so on, to be selected in the select clause.

@sandermarechal
Contributor

I don't have psql, nor do I have much experience using it. This is the query that is generated. sclr2 is in the inner select clause. If you can tell me what's wrong with it, I can probably fix it.

SELECT COUNT(*) AS _dctrn_count FROM (
    SELECT DISTINCT id0 FROM (
        SELECT c0_.id AS id0, c0_.name AS name1, COUNT(c1_.id) AS sclr2
        FROM cms_groups c0_ LEFT JOIN cms_users_groups c2_ ON c0_.id = c2_.group_id
        LEFT JOIN cms_users c1_ ON c1_.id = c2_.user_id
        GROUP BY c0_.id
        HAVING sclr2 > 0
    ) AS _dctrn_result
) AS _dctrn_table
@l3pp4rd
l3pp4rd commented Mar 12, 2012

Hey, COUNT(c1_.id) AS sclr2 is processed after HAVING sclr2 > 0 so if you had for instance HAVING COUNT(c1_.id) > 0 it should work

@sandermarechal
Contributor

I think I fixed it. I can't test it with psql myself, but from what @l3pp4rd says it should work. I tested it with MySQL and SQLite. I created a new PR for it: doctrine/doctrine2#301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment