Updates regarding doctrine performances with associations #717

soyuka · 2016-08-31T09:45:13Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	∅
License	MIT
Doc PR	∅

Do not join twice when filtering on an association (EagerLoadingExtension + SearchFilter) (tested)
Inner join instead of left join relations when those aren't nullable EagerLoadingExtension (tested)
Do not to fetch properties that are not serializable! (tested)
Discuss Query::HINT_FORCE_PARTIAL_LOAD to avoid fetching OneToOne relations when we don't need them

dunglas · 2016-08-31T10:17:49Z

src/Bridge/Doctrine/Orm/Filter/SearchFilter.php

@@ -354,4 +367,21 @@ private function normalizeValues(array $values) : array

       return array_values($values);
   }
+
+    private function hasJoin($queryBuilder, $alias, $association)


Missing type hints.

By the way I think getJoin returning null when the join doesn't exist would be a cleaner API.

dunglas · 2016-08-31T10:21:20Z

Great improvements!

soyuka · 2016-08-31T16:56:43Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

+
+            //@TODO useless at the moment, but Doctrine queries non-necessary relations because addSelect($associationAlias) lazy fetch every association
+            //By using this array of properties, it Lazy fetch association, which are initially in an INNER JOIN (fetches them twice ?!)
+            //If I try to use associationAlias when it has every property, but the properties array when it doesn't it fails because it can not fetch parent association


This is still an issue, @Simperfit mind taking a look? I think you've a good background with Doctrine maybe you can help figuring this out :/.

soyuka · 2016-08-31T17:11:58Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

