Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft][Please review] Make Paginator use simpler queries when there are no sql joins used (#8278) #11595

Draft
wants to merge 10 commits into
base: 3.3.x
Choose a base branch
from
Draft
69 changes: 63 additions & 6 deletions src/Tools/Pagination/Paginator.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Doctrine\ORM\Internal\SQLResultCasing;
use Doctrine\ORM\NoResultException;
use Doctrine\ORM\Query;
use Doctrine\ORM\Query\Expr\Join;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\Query\Parser;
use Doctrine\ORM\Query\ResultSetMapping;
Expand All @@ -21,6 +22,7 @@
use function array_map;
use function array_sum;
use function assert;
use function count;
use function is_string;

/**
Expand All @@ -38,6 +40,16 @@
private readonly Query $query;
private bool|null $useOutputWalkers = null;
private int|null $count = null;
/**
* @var bool The auto-detection of queries style was added a lot later to this class, and this
d-ph marked this conversation as resolved.
Show resolved Hide resolved
* class historically was by default using the more complex queries style, which means that
* the simple queries style is potentially very under-tested in production systems. The purpose
* of this variable is to not introduce breaking changes until an impression is developed that
* the simple queries style has been battle-tested enough.
*/
private bool $queryStyleAutoDetectionEnabled = false;
/** @var bool|null Null means "undetermined". */
private bool|null $queryHasHavingClause = null;

/** @param bool $fetchJoinCollection Whether the query joins a collection (true by default). */
public function __construct(
Expand All @@ -51,6 +63,29 @@
$this->query = $query;
}

/**
* Create an instance of Paginator with auto-detection of whether the provided
* query is suitable for simple (and fast) pagination queries, or whether a complex
* set of pagination queries has to be used.
*/
public static function newWithAutoDetection(QueryBuilder $queryBuilder): self
{
/** @var array<string, Join[]> $joinsPerRootAlias */
$joinsPerRootAlias = $queryBuilder->getDQLPart('join');
d-ph marked this conversation as resolved.
Show resolved Hide resolved

// For now, only check whether there are any sql joins present in the query. Note,
d-ph marked this conversation as resolved.
Show resolved Hide resolved
// however, that it is doable to detect a presence of only *ToOne joins via the access to
// joined entity classes' metadata (see: QueryBuilder::getEntityManager()->getClassMetadata(className)).
d-ph marked this conversation as resolved.
Show resolved Hide resolved
$sqlJoinsPresent = count($joinsPerRootAlias) > 0;

$paginator = new self($queryBuilder, $sqlJoinsPresent);

$paginator->queryStyleAutoDetectionEnabled = true;
$paginator->queryHasHavingClause = $queryBuilder->getDQLPart('having') !== null;

return $paginator;
}

/**
* Returns the query.
*/
Expand Down Expand Up @@ -171,13 +206,25 @@
/**
* Determines whether to use an output walker for the query.
*/
private function useOutputWalker(Query $query): bool
private function useOutputWalker(Query $query, bool $forCountQuery = false): bool
{
if ($this->useOutputWalkers === null) {
return (bool) $query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER) === false;
if ($this->useOutputWalkers !== null) {
return $this->useOutputWalkers;
}

return $this->useOutputWalkers;
// When a custom output walker already present, then do not use the Paginator's.
if ($query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER) !== false) {
return false;
}

// When not joining onto *ToMany relations, then do not use the more complex CountOutputWalker.
return ! (
$forCountQuery
d-ph marked this conversation as resolved.
Show resolved Hide resolved
&& $this->queryStyleAutoDetectionEnabled
&& ! $this->fetchJoinCollection
// CountWalker doesn't support the "having" clause, while CountOutputWalker does.
&& $this->queryHasHavingClause === false
);
}

/**
Expand Down Expand Up @@ -205,10 +252,20 @@
$countQuery = $this->cloneQuery($this->query);

if (! $countQuery->hasHint(CountWalker::HINT_DISTINCT)) {
$countQuery->setHint(CountWalker::HINT_DISTINCT, true);
$hintDistinctDefaultTrue = true;

// When not joining onto *ToMany relations, then use a simpler COUNT query in the CountWalker.
d-ph marked this conversation as resolved.
Show resolved Hide resolved
if (
$this->queryStyleAutoDetectionEnabled
&& ! $this->fetchJoinCollection
) {
$hintDistinctDefaultTrue = false;

Check warning on line 262 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L262

Added line #L262 was not covered by tests
}

$countQuery->setHint(CountWalker::HINT_DISTINCT, $hintDistinctDefaultTrue);
}

if ($this->useOutputWalker($countQuery)) {
if ($this->useOutputWalker($countQuery, forCountQuery: true)) {
$platform = $countQuery->getEntityManager()->getConnection()->getDatabasePlatform(); // law of demeter win

$rsm = new ResultSetMapping();
Expand Down
14 changes: 14 additions & 0 deletions tests/Tests/ORM/Tools/Pagination/PaginatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
use Doctrine\ORM\Internal\Hydration\AbstractHydrator;
use Doctrine\ORM\Query;
use Doctrine\ORM\Query\QueryException;
use Doctrine\ORM\QueryBuilder;
use Doctrine\ORM\Tools\Pagination\Paginator;
use Doctrine\Tests\Models\CMS\CmsUser;
use Doctrine\Tests\OrmTestCase;
use PHPUnit\Framework\MockObject\MockObject;

Expand Down Expand Up @@ -110,6 +112,18 @@ public function testgetIteratorDoesCareAboutExtraParametersWithoutOutputWalkersW
$this->createPaginatorWithExtraParametersWithoutOutputWalkers([[10]])->getIterator();
}

/** @todo-PR-11595 FINISH TESTS */
public function testNewWithAutoDetection(): void
{
$queryBuilder = new QueryBuilder($this->em);
$queryBuilder->select('u');
$queryBuilder->from(CmsUser::class, 'u');

$paginator = Paginator::newWithAutoDetection($queryBuilder);

$this->assertFalse($paginator->getFetchJoinCollection());
}

/** @param int[][] $willReturnRows */
private function createPaginatorWithExtraParametersWithoutOutputWalkers(array $willReturnRows): Paginator
{
Expand Down
Loading