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

[GH-1569] Optimize eager fetch for collections to batch query #8391

Merged
merged 19 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
dc899e2
[GH-1569] Add new SUBSELECT fetch mode for OneToMany associations.
beberlei Dec 13, 2020
fdceb82
Disallow WITH keyword on fetch joined associatiosn via subselect.
beberlei Oct 10, 2023
47952c3
Houskeeping: phpcs
beberlei Oct 10, 2023
b9e55ba
Introduce configuration option for subselect batch size.
beberlei Oct 10, 2023
41410e6
Go through Persister API instead of indirectly through repository.
beberlei Oct 10, 2023
ff28ba8
Disallow use of fetch=SUBSELECT on to-one associations.
beberlei Oct 10, 2023
40bfe07
Make sure to many assocatinos are also respecting AbstractQuery::setF…
beberlei Oct 10, 2023
76fd34f
Avoid new fetch mode, use this strategy with fetch=EAGER for collecti…
beberlei Oct 14, 2023
cd54c56
Directly load many to many collections, batching not supported yet. f…
beberlei Oct 14, 2023
bf74b43
Housekeeping: phpcs
beberlei Oct 14, 2023
8ec599b
Static analysis
beberlei Oct 14, 2023
c09660a
Merge remote-tracking branch 'origin/2.17.x' into GH-1569-SubselectFe…
beberlei Oct 14, 2023
8057b51
last violation hopefully
beberlei Oct 14, 2023
3f2fa30
Add another testcase for DQL based fetch eager of collection.
beberlei Oct 14, 2023
6993ad2
1:1 and M:1 associations also use fetch batch size configuration now.
beberlei Oct 22, 2023
d03aed1
Explain internals of eager loading in a bit more detail and how its c…
beberlei Oct 22, 2023
609e10d
Address review comments.
beberlei Oct 22, 2023
4b2b486
Housekeeping: Revert change to AbstractExporter, not needed without s…
beberlei Oct 22, 2023
ec74c83
Fix typos
beberlei Nov 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/en/reference/working-with-objects.rst
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,23 @@ and these associations are mapped as EAGER, they will automatically
be loaded together with the entity being queried and is thus
immediately available to your application.

Eager Loading can also be configured at runtime through
``AbstractQuery::setFetchMode`` in DQL or Native Queries.

Eager loading for many-to-one and one-to-one associations is using either a
LEFT JOIN or a second query for fetching the related entity eagerly.

Eager loading for many-to-one associations uses a second query to load
the collections for several entities at the same time.

When many-to-many, one-to-one or one-to-many associations are eagerly loaded,
then the global batch size configuration is used to avoid IN(?) queries with
too many arguments. The default batch size is 100 and can be changed with
``Configuration::setEagerFetchBatchSize()``.

For eagerly loaded Many-To-Many associations one query has to be made for each
collection.

By Lazy Loading
~~~~~~~~~~~~~~~

Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -1144,4 +1144,14 @@
{
return $this->_attributes['rejectIdCollisionInIdentityMap'] ?? false;
}

public function setEagerFetchBatchSize(int $batchSize = 100): void

Check warning on line 1148 in lib/Doctrine/ORM/Configuration.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Configuration.php#L1148

Added line #L1148 was not covered by tests
{
$this->_attributes['fetchModeSubselectBatchSize'] = $batchSize;
}

Check warning on line 1151 in lib/Doctrine/ORM/Configuration.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Configuration.php#L1150-L1151

Added lines #L1150 - L1151 were not covered by tests

public function getEagerFetchBatchSize(): int
{
return $this->_attributes['fetchModeSubselectBatchSize'] ?? 100;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1264,7 +1264,7 @@ protected function getSelectColumnsSQL()
}

$isAssocToOneInverseSide = $assoc['type'] & ClassMetadata::TO_ONE && ! $assoc['isOwningSide'];
$isAssocFromOneEager = $assoc['type'] !== ClassMetadata::MANY_TO_MANY && $assoc['fetch'] === ClassMetadata::FETCH_EAGER;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