+            //
+            //By using this array of properties, Doctrine will lazy fetch every associations, which are initially in an INNER JOIN (fetches them twice ?!)
+            //If I try to use associationAlias when it has every property (so that Doctrine does not do sub queries on top of the initial one) it fails because it can not fetch parent association (like internally they try to find aliasX, but no aliasX is selected, we've instead aliasX.id, aliasX.name)
+            //I obviously tried to specify `WITH` `aliasX.id = aliasY.id` without success
            foreach ($this->getMetadataProperties($mapping['targetEntity']) as $property => $propertyMetadata) {


Please read my story, if you have any clue or idea to help me think it'd be wonderful :D. ping @Simperfit

Note to myself:
use partial ref ref2 ref3

teohhanhui · 2016-09-01T08:40:43Z

tests/Bridge/Doctrine/Orm/Extension/EagerLoadingExtensionTest.php

@@ -131,16 +131,16 @@ public function testApplyToItem()
        $classMetadataProphecy->getAssociationNames()->shouldBeCalled()->willReturn([0 => 'relatedDummy', 'relatedDummy2']);
        $classMetadataProphecy->associationMappings = [
            'relatedDummy' => [
-                'fetch' => 3, 'joinColumns' => [['nullable' => true]], 'targetEntity' => DummyRelated::class
+                'fetch' => 3, 'joinColumns' => [['nullable' => true]], 'targetEntity' => DummyRelated::class,


1 on each line please...

soyuka · 2016-09-01T08:43:29Z

Ready for review :D !

This improves the complex queries a lot! Strong impact on performances, mainly on collections with lots of related fields (almost counts in seconds).

dunglas · 2016-09-01T08:58:49Z

src/Bridge/Doctrine/Orm/Filter/AbstractFilter.php

+     *
+     * @return string the new association alias
+     */
+    protected function addJoinOnce($queryBuilder, $queryNameGenerator, $alias, $association): string


Why protected?

Missing typehints btw

Protected because it's called from filters (ie SearchFilter).

dunglas · 2016-09-05T08:40:27Z

Is it ready to be merged?

soyuka · 2016-09-05T10:21:06Z

Apart from my comment on query hint it's good to go.

Simperfit · 2016-09-05T18:24:38Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

+
+            $queryBuilder->addSelect(sprintf('partial %s.{%s}', $associationAlias, implode(',', $select)));
+
+            $relationAlias = $relationAlias.++$j;


is there any reason that $j could not be $nbOfJoin or smth like that. IMO it's not that clear

Yeah it's not really the number of join, it's a way to keep the alias unique across relation, ie a random number.

We should avoid random numbers in DQL as much as possible to trigger the Doctrine cache as often as possible.

It's the same in every query ofc. Just giving a explanation to the simple variable name.

Simperfit · 2016-09-05T18:29:28Z

@soyuka Amazing work ;)

soyuka · 2016-09-06T07:36:27Z

@dunglas I added a decoration possibility, not sure if I have to catch an undefined method call though. Should I add the same for the ItemDataProvider? Should I add an interface to be implemented by decorators using getQuery?

soyuka · 2016-09-06T07:50:35Z

Done thanks. I've added the same on ItemDataProvider for the sake of consistency.

dunglas · 2016-09-06T07:56:07Z

@soyuka After double checking, isn't this use case already supported by the QueryResultExtensionInterface introduced by @sroze?

Someone wanting to set Query::HINT_FORCE_PARTIAL_LOAD can register an extension having a getResult method doing it. WDYT?

soyuka · 2016-09-06T08:00:52Z

What I don't like with this interface is that it expects the query to return a result. For example, if you wanted to use the PaginationExtension but also modify some hints on Query it won't be possible. Thing is, Query can only be modified only once every QueryBuilder tasks are done...

dunglas · 2016-09-06T08:07:54Z

But for such cases, decoration can be used (decorating the PaginationExtension, and especially its getResult method for instance).

soyuka · 2016-09-06T08:11:55Z

Indeed, this is what I was going to say. For example:

/**
 * {@inheritdoc}
 */
public function getResult(QueryBuilder $queryBuilder)
{
    $query = $queryBuilder->getQuery();
    $query->setHint(Query::HINT_FORCE_PARTIAL_LOAD, true);

    $doctrineOrmPaginator = new DoctrineOrmPaginator($query, $this->useFetchJoinCollection($queryBuilder));
    $doctrineOrmPaginator->setUseOutputWalkers($this->useOutputWalkers($queryBuilder));

    return new Paginator($doctrineOrmPaginator);
}

soyuka · 2016-09-06T08:14:49Z

Wait for tests to pass and to me it's good to go.

dunglas · 2016-09-06T08:14:50Z

So, can you revert the last commit introducing the $decorated constructor parameter?

dunglas · 2016-09-06T08:15:58Z

src/Bridge/Doctrine/Orm/Filter/AbstractFilter.php

+     * @param string       $alias
+     * @param string       $association  the association field
+     *
+     * @return Doctrine\ORM\Query\Expr\Join|null


Can you add a use statement instead?

- use innerJoin when possible (relation is not nullable) - use partial selects and skip unreadable properties - improve coverage

dunglas · 2016-09-06T11:25:22Z

Thanks @soyuka

Updates regarding doctrine performances with associations

soyuka force-pushed the improve-doctrine-query branch from 891f884 to bc1406a Compare August 31, 2016 09:46

dunglas reviewed Aug 31, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch from bc1406a to 3f79227 Compare August 31, 2016 16:55

soyuka reviewed Aug 31, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch from 2fa9acd to 04fab64 Compare August 31, 2016 17:03

soyuka reviewed Aug 31, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch from e5570a2 to 179399d Compare September 1, 2016 08:36

teohhanhui reviewed Sep 1, 2016
View reviewed changes

soyuka changed the title ~~[WIP] Updates regarding doctrine performances with associations~~ Updates regarding doctrine performances with associations Sep 1, 2016

dunglas reviewed Sep 1, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch 2 times, most recently from 52e93dd to 20e380e Compare September 1, 2016 12:34

Simperfit reviewed Sep 5, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch 3 times, most recently from 140ade9 to d31b39f Compare September 6, 2016 07:32

soyuka force-pushed the improve-doctrine-query branch from d31b39f to 7bb1ca1 Compare September 6, 2016 07:46

soyuka force-pushed the improve-doctrine-query branch 3 times, most recently from 62832fd to ba129bc Compare September 6, 2016 07:50

soyuka force-pushed the improve-doctrine-query branch 2 times, most recently from 98b82ff to 6568143 Compare September 6, 2016 07:52

soyuka force-pushed the improve-doctrine-query branch from 6568143 to 4f21a05 Compare September 6, 2016 08:05

soyuka force-pushed the improve-doctrine-query branch from 4f21a05 to 583d234 Compare September 6, 2016 08:13

dunglas reviewed Sep 6, 2016
View reviewed changes

soyuka force-pushed the improve-doctrine-query branch from 583d234 to 430a714 Compare September 6, 2016 08:19

fix do not join twice when filtering on an association

ccc0ae4

soyuka force-pushed the improve-doctrine-query branch from 430a714 to 3b2ebd6 Compare September 6, 2016 08:21

Feat improve eager loading extension performance

ebf4735

- use innerJoin when possible (relation is not nullable) - use partial selects and skip unreadable properties - improve coverage

soyuka force-pushed the improve-doctrine-query branch from 3b2ebd6 to ebf4735 Compare September 6, 2016 08:22

soyuka mentioned this pull request Sep 6, 2016

Add notes about Doctrine performances api-platform/docs#106

Closed

dunglas merged commit 36b98c4 into api-platform:master Sep 6, 2016

soyuka deleted the improve-doctrine-query branch September 9, 2016 16:01

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#717 from soyuka/improve-doctrine-query

da25509

Updates regarding doctrine performances with associations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates regarding doctrine performances with associations #717

Updates regarding doctrine performances with associations #717

soyuka commented Aug 31, 2016 •

edited

Loading

dunglas Aug 31, 2016

dunglas Aug 31, 2016

dunglas commented Aug 31, 2016

soyuka Aug 31, 2016

soyuka Aug 31, 2016

soyuka Aug 31, 2016 •

edited

Loading

teohhanhui Sep 1, 2016

soyuka commented Sep 1, 2016

dunglas Sep 1, 2016

dunglas Sep 1, 2016

soyuka Sep 1, 2016

dunglas commented Sep 5, 2016

soyuka commented Sep 5, 2016

Simperfit Sep 5, 2016 •

edited

Loading

soyuka Sep 6, 2016

dunglas Sep 6, 2016

soyuka Sep 6, 2016 •

edited

Loading

Simperfit commented Sep 5, 2016

soyuka commented Sep 6, 2016

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

soyuka commented Sep 6, 2016 •

edited

Loading

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

dunglas Sep 6, 2016

dunglas commented Sep 6, 2016


		$queryBuilder->addSelect(sprintf('partial %s.{%s}', $associationAlias, implode(',', $select)));

		$relationAlias = $relationAlias.++$j;

Updates regarding doctrine performances with associations #717

Updates regarding doctrine performances with associations #717

Conversation

soyuka commented Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Sep 5, 2016

soyuka commented Sep 5, 2016

Simperfit Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

Simperfit commented Sep 5, 2016

soyuka commented Sep 6, 2016

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

soyuka commented Sep 6, 2016 • edited Loading

soyuka commented Sep 6, 2016

dunglas commented Sep 6, 2016

Choose a reason for hiding this comment

dunglas commented Sep 6, 2016

soyuka commented Aug 31, 2016 •

edited

Loading

soyuka Aug 31, 2016 •

edited

Loading

Simperfit Sep 5, 2016 •

edited

Loading

soyuka Sep 6, 2016 •

edited

Loading

soyuka commented Sep 6, 2016 •

edited

Loading