Do not use DISTINCT where there are no joins

[soyuka]

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

@teohhanhui would this work?

[meyerbaptiste]

Wonderful, I stopped counting the number of times I had to do this in projects! 🎉

[teohhanhui]

Thanks @soyuka! 🎉

[bendavies]

having tried this, i think it's hard to take advantage of this because of eager loading.

to improve, left joins could be removed from the count query, i think? i think that's harder than it sounds though.

[teohhanhui]

@bendavies But eager loading only kicks in if you use those relations in your serializer groups, no? So, DISTINCT is indeed necessary for such a query. (Or did you mean rewriting the query for COUNT?)

[bendavies]

ah, it's because i have one property with @ApiProperty(fetchEager=true)

[bendavies]

yes, i meant writing the query for count, as left joins don't affect the final count.

[teohhanhui]

yes, i meant writing the query for count, as left joins don't affect the final count.

You'll have to propose that in Doctrine ORM. ;)

[bendavies]

there is an improvement to be made here.
the count query has the distinct removed, but the next query to get the list of IDs does not

[soyuka]

Mhhh, I'm not sure that we have access to another query where we could set the hint?!

[teohhanhui]

@bendavies Same response as above: #2611 (comment)

You'll have to propose that in Doctrine ORM. ;)

[er1z]

You'll have to propose that in Doctrine ORM. ;)

Good luck. :D

[bendavies]

ah, that's not our query? oh well

[meyerbaptiste]

No, that comes from Doctrine: https://github.com/doctrine/orm/blob/32efbd3edd54a6f0fc331b6a0bf394fcacb88444/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L144-L149

[teohhanhui]

Hmm... But fetchJoinCollection should be set to false when there are no joins, right?

So, I think we could improve the logic here:

core/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

Lines 278 to 296 in 049d5aa

	/**
	* Determines whether the Paginator should fetch join collections, if the root entity uses composite identifiers it should not.
	*
	* @see https://github.com/doctrine/doctrine2/issues/2910
	*/
	private function useFetchJoinCollection(QueryBuilder $queryBuilder, string $resourceClass = null, string $operationName = null): bool
	{
	if (QueryChecker::hasRootEntityWithCompositeIdentifier($queryBuilder, $this->managerRegistry)) {
	return false;
	}
	if (null === $resourceClass) {
	return true;
	}
	$resourceMetadata = $this->resourceMetadataFactory->create($resourceClass);
	return $resourceMetadata->getCollectionOperationAttribute($operationName, 'pagination_fetch_join_collection', true, true);
	}

[teohhanhui]

By default the pagination extension does the following steps to compute the correct result:

- Perform a Count query using `DISTINCT` keyword.
- Perform a Limit Subquery with `DISTINCT` to find all ids of the entity in from on the current page.
- Perform a WHERE IN query to get all results for the current page.

This behavior is only necessary if you actually fetch join a to-many collection. You can disable this behavior by setting the $fetchJoinCollection flag to false; in that case only 2 instead of the 3 queries described are executed. We hope to automate the detection for this in the future.

https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/tutorials/pagination.html

I don't know how I've missed this important optimization... 🙈

[teohhanhui]

Note to self: we probably have to take care of fetch eager associations.