$isAssocFromOneEager = $assoc['type'] & ClassMetadata::TO_ONE && $assoc['fetch'] === ClassMetadata::FETCH_EAGER;

if (! ($isAssocFromOneEager || $isAssocToOneInverseSide)) {
continue;
Expand Down
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Query/QueryException.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,14 @@ public static function iterateWithFetchJoinNotAllowed($assoc)
);
}

public static function eagerFetchJoinWithNotAllowed(string $sourceEntity, string $fieldName): QueryException
{
return new self(
'Associations with fetch-mode=EAGER may not be using WITH conditions in
"' . $sourceEntity . '#' . $fieldName . '".'
);
}

public static function iterateWithMixedResultNotAllowed(): QueryException
{
return new self('Iterating a query with mixed results (using scalars) is not supported.');
Expand Down
4 changes: 3 additions & 1 deletion lib/Doctrine/ORM/Query/SqlWalker.php
Original file line number Diff line number Diff line change
Expand Up @@ -1047,7 +1047,9 @@ public function walkJoinAssociationDeclaration($joinAssociationDeclaration, $joi
}
}

$targetTableJoin = null;
if ($relation['fetch'] === ClassMetadata::FETCH_EAGER && $condExpr !== null) {
throw QueryException::eagerFetchJoinWithNotAllowed($assoc['sourceEntity'], $assoc['fieldName']);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I probably have a poor implementation or use of eager mode on my relationships but this exception creates a BC break on my side.
Maybe it should be replaced by a deprecation pointing to this ticket : #10978 to explain why this is a bad pratice ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this is a BC in v2... How was it not been cached before merging? :(

}

// This condition is not checking ClassMetadata::MANY_TO_ONE, because by definition it cannot
// be the owning side and previously we ensured that $assoc is always the owning side of the associations.
Expand Down
118 changes: 106 additions & 12 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
use Throwable;
use UnexpectedValueException;

use function array_chunk;
use function array_combine;
use function array_diff_key;
use function array_filter;
Expand Down Expand Up @@ -314,6 +315,9 @@
*/
private $eagerLoadingEntities = [];

/** @var array<string, array<string, mixed>> */
private $eagerLoadingCollections = [];

/** @var bool */
protected $hasCache = false;

