From 3822686bf8d065495d4e8813ba082b7103ec4e1f Mon Sep 17 00:00:00 2001 From: d-ph Date: Thu, 5 Sep 2024 19:02:03 +0100 Subject: [PATCH 1/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 96 +++++++++++++++++++++++++++--- 1 file changed, 87 insertions(+), 9 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index db1b34db715..c5dee9bcfbf 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -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; @@ -21,6 +22,7 @@ use function array_map; use function array_sum; use function assert; +use function count; use function is_string; /** @@ -38,14 +40,43 @@ class Paginator implements Countable, IteratorAggregate private readonly Query $query; private bool|null $useOutputWalkers = null; private int|null $count = null; + private bool $fetchJoinCollection; + /** + * @var bool The auto-detection of queries style was added a lot later to this class, and this + * 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). */ + /** + * @param bool|null $fetchJoinCollection Whether the query joins a collection (true by default). Set + * to null to enable auto-detection of this parameter, however note that the auto-detection requires + * a QueryBuilder to be provided to Paginator, otherwise this parameter goes back to its default value. + * Also, for now, when the auto-detection of this parameter is enabled, then auto-detection + * of $useOutputWalkers and of the CountWalker::HINT_DISTINCT is also enabled. + */ public function __construct( Query|QueryBuilder $query, - private readonly bool $fetchJoinCollection = true, + bool|null $fetchJoinCollection = true, ) { + if ($fetchJoinCollection === null) { + $fetchJoinCollection = $this->autoDetectFetchJoinCollection($query); + } + + if ($fetchJoinCollection === null) { + $this->fetchJoinCollection = true; + } else { + $this->fetchJoinCollection = $fetchJoinCollection; + $this->queryStyleAutoDetectionEnabled = true; + } + if ($query instanceof QueryBuilder) { - $query = $query->getQuery(); + $this->queryHasHavingClause = $query->getDQLPart('having') !== null; + $query = $query->getQuery(); } $this->query = $query; @@ -154,6 +185,27 @@ public function getIterator(): Traversable return new ArrayIterator($result); } + /** @return bool|null Null means that auto-detection could not be carried out. */ + private function autoDetectFetchJoinCollection(Query|QueryBuilder $query): bool|null + { + // For now, only working with QueryBuilder is supported. + if (! $query instanceof QueryBuilder) { + return null; + } + + /** @var array $joinsPerRootAlias */ + $joinsPerRootAlias = $query->getDQLPart('join'); + + if (count($joinsPerRootAlias) === 0) { + return false; + } + + // For now, do not try to investigate what kind of joins are used. It is, however, doable + // to detect a presence of only *ToOne joins via the access to join entity classes' + // metadata (see: QueryBuilder::getEntityManager()->getClassMetadata(className)). + return true; + } + private function cloneQuery(Query $query): Query { $cloneQuery = clone $query; @@ -171,13 +223,29 @@ private function cloneQuery(Query $query): Query /** * 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. + if ( + $forCountQuery + && $this->queryStyleAutoDetectionEnabled + && $this->fetchJoinCollection === false + // CountWalker doesn't support the "having" clause, while CountOutputWalker does. + && $this->queryHasHavingClause === false + ) { + return false; + } + + return true; } /** @@ -205,10 +273,20 @@ private function getCountQuery(): Query $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. + if ( + $this->queryStyleAutoDetectionEnabled + && $this->fetchJoinCollection === false + ) { + $hintDistinctDefaultTrue = false; + } + + $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(); From 9dc93b90d21224b60953cf528a987b4daf36bda0 Mon Sep 17 00:00:00 2001 From: d-ph Date: Thu, 5 Sep 2024 19:31:02 +0100 Subject: [PATCH 2/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index c5dee9bcfbf..fd99107348f 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -196,14 +196,10 @@ private function autoDetectFetchJoinCollection(Query|QueryBuilder $query): bool| /** @var array $joinsPerRootAlias */ $joinsPerRootAlias = $query->getDQLPart('join'); - if (count($joinsPerRootAlias) === 0) { - return false; - } - // For now, do not try to investigate what kind of joins are used. It is, however, doable - // to detect a presence of only *ToOne joins via the access to join entity classes' + // to detect a presence of only *ToOne joins via the access to joined entity classes' // metadata (see: QueryBuilder::getEntityManager()->getClassMetadata(className)). - return true; + return count($joinsPerRootAlias) > 0; } private function cloneQuery(Query $query): Query @@ -235,10 +231,11 @@ private function useOutputWalker(Query $query, bool $forCountQuery = false): boo } // When not joining onto *ToMany relations, then do not use the more complex CountOutputWalker. + // phpcs:ignore SlevomatCodingStandard.ControlStructures.UselessIfConditionWithReturn.UselessIfCondition if ( $forCountQuery && $this->queryStyleAutoDetectionEnabled - && $this->fetchJoinCollection === false + && ! $this->fetchJoinCollection === false // CountWalker doesn't support the "having" clause, while CountOutputWalker does. && $this->queryHasHavingClause === false ) { From c9896b55d5676b623b07e82d17b2f3e3c39b24d9 Mon Sep 17 00:00:00 2001 From: d-ph Date: Fri, 6 Sep 2024 16:57:18 +0100 Subject: [PATCH 3/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 78 +++++++------------ .../ORM/Tools/Pagination/PaginatorTest.php | 14 ++++ 2 files changed, 44 insertions(+), 48 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index fd99107348f..a392072a98e 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -40,7 +40,6 @@ class Paginator implements Countable, IteratorAggregate private readonly Query $query; private bool|null $useOutputWalkers = null; private int|null $count = null; - private bool $fetchJoinCollection; /** * @var bool The auto-detection of queries style was added a lot later to this class, and this * class historically was by default using the more complex queries style, which means that @@ -52,36 +51,41 @@ class Paginator implements Countable, IteratorAggregate /** @var bool|null Null means "undetermined". */ private bool|null $queryHasHavingClause = null; - /** - * @param bool|null $fetchJoinCollection Whether the query joins a collection (true by default). Set - * to null to enable auto-detection of this parameter, however note that the auto-detection requires - * a QueryBuilder to be provided to Paginator, otherwise this parameter goes back to its default value. - * Also, for now, when the auto-detection of this parameter is enabled, then auto-detection - * of $useOutputWalkers and of the CountWalker::HINT_DISTINCT is also enabled. - */ + /** @param bool $fetchJoinCollection Whether the query joins a collection (true by default). */ public function __construct( Query|QueryBuilder $query, - bool|null $fetchJoinCollection = true, + private readonly bool $fetchJoinCollection = true, ) { - if ($fetchJoinCollection === null) { - $fetchJoinCollection = $this->autoDetectFetchJoinCollection($query); - } - - if ($fetchJoinCollection === null) { - $this->fetchJoinCollection = true; - } else { - $this->fetchJoinCollection = $fetchJoinCollection; - $this->queryStyleAutoDetectionEnabled = true; - } - if ($query instanceof QueryBuilder) { - $this->queryHasHavingClause = $query->getDQLPart('having') !== null; - $query = $query->getQuery(); + $query = $query->getQuery(); } $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 $joinsPerRootAlias */ + $joinsPerRootAlias = $queryBuilder->getDQLPart('join'); + + // For now, only check whether there are any sql joins present in the query. Note, + // 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)). + $sqlJoinsPresent = count($joinsPerRootAlias) > 0; + + $paginator = new self($queryBuilder, $sqlJoinsPresent); + + $paginator->queryStyleAutoDetectionEnabled = true; + $paginator->queryHasHavingClause = $queryBuilder->getDQLPart('having') !== null; + + return $paginator; + } + /** * Returns the query. */ @@ -185,23 +189,6 @@ public function getIterator(): Traversable return new ArrayIterator($result); } - /** @return bool|null Null means that auto-detection could not be carried out. */ - private function autoDetectFetchJoinCollection(Query|QueryBuilder $query): bool|null - { - // For now, only working with QueryBuilder is supported. - if (! $query instanceof QueryBuilder) { - return null; - } - - /** @var array $joinsPerRootAlias */ - $joinsPerRootAlias = $query->getDQLPart('join'); - - // For now, do not try to investigate what kind of joins are used. It is, however, doable - // to detect a presence of only *ToOne joins via the access to joined entity classes' - // metadata (see: QueryBuilder::getEntityManager()->getClassMetadata(className)). - return count($joinsPerRootAlias) > 0; - } - private function cloneQuery(Query $query): Query { $cloneQuery = clone $query; @@ -231,18 +218,13 @@ private function useOutputWalker(Query $query, bool $forCountQuery = false): boo } // When not joining onto *ToMany relations, then do not use the more complex CountOutputWalker. - // phpcs:ignore SlevomatCodingStandard.ControlStructures.UselessIfConditionWithReturn.UselessIfCondition - if ( + return ! ( $forCountQuery && $this->queryStyleAutoDetectionEnabled - && ! $this->fetchJoinCollection === false + && ! $this->fetchJoinCollection // CountWalker doesn't support the "having" clause, while CountOutputWalker does. && $this->queryHasHavingClause === false - ) { - return false; - } - - return true; + ); } /** @@ -275,7 +257,7 @@ private function getCountQuery(): Query // When not joining onto *ToMany relations, then use a simpler COUNT query in the CountWalker. if ( $this->queryStyleAutoDetectionEnabled - && $this->fetchJoinCollection === false + && ! $this->fetchJoinCollection ) { $hintDistinctDefaultTrue = false; } diff --git a/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php b/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php index 41e3981e3b2..da43c2e5fac 100644 --- a/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php +++ b/tests/Tests/ORM/Tools/Pagination/PaginatorTest.php @@ -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; @@ -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 { From 7155bfe4182de4c22bb1be6a02c6d95279df9c28 Mon Sep 17 00:00:00 2001 From: d-ph <4347095+d-ph@users.noreply.github.com> Date: Thu, 31 Oct 2024 18:13:59 +0000 Subject: [PATCH 4/8] Update src/Tools/Pagination/Paginator.php Co-authored-by: Christophe Coevoet --- src/Tools/Pagination/Paginator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index a392072a98e..61c0714a936 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -41,7 +41,7 @@ class Paginator implements Countable, IteratorAggregate 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 + * The auto-detection of queries style was added a lot later to this class, and this * 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 From f88bad98b77203762e540eb8fb4497748b9e98bb Mon Sep 17 00:00:00 2001 From: d-ph Date: Thu, 31 Oct 2024 18:49:03 +0000 Subject: [PATCH 5/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 254 +++++++++++++++++++++++++---- 1 file changed, 222 insertions(+), 32 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index 61c0714a936..c4a13d82983 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -7,14 +7,20 @@ use ArrayIterator; use Countable; use Doctrine\Common\Collections\Collection; +use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Internal\SQLResultCasing; +use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\NoResultException; use Doctrine\ORM\Query; -use Doctrine\ORM\Query\Expr\Join; +use Doctrine\ORM\Query\AST\Join; +use Doctrine\ORM\Query\AST\JoinAssociationDeclaration; +use Doctrine\ORM\Query\AST\Node; +use Doctrine\ORM\Query\AST\SelectExpression; use Doctrine\ORM\Query\Parameter; use Doctrine\ORM\Query\Parser; use Doctrine\ORM\Query\ResultSetMapping; use Doctrine\ORM\QueryBuilder; +use Doctrine\Persistence\Mapping\MappingException; use IteratorAggregate; use Traversable; @@ -38,7 +44,8 @@ class Paginator implements Countable, IteratorAggregate public const HINT_ENABLE_DISTINCT = 'paginator.distinct.enable'; private readonly Query $query; - private bool|null $useOutputWalkers = null; + private bool|null $useResultQueryOutputWalker = null; + private bool|null $useCountQueryOutputWalker = null; private int|null $count = null; /** * The auto-detection of queries style was added a lot later to this class, and this @@ -48,13 +55,15 @@ class Paginator implements Countable, IteratorAggregate * the simple queries style has been battle-tested enough. */ private bool $queryStyleAutoDetectionEnabled = false; - /** @var bool|null Null means "undetermined". */ - private bool|null $queryHasHavingClause = null; + private bool $queryCouldHaveToManyJoins = true; - /** @param bool $fetchJoinCollection Whether the query joins a collection (true by default). */ + /** + * @param bool $queryCouldProduceDuplicates Whether the query could produce partially duplicated records. One case + * when it does is when it joins a collection. + */ public function __construct( Query|QueryBuilder $query, - private readonly bool $fetchJoinCollection = true, + private readonly bool $queryCouldProduceDuplicates = true, ) { if ($query instanceof QueryBuilder) { $query = $query->getQuery(); @@ -68,24 +77,159 @@ public function __construct( * 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 + public static function newWithAutoDetection(Query|QueryBuilder $query): self { - /** @var array $joinsPerRootAlias */ - $joinsPerRootAlias = $queryBuilder->getDQLPart('join'); + if ($query instanceof QueryBuilder) { + $query = $query->getQuery(); + } - // For now, only check whether there are any sql joins present in the query. Note, - // 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)). - $sqlJoinsPresent = count($joinsPerRootAlias) > 0; + $queryAST = $query->getAST(); + [ + 'queryCouldProduceDuplicates' => $queryCouldProduceDuplicates, + 'queryCouldHaveToManyJoins' => $queryCouldHaveToManyJoins, + ] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST); - $paginator = new self($queryBuilder, $sqlJoinsPresent); + $paginator = new self($query, $queryCouldProduceDuplicates); $paginator->queryStyleAutoDetectionEnabled = true; - $paginator->queryHasHavingClause = $queryBuilder->getDQLPart('having') !== null; + $paginator->queryCouldHaveToManyJoins = $queryCouldHaveToManyJoins; + $paginator->useCountQueryOutputWalker = $queryAST->havingClause !== null; return $paginator; } + /** + * @return array{ + * queryCouldProduceDuplicates: bool, + * queryCouldHaveToManyJoins: bool, + * } + */ + private static function autoDetectQueryFeatures(EntityManagerInterface $entityManager, Node $queryAST): array + { + $queryFeatures = [ + 'queryCouldProduceDuplicates' => true, + 'queryCouldHaveToManyJoins' => true, + ]; + + if (! $queryAST instanceof Query\AST\SelectStatement) { + return $queryFeatures; + } + + $from = $queryAST->fromClause->identificationVariableDeclarations; + if (count($from) > 1) { + return $queryFeatures; + } + + $fromRoot = reset($from); + if (! $fromRoot instanceof Query\AST\IdentificationVariableDeclaration) { + return $queryFeatures; + } + + if (! $fromRoot->rangeVariableDeclaration) { + return $queryFeatures; + } + + $rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable; + $rootClassName = $fromRoot->rangeVariableDeclaration->abstractSchemaName; + + $aliasesToClassNameOrClassMetadataMap = []; + $aliasesToClassNameOrClassMetadataMap[$rootAlias] = $rootClassName; + $toManyJoinsAliases = []; + + // Check the Joins list. + foreach ($fromRoot->joins as $join) { + if (! $join instanceof Join || ! $join->joinAssociationDeclaration instanceof JoinAssociationDeclaration) { + return $queryFeatures; + } + + $joinParentAlias = $join->joinAssociationDeclaration->joinAssociationPathExpression->identificationVariable; + $joinParentFieldName = $join->joinAssociationDeclaration->joinAssociationPathExpression->associationField; + $joinAlias = $join->joinAssociationDeclaration->aliasIdentificationVariable; + + // Every Join descending from a ToMany Join is "in principle" also a ToMany Join + if (in_array($joinParentAlias, $toManyJoinsAliases, true)) { + $toManyJoinsAliases[] = $joinAlias; + + continue; + } + + $parentClassMetadata = $aliasesToClassNameOrClassMetadataMap[$joinParentAlias] ?? null; + if (! $parentClassMetadata) { + return $queryFeatures; + } + + // Load entity class metadata. + if (is_string($parentClassMetadata)) { + try { + $parentClassMetadata = $aliasesToClassNameOrClassMetadataMap[$joinParentAlias] + = $entityManager->getClassMetadata($parentClassMetadata); + } catch (MappingException) { + return $queryFeatures; + } + } + + $parentJoinAssociationMapping = $parentClassMetadata->associationMappings[$joinParentFieldName] ?? null; + if (! $parentJoinAssociationMapping) { + return $queryFeatures; + } + + $aliasesToClassNameOrClassMetadataMap[$joinAlias] = $parentJoinAssociationMapping['targetEntity']; + + if (! ($parentJoinAssociationMapping['type'] & ClassMetadata::TO_MANY)) { + continue; + } + + // The Join is a ToMany Join. + $toManyJoinsAliases[] = $joinAlias; + } + + $queryFeatures['queryCouldHaveToManyJoins'] = count($toManyJoinsAliases) > 0; + + // Check the Select list. + foreach ($queryAST->selectClause->selectExpressions as $selectExpression) { + if (! $selectExpression instanceof SelectExpression) { + return $queryFeatures; + } + + // Must not use any of the ToMany aliases + if (is_string($selectExpression->expression)) { + foreach ($toManyJoinsAliases as $toManyJoinAlias) { + if ($selectExpression->expression === $toManyJoinAlias + || str_starts_with($selectExpression->expression, "{$toManyJoinAlias}.") + ) { + return $queryFeatures; + } + } + } + + // If it's a function, then it has to be one from the following list. Reason: in some databases, + // there are functions that "generate rows". + if ($selectExpression->expression instanceof Query\AST\Functions\FunctionNode + && !in_array($selectExpression->expression::class, [ + Query\AST\Functions\CountFunction::class, + Query\AST\Functions\AvgFunction::class, + Query\AST\Functions\SumFunction::class, + Query\AST\Functions\MinFunction::class, + Query\AST\Functions\MaxFunction::class, + ], true) + ) { + return $queryFeatures; + } + } + + // If there are ToMany Joins, then the Select clause has to use the DISTINCT keyword. Note: the foreach + // above also ensures that the ToMany Joins are not in the Select list, which is relevant. + if (count($toManyJoinsAliases) > 0 + && ! $queryAST->selectClause->isDistinct + ) { + return $queryFeatures; + } + + $queryFeatures['queryCouldProduceDuplicates'] = false; + + return $queryFeatures; + } + /** * Returns the query. */ @@ -95,31 +239,80 @@ public function getQuery(): Query } /** - * Returns whether the query joins a collection. + * @deprecated Use ::getQueryCouldProduceDuplicates() instead. * - * @return bool Whether the query joins a collection. + * Returns whether the query joins a collection. */ public function getFetchJoinCollection(): bool { - return $this->fetchJoinCollection; + return $this->queryCouldProduceDuplicates; } /** + * Returns whether the query could produce partially duplicated records. + */ + public function getQueryCouldProduceDuplicates(): bool + { + return $this->queryCouldProduceDuplicates; + } + + /** + * @deprecated Use the individual ::get*OutputWalker() + * * Returns whether the paginator will use an output walker. */ public function getUseOutputWalkers(): bool|null { - return $this->useOutputWalkers; + return $this->getUseResultQueryOutputWalker() && $this->getUseCountQueryOutputWalker(); } /** + * @deprecated Use the individual ::set*OutputWalker() + * * Sets whether the paginator will use an output walker. * * @return $this */ public function setUseOutputWalkers(bool|null $useOutputWalkers): static { - $this->useOutputWalkers = $useOutputWalkers; + $this->setUseResultQueryOutputWalker($useOutputWalkers); + $this->setUseCountQueryOutputWalker($useOutputWalkers); + + return $this; + } + + /** + * Returns whether the paginator will use an output walker for the result query. + */ + public function getUseResultQueryOutputWalker(): bool|null + { + return $this->useResultQueryOutputWalker; + } + + /** + * Sets whether the paginator will use an output walker for the result query. + */ + public function setUseResultQueryOutputWalker(bool|null $useResultQueryOutputWalker): static + { + $this->useResultQueryOutputWalker = $useResultQueryOutputWalker; + + return $this; + } + + /** + * Returns whether the paginator will use an output walker for the count query. + */ + public function getUseCountQueryOutputWalker(): bool|null + { + return $this->useCountQueryOutputWalker; + } + + /** + * Sets whether the paginator will use an output walker for the count query. + */ + public function setUseCountQueryOutputWalker(bool|null $useCountQueryOutputWalker): static + { + $this->useCountQueryOutputWalker = $useCountQueryOutputWalker; return $this; } @@ -147,7 +340,7 @@ public function getIterator(): Traversable $offset = $this->query->getFirstResult(); $length = $this->query->getMaxResults(); - if ($this->fetchJoinCollection && $length !== null) { + if ($this->queryCouldProduceDuplicates && $length !== null) { $subQuery = $this->cloneQuery($this->query); if ($this->useOutputWalker($subQuery)) { @@ -208,8 +401,12 @@ private function cloneQuery(Query $query): Query */ private function useOutputWalker(Query $query, bool $forCountQuery = false): bool { - if ($this->useOutputWalkers !== null) { - return $this->useOutputWalkers; + if ($forCountQuery === false && $this->useResultQueryOutputWalker !== null) { + return $this->useResultQueryOutputWalker; + } + + if ($forCountQuery === true && $this->useCountQueryOutputWalker !== null) { + return $this->useCountQueryOutputWalker; } // When a custom output walker already present, then do not use the Paginator's. @@ -217,14 +414,7 @@ private function useOutputWalker(Query $query, bool $forCountQuery = false): boo return false; } - // When not joining onto *ToMany relations, then do not use the more complex CountOutputWalker. - return ! ( - $forCountQuery - && $this->queryStyleAutoDetectionEnabled - && ! $this->fetchJoinCollection - // CountWalker doesn't support the "having" clause, while CountOutputWalker does. - && $this->queryHasHavingClause === false - ); + return true; } /** @@ -257,7 +447,7 @@ private function getCountQuery(): Query // When not joining onto *ToMany relations, then use a simpler COUNT query in the CountWalker. if ( $this->queryStyleAutoDetectionEnabled - && ! $this->fetchJoinCollection + && ! $this->queryCouldHaveToManyJoins ) { $hintDistinctDefaultTrue = false; } From 585db833fbd4b4929923e5d61f464cd8623b9c71 Mon Sep 17 00:00:00 2001 From: d-ph Date: Thu, 31 Oct 2024 19:07:03 +0000 Subject: [PATCH 6/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 47 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index c4a13d82983..c1136c3cb92 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -29,7 +29,10 @@ use function array_sum; use function assert; use function count; +use function in_array; use function is_string; +use function reset; +use function str_starts_with; /** * The paginator can handle various complex scenarios with DQL. @@ -45,8 +48,8 @@ class Paginator implements Countable, IteratorAggregate private readonly Query $query; private bool|null $useResultQueryOutputWalker = null; - private bool|null $useCountQueryOutputWalker = null; - private int|null $count = null; + private bool|null $useCountQueryOutputWalker = null; + private int|null $count = null; /** * The auto-detection of queries style was added a lot later to this class, and this * class historically was by default using the more complex queries style, which means that @@ -55,7 +58,7 @@ class Paginator implements Countable, IteratorAggregate * the simple queries style has been battle-tested enough. */ private bool $queryStyleAutoDetectionEnabled = false; - private bool $queryCouldHaveToManyJoins = true; + private bool $queryCouldHaveToManyJoins = true; /** * @param bool $queryCouldProduceDuplicates Whether the query could produce partially duplicated records. One case @@ -87,13 +90,14 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self [ 'queryCouldProduceDuplicates' => $queryCouldProduceDuplicates, 'queryCouldHaveToManyJoins' => $queryCouldHaveToManyJoins, - ] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST); + ] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST); $paginator = new self($query, $queryCouldProduceDuplicates); $paginator->queryStyleAutoDetectionEnabled = true; $paginator->queryCouldHaveToManyJoins = $queryCouldHaveToManyJoins; - $paginator->useCountQueryOutputWalker = $queryAST->havingClause !== null; + $paginator->useCountQueryOutputWalker = ! $queryAST instanceof Query\AST\SelectStatement + || $queryAST->havingClause !== null; return $paginator; } @@ -129,12 +133,12 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa return $queryFeatures; } - $rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable; + $rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable; $rootClassName = $fromRoot->rangeVariableDeclaration->abstractSchemaName; - $aliasesToClassNameOrClassMetadataMap = []; + $aliasesToClassNameOrClassMetadataMap = []; $aliasesToClassNameOrClassMetadataMap[$rootAlias] = $rootClassName; - $toManyJoinsAliases = []; + $toManyJoinsAliases = []; // Check the Joins list. foreach ($fromRoot->joins as $join) { @@ -142,9 +146,9 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa return $queryFeatures; } - $joinParentAlias = $join->joinAssociationDeclaration->joinAssociationPathExpression->identificationVariable; + $joinParentAlias = $join->joinAssociationDeclaration->joinAssociationPathExpression->identificationVariable; $joinParentFieldName = $join->joinAssociationDeclaration->joinAssociationPathExpression->associationField; - $joinAlias = $join->joinAssociationDeclaration->aliasIdentificationVariable; + $joinAlias = $join->joinAssociationDeclaration->aliasIdentificationVariable; // Every Join descending from a ToMany Join is "in principle" also a ToMany Join if (in_array($joinParentAlias, $toManyJoinsAliases, true)) { @@ -161,8 +165,8 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa // Load entity class metadata. if (is_string($parentClassMetadata)) { try { - $parentClassMetadata = $aliasesToClassNameOrClassMetadataMap[$joinParentAlias] - = $entityManager->getClassMetadata($parentClassMetadata); + $parentClassMetadata = $entityManager->getClassMetadata($parentClassMetadata); + $aliasesToClassNameOrClassMetadataMap[$joinParentAlias] = $parentClassMetadata; } catch (MappingException) { return $queryFeatures; } @@ -194,8 +198,9 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa // Must not use any of the ToMany aliases if (is_string($selectExpression->expression)) { foreach ($toManyJoinsAliases as $toManyJoinAlias) { - if ($selectExpression->expression === $toManyJoinAlias - || str_starts_with($selectExpression->expression, "{$toManyJoinAlias}.") + if ( + $selectExpression->expression === $toManyJoinAlias + || str_starts_with($selectExpression->expression, $toManyJoinAlias . '.') ) { return $queryFeatures; } @@ -204,8 +209,9 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa // If it's a function, then it has to be one from the following list. Reason: in some databases, // there are functions that "generate rows". - if ($selectExpression->expression instanceof Query\AST\Functions\FunctionNode - && !in_array($selectExpression->expression::class, [ + if ( + $selectExpression->expression instanceof Query\AST\Functions\FunctionNode + && ! in_array($selectExpression->expression::class, [ Query\AST\Functions\CountFunction::class, Query\AST\Functions\AvgFunction::class, Query\AST\Functions\SumFunction::class, @@ -219,7 +225,8 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa // If there are ToMany Joins, then the Select clause has to use the DISTINCT keyword. Note: the foreach // above also ensures that the ToMany Joins are not in the Select list, which is relevant. - if (count($toManyJoinsAliases) > 0 + if ( + count($toManyJoinsAliases) > 0 && ! $queryAST->selectClause->isDistinct ) { return $queryFeatures; @@ -410,11 +417,7 @@ private function useOutputWalker(Query $query, bool $forCountQuery = false): boo } // 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; - } - - return true; + return $query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER) === false; } /** From b2f087d6a16bd0f7f86e732478b272878119f841 Mon Sep 17 00:00:00 2001 From: d-ph Date: Thu, 31 Oct 2024 19:32:57 +0000 Subject: [PATCH 7/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index c1136c3cb92..dbf36228aa7 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -88,6 +88,9 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self $queryAST = $query->getAST(); [ + 'queryHasGroupByClause' => $queryHasGroupByClause, + 'queryHasHavingClause' => $queryHasHavingClause, + 'rootEntityHasSingleIdentifierFieldName' => $rootEntityHasSingleIdentifierFieldName, 'queryCouldProduceDuplicates' => $queryCouldProduceDuplicates, 'queryCouldHaveToManyJoins' => $queryCouldHaveToManyJoins, ] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST); @@ -96,14 +99,19 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self $paginator->queryStyleAutoDetectionEnabled = true; $paginator->queryCouldHaveToManyJoins = $queryCouldHaveToManyJoins; - $paginator->useCountQueryOutputWalker = ! $queryAST instanceof Query\AST\SelectStatement - || $queryAST->havingClause !== null; + // The following is ensuring the conditions for when the CountWalker cannot be used. + $paginator->useCountQueryOutputWalker = $queryHasHavingClause !== false + || $rootEntityHasSingleIdentifierFieldName !== true + || ($queryCouldHaveToManyJoins && $queryHasGroupByClause !== false); return $paginator; } /** * @return array{ + * queryHasGroupByClause: bool|null, + * queryHasHavingClause: bool|null, + * rootEntityHasSingleIdentifierFieldName: bool|null, * queryCouldProduceDuplicates: bool, * queryCouldHaveToManyJoins: bool, * } @@ -111,6 +119,10 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self private static function autoDetectQueryFeatures(EntityManagerInterface $entityManager, Node $queryAST): array { $queryFeatures = [ + // Null means undetermined + 'queryHasGroupByClause' => null, + 'queryHasHavingClause' => null, + 'rootEntityHasSingleIdentifierFieldName' => null, 'queryCouldProduceDuplicates' => true, 'queryCouldHaveToManyJoins' => true, ]; @@ -119,6 +131,9 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa return $queryFeatures; } + $queryFeatures['queryHasGroupByClause'] = $queryAST->groupByClause !== null; + $queryFeatures['queryHasHavingClause'] = $queryAST->havingClause !== null; + $from = $queryAST->fromClause->identificationVariableDeclarations; if (count($from) > 1) { return $queryFeatures; @@ -133,11 +148,18 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa return $queryFeatures; } - $rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable; + $rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable; + try { + $rootClassMetadata = $entityManager->getClassMetadata($fromRoot->rangeVariableDeclaration->abstractSchemaName); + $queryFeatures['rootEntityHasSingleIdentifierFieldName'] = (bool) $rootClassMetadata->getSingleIdentifierFieldName(); + } catch (MappingException) { + return $queryFeatures; + } + $rootClassName = $fromRoot->rangeVariableDeclaration->abstractSchemaName; $aliasesToClassNameOrClassMetadataMap = []; - $aliasesToClassNameOrClassMetadataMap[$rootAlias] = $rootClassName; + $aliasesToClassNameOrClassMetadataMap[$rootAlias] = $rootClassMetadata; $toManyJoinsAliases = []; // Check the Joins list. From c06e100e6ab5f2fc54377998005f82fa87e4dea3 Mon Sep 17 00:00:00 2001 From: d-ph Date: Fri, 1 Nov 2024 17:20:07 +0000 Subject: [PATCH 8/8] Make Paginator use simpler queries when there are no sql joins used (#8278) This is work in progress. --- src/Tools/Pagination/Paginator.php | 36 +++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Tools/Pagination/Paginator.php b/src/Tools/Pagination/Paginator.php index dbf36228aa7..c3cc0a38dbf 100644 --- a/src/Tools/Pagination/Paginator.php +++ b/src/Tools/Pagination/Paginator.php @@ -88,11 +88,11 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self $queryAST = $query->getAST(); [ - 'queryHasGroupByClause' => $queryHasGroupByClause, - 'queryHasHavingClause' => $queryHasHavingClause, + 'hasGroupByClause' => $queryHasGroupByClause, + 'hasHavingClause' => $queryHasHavingClause, 'rootEntityHasSingleIdentifierFieldName' => $rootEntityHasSingleIdentifierFieldName, - 'queryCouldProduceDuplicates' => $queryCouldProduceDuplicates, - 'queryCouldHaveToManyJoins' => $queryCouldHaveToManyJoins, + 'couldProduceDuplicates' => $queryCouldProduceDuplicates, + 'couldHaveToManyJoins' => $queryCouldHaveToManyJoins, ] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST); $paginator = new self($query, $queryCouldProduceDuplicates); @@ -109,30 +109,30 @@ public static function newWithAutoDetection(Query|QueryBuilder $query): self /** * @return array{ - * queryHasGroupByClause: bool|null, - * queryHasHavingClause: bool|null, + * hasGroupByClause: bool|null, + * hasHavingClause: bool|null, * rootEntityHasSingleIdentifierFieldName: bool|null, - * queryCouldProduceDuplicates: bool, - * queryCouldHaveToManyJoins: bool, + * couldProduceDuplicates: bool, + * couldHaveToManyJoins: bool, * } */ private static function autoDetectQueryFeatures(EntityManagerInterface $entityManager, Node $queryAST): array { $queryFeatures = [ // Null means undetermined - 'queryHasGroupByClause' => null, - 'queryHasHavingClause' => null, + 'hasGroupByClause' => null, + 'hasHavingClause' => null, 'rootEntityHasSingleIdentifierFieldName' => null, - 'queryCouldProduceDuplicates' => true, - 'queryCouldHaveToManyJoins' => true, + 'couldProduceDuplicates' => true, + 'couldHaveToManyJoins' => true, ]; if (! $queryAST instanceof Query\AST\SelectStatement) { return $queryFeatures; } - $queryFeatures['queryHasGroupByClause'] = $queryAST->groupByClause !== null; - $queryFeatures['queryHasHavingClause'] = $queryAST->havingClause !== null; + $queryFeatures['hasGroupByClause'] = $queryAST->groupByClause !== null; + $queryFeatures['hasHavingClause'] = $queryAST->havingClause !== null; $from = $queryAST->fromClause->identificationVariableDeclarations; if (count($from) > 1) { @@ -209,7 +209,7 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa $toManyJoinsAliases[] = $joinAlias; } - $queryFeatures['queryCouldHaveToManyJoins'] = count($toManyJoinsAliases) > 0; + $queryFeatures['couldHaveToManyJoins'] = count($toManyJoinsAliases) > 0; // Check the Select list. foreach ($queryAST->selectClause->selectExpressions as $selectExpression) { @@ -254,7 +254,7 @@ private static function autoDetectQueryFeatures(EntityManagerInterface $entityMa return $queryFeatures; } - $queryFeatures['queryCouldProduceDuplicates'] = false; + $queryFeatures['couldProduceDuplicates'] = false; return $queryFeatures; } @@ -430,11 +430,11 @@ private function cloneQuery(Query $query): Query */ private function useOutputWalker(Query $query, bool $forCountQuery = false): bool { - if ($forCountQuery === false && $this->useResultQueryOutputWalker !== null) { + if (! $forCountQuery && $this->useResultQueryOutputWalker !== null) { return $this->useResultQueryOutputWalker; } - if ($forCountQuery === true && $this->useCountQueryOutputWalker !== null) { + if ($forCountQuery && $this->useCountQueryOutputWalker !== null) { return $this->useCountQueryOutputWalker; }