Expand Down Expand Up @@ -2749,6 +2753,7 @@
$this->pendingCollectionElementRemovals =
$this->visitedCollections =
$this->eagerLoadingEntities =
$this->eagerLoadingCollections =
$this->orphanRemovals = [];
} else {
Deprecation::triggerIfCalledFromOutside(
Expand Down Expand Up @@ -2938,6 +2943,10 @@
continue;
}

if (! isset($hints['fetchMode'][$class->name][$field])) {
$hints['fetchMode'][$class->name][$field] = $assoc['fetch'];
}

$targetClass = $this->em->getClassMetadata($assoc['targetEntity']);

switch (true) {
Expand Down Expand Up @@ -3001,10 +3010,6 @@
break;
}

if (! isset($hints['fetchMode'][$class->name][$field])) {
$hints['fetchMode'][$class->name][$field] = $assoc['fetch'];
}

// Foreign key is set
// Check identity map first
// FIXME: Can break easily with composite keys if join column values are in
Expand Down Expand Up @@ -3098,9 +3103,13 @@
$reflField = $class->reflFields[$field];
$reflField->setValue($entity, $pColl);

if ($assoc['fetch'] === ClassMetadata::FETCH_EAGER) {
$this->loadCollection($pColl);
$pColl->takeSnapshot();
if ($hints['fetchMode'][$class->name][$field] === ClassMetadata::FETCH_EAGER) {
if ($assoc['type'] === ClassMetadata::ONE_TO_MANY) {
$this->scheduleCollectionForBatchLoading($pColl, $class);
} elseif ($assoc['type'] === ClassMetadata::MANY_TO_MANY) {
$this->loadCollection($pColl);
$pColl->takeSnapshot();

Check warning on line 3111 in lib/Doctrine/ORM/UnitOfWork.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/UnitOfWork.php#L3109-L3111

Added lines #L3109 - L3111 were not covered by tests
}
}

$this->originalEntityData[$oid][$field] = $pColl;
Expand All @@ -3117,7 +3126,7 @@
/** @return void */
public function triggerEagerLoads()
{
if (! $this->eagerLoadingEntities) {
if (! $this->eagerLoadingEntities && ! $this->eagerLoadingCollections) {
return;
}

Expand All @@ -3130,11 +3139,69 @@
continue;
}

$class = $this->em->getClassMetadata($entityName);
$class = $this->em->getClassMetadata($entityName);
$batches = array_chunk($ids, $this->em->getConfiguration()->getEagerFetchBatchSize());

$this->getEntityPersister($entityName)->loadAll(
array_combine($class->identifier, [array_values($ids)])
);
foreach ($batches as $batchedIds) {
$this->getEntityPersister($entityName)->loadAll(
array_combine($class->identifier, [$batchedIds])
);
}
}

$eagerLoadingCollections = $this->eagerLoadingCollections; // avoid recursion
$this->eagerLoadingCollections = [];

foreach ($eagerLoadingCollections as $group) {
$this->eagerLoadCollections($group['items'], $group['mapping']);
}
}

/**
* Load all data into the given collections, according to the specified mapping
*
* @param PersistentCollection[] $collections
* @param array<string, mixed> $mapping
* @psalm-param array{targetEntity: class-string, sourceEntity: class-string, mappedBy: string, indexBy: string|null} $mapping
*/
private function eagerLoadCollections(array $collections, array $mapping): void
{
$targetEntity = $mapping['targetEntity'];
$class = $this->em->getClassMetadata($mapping['sourceEntity']);
$mappedBy = $mapping['mappedBy'];

$batches = array_chunk($collections, $this->em->getConfiguration()->getEagerFetchBatchSize(), true);

foreach ($batches as $collectionBatch) {
$entities = [];

foreach ($collectionBatch as $collection) {
$entities[] = $collection->getOwner();
}

$found = $this->getEntityPersister($targetEntity)->loadAll([$mappedBy => $entities]);

$targetClass = $this->em->getClassMetadata($targetEntity);
$targetProperty = $targetClass->getReflectionProperty($mappedBy);

foreach ($found as $targetValue) {
$sourceEntity = $targetProperty->getValue($targetValue);

$id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity));
$idHash = implode(' ', $id);

if (isset($mapping['indexBy'])) {
$indexByProperty = $targetClass->getReflectionProperty($mapping['indexBy']);
$collectionBatch[$idHash]->hydrateSet($indexByProperty->getValue($targetValue), $targetValue);
} else {
$collectionBatch[$idHash]->add($targetValue);
}
}
}

foreach ($collections as $association) {
$association->setInitialized(true);
$association->takeSnapshot();
}
}

Expand Down Expand Up @@ -3165,6 +3232,33 @@
$collection->setInitialized(true);
}

/**
* Schedule this collection for batch loading at the end of the UnitOfWork
*/
private function scheduleCollectionForBatchLoading(PersistentCollection $collection, ClassMetadata $sourceClass): void
{
$mapping = $collection->getMapping();
$name = $mapping['sourceEntity'] . '#' . $mapping['fieldName'];

if (! isset($this->eagerLoadingCollections[$name])) {
$this->eagerLoadingCollections[$name] = [
'items' => [],
'mapping' => $mapping,
];
}

$owner = $collection->getOwner();
assert($owner !== null);

$id = $this->identifierFlattener->flattenIdentifier(
$sourceClass,
$sourceClass->getIdentifierValues($owner)
);
$idHash = implode(' ', $id);

$this->eagerLoadingCollections[$name]['items'][$idHash] = $collection;
}

/**
* Gets the identity map of the UnitOfWork.
*
Expand Down
Loading