From a41b17f7ad1fa37285d4f3be00f9fbce89c961fa Mon Sep 17 00:00:00 2001 From: Constantine Karnaukhov Date: Thu, 4 Nov 2021 20:26:02 +0400 Subject: [PATCH 1/4] feat(select): use count(PK) if possible --- src/Select.php | 9 ++------- src/Select/RootLoader.php | 15 +++++++++++++++ tests/ORM/PaginateTest.php | 1 - tests/ORM/RelationWithColumnAliasTest.php | 1 - tests/ORM/SelectorTest.php | 16 ++++++++++++++++ 5 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/Select.php b/src/Select.php index 3686a3afa..a82955960 100644 --- a/src/Select.php +++ b/src/Select.php @@ -167,18 +167,13 @@ public function wherePK($id): self /** * Attention, column will be quoted by driver! * - * @param string|null $column When column is null DISTINCT(PK) will be generated. + * @param string|null $column When column is null PK or DISTINCT(PK) will be generated. * * @return int */ public function count(string $column = null): int { - if ($column === null) { - // @tuneyourserver solves the issue with counting on queries with joins. - $column = sprintf('DISTINCT(%s)', $this->loader->getPK()); - } - - return (int) $this->__call('count', [$column]); + return (int) $this->__call('count', [$column ?? $this->loader->getCountField()]); } /** diff --git a/src/Select/RootLoader.php b/src/Select/RootLoader.php index 972eb5f00..c386cf272 100644 --- a/src/Select/RootLoader.php +++ b/src/Select/RootLoader.php @@ -84,6 +84,21 @@ public function getPK(): string return $this->getAlias() . '.' . $this->fieldAlias($this->define(Schema::PRIMARY_KEY)); } + /** + * Returns PK or DISTINCT(PK) if query has any join. + * + * @return string + */ + public function getCountField(): string + { + if (0 < count($this->join)) { + // @tuneyourserver solves the issue with counting on queries with joins. + return sprintf('DISTINCT(%s)', $this->getPK()); + } + + return $this->getPK(); + } + /** * Return base query associated with the loader. * diff --git a/tests/ORM/PaginateTest.php b/tests/ORM/PaginateTest.php index dea95e4b6..a44cc93f0 100644 --- a/tests/ORM/PaginateTest.php +++ b/tests/ORM/PaginateTest.php @@ -8,7 +8,6 @@ */ declare(strict_types=1); -declare(strict_types=1); namespace Cycle\ORM\Tests; diff --git a/tests/ORM/RelationWithColumnAliasTest.php b/tests/ORM/RelationWithColumnAliasTest.php index e90d3ac1c..15dc9c510 100644 --- a/tests/ORM/RelationWithColumnAliasTest.php +++ b/tests/ORM/RelationWithColumnAliasTest.php @@ -8,7 +8,6 @@ */ declare(strict_types=1); -declare(strict_types=1); namespace Cycle\ORM\Tests; diff --git a/tests/ORM/SelectorTest.php b/tests/ORM/SelectorTest.php index 2393d252f..fe36af0db 100644 --- a/tests/ORM/SelectorTest.php +++ b/tests/ORM/SelectorTest.php @@ -133,4 +133,20 @@ public function testSelectCustomSQL(): void ], ], $result); } + + public function testCount(): void + { + $s = new Select($this->orm, User::class); + + $this->assertSame(2, $s->count()); + } + + public function testCountWithJoin(): void + { + $s = new Select($this->orm, User::class); + + $s->with('comments'); + + $this->assertSame(2, $s->count()); + } } From a051a7844f3791d3f75bfec909ef9d1aaf41fa93 Mon Sep 17 00:00:00 2001 From: Constantine Karnaukhov Date: Sat, 6 Nov 2021 22:51:33 +0400 Subject: [PATCH 2/4] feat(select): use count(PK) when joins not cause data duplication --- src/ORM.php | 2 +- src/Select.php | 12 +-- src/Select/AbstractLoader.php | 17 ++++ src/Select/JoinableLoader.php | 13 +++ src/Select/Loader/HasManyLoader.php | 5 ++ src/Select/Loader/ManyToManyLoader.php | 5 ++ src/Select/RootLoader.php | 2 +- tests/ORM/SelectorTest.php | 109 ++++++++++++++++++++++--- 8 files changed, 141 insertions(+), 24 deletions(-) diff --git a/src/ORM.php b/src/ORM.php index 44afc4bf8..f343baffc 100644 --- a/src/ORM.php +++ b/src/ORM.php @@ -388,7 +388,7 @@ public function queueDelete($entity, int $mode = TransactionInterface::MODE_CASC * * @return array */ - protected function getIndexes(string $role): array + public function getIndexes(string $role): array { if (isset($this->indexes[$role])) { return $this->indexes[$role]; diff --git a/src/Select.php b/src/Select.php index a82955960..35a3e697b 100644 --- a/src/Select.php +++ b/src/Select.php @@ -65,7 +65,7 @@ public function __construct(ORMInterface $orm, string $role) { $this->orm = $orm; $this->loader = new RootLoader($orm, $this->orm->resolveRole($role)); - $this->builder = new QueryBuilder($this->getLoader()->getQuery(), $this->loader); + $this->builder = new QueryBuilder($this->loader->getQuery(), $this->loader); } /** @@ -436,14 +436,4 @@ public function sqlStatement(): string { return $this->buildQuery()->sqlStatement(); } - - /** - * Return base loader associated with the selector. - * - * @return RootLoader - */ - private function getLoader(): RootLoader - { - return $this->loader; - } } diff --git a/src/Select/AbstractLoader.php b/src/Select/AbstractLoader.php index 3fe481c22..1939ac8b8 100644 --- a/src/Select/AbstractLoader.php +++ b/src/Select/AbstractLoader.php @@ -259,6 +259,23 @@ public function loadData(AbstractNode $node): void */ abstract public function isLoaded(): bool; + /** + * Indicates that query can load multiple joined rows, + * which possibly can lead to data duplication. + * + * @return bool + */ + public function isDataDuplicationPossible(): bool + { + foreach ($this->join as $loader) { + if ($loader->isDataDuplicationPossible()) { + return true; + } + } + + return false; + } + /** * @param AbstractNode $node */ diff --git a/src/Select/JoinableLoader.php b/src/Select/JoinableLoader.php index 84f41c41b..745a72d99 100644 --- a/src/Select/JoinableLoader.php +++ b/src/Select/JoinableLoader.php @@ -14,6 +14,7 @@ use Cycle\ORM\Exception\LoaderException; use Cycle\ORM\ORMInterface; use Cycle\ORM\Parser\AbstractNode; +use Cycle\ORM\Relation; use Cycle\ORM\Schema; use Cycle\ORM\Select\Traits\ColumnsTrait; use Cycle\ORM\Select\Traits\ScopeTrait; @@ -233,6 +234,18 @@ public function configureQuery(SelectQuery $query, array $outerKeys = []): Selec return parent::configureQuery($query); } + public function isDataDuplicationPossible(): bool + { + $outerKey = $this->schema[Relation::OUTER_KEY]; + $indexes = $this->orm->getIndexes($this->target); + + if (!\in_array($outerKey, $indexes, true)) { + return true; + } + + return parent::isDataDuplicationPossible(); + } + /** * @param SelectQuery $query * diff --git a/src/Select/Loader/HasManyLoader.php b/src/Select/Loader/HasManyLoader.php index 8516c4b89..491b6d3f3 100644 --- a/src/Select/Loader/HasManyLoader.php +++ b/src/Select/Loader/HasManyLoader.php @@ -102,6 +102,11 @@ public function configureQuery(SelectQuery $query, array $outerKeys = []): Selec return parent::configureQuery($query); } + public function isDataDuplicationPossible(): bool + { + return true; + } + /** * {@inheritdoc} */ diff --git a/src/Select/Loader/ManyToManyLoader.php b/src/Select/Loader/ManyToManyLoader.php index a82b0f4c1..9b0d3c84e 100644 --- a/src/Select/Loader/ManyToManyLoader.php +++ b/src/Select/Loader/ManyToManyLoader.php @@ -197,6 +197,11 @@ public function createNode(): AbstractNode return $node; } + public function isDataDuplicationPossible(): bool + { + return true; + } + /** * @param AbstractNode $node */ diff --git a/src/Select/RootLoader.php b/src/Select/RootLoader.php index c386cf272..3e297ab55 100644 --- a/src/Select/RootLoader.php +++ b/src/Select/RootLoader.php @@ -91,7 +91,7 @@ public function getPK(): string */ public function getCountField(): string { - if (0 < count($this->join)) { + if ($this->isDataDuplicationPossible()) { // @tuneyourserver solves the issue with counting on queries with joins. return sprintf('DISTINCT(%s)', $this->getPK()); } diff --git a/tests/ORM/SelectorTest.php b/tests/ORM/SelectorTest.php index fe36af0db..af921100d 100644 --- a/tests/ORM/SelectorTest.php +++ b/tests/ORM/SelectorTest.php @@ -16,7 +16,9 @@ use Cycle\ORM\Schema; use Cycle\ORM\Select; use Cycle\ORM\Select\JoinableLoader; +use Cycle\ORM\Select\RootLoader; use Cycle\ORM\Tests\Fixtures\Comment; +use Cycle\ORM\Tests\Fixtures\Profile; use Cycle\ORM\Tests\Fixtures\User; use Cycle\ORM\Tests\Traits\TableTrait; @@ -32,6 +34,7 @@ public function setUp(): void 'id' => 'primary', 'email' => 'string', 'balance' => 'float', + 'comment_id' => 'integer,nullable', ]); $this->getDatabase()->table('user')->insertMultiple( @@ -42,6 +45,20 @@ public function setUp(): void ] ); + $this->makeTable('profile', [ + 'id' => 'primary', + 'user_id' => 'integer', + 'image' => 'string', + ]); + + $this->getDatabase()->table('profile')->insertMultiple( + ['user_id', 'image'], + [ + [1, 'image.png'], + [2, 'second.png'], + ] + ); + $this->makeTable('comment', [ 'id' => 'primary', 'user_id' => 'integer', @@ -62,16 +79,40 @@ public function setUp(): void ] ); - $this->orm = $this->withSchema(new Schema([ + $this->orm = $this->withSchema(new Schema($this->getSchemaDefinition())); + } + + private function getSchemaDefinition(): array + { + return [ User::class => [ Schema::ROLE => 'user', Schema::MAPPER => Mapper::class, Schema::DATABASE => 'default', Schema::TABLE => 'user', Schema::PRIMARY_KEY => 'id', - Schema::COLUMNS => ['id', 'email', 'balance'], + Schema::COLUMNS => ['id', 'email', 'balance', 'comment_id'], Schema::SCHEMA => [], Schema::RELATIONS => [ + 'profile' => [ + Relation::TYPE => Relation::HAS_ONE, + Relation::TARGET => Profile::class, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::INNER_KEY => 'id', + Relation::OUTER_KEY => 'user_id', + Relation::NULLABLE => false, + ], + ], + 'lastComment' => [ + Relation::TYPE => Relation::REFERS_TO, + Relation::TARGET => Comment::class, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::INNER_KEY => 'comment_id', + Relation::OUTER_KEY => 'id', + ], + ], 'comments' => [ Relation::TYPE => Relation::HAS_MANY, Relation::TARGET => Comment::class, @@ -93,7 +134,29 @@ public function setUp(): void Schema::SCHEMA => [], Schema::RELATIONS => [], ], - ])); + Profile::class => [ + Schema::ROLE => 'profile', + Schema::MAPPER => Mapper::class, + Schema::DATABASE => 'default', + Schema::TABLE => 'profile', + Schema::PRIMARY_KEY => 'id', + Schema::FIND_BY_KEYS => ['user_id'], + Schema::COLUMNS => ['id', 'user_id', 'image'], + Schema::SCHEMA => [], + Schema::RELATIONS => [ + 'user' => [ + Relation::TYPE => Relation::BELONGS_TO, + Relation::TARGET => User::class, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::INNER_KEY => 'user_id', + Relation::OUTER_KEY => 'id', + Relation::NULLABLE => false, + ], + ], + ], + ], + ]; } public function testStableStatement(): void @@ -136,17 +199,41 @@ public function testSelectCustomSQL(): void public function testCount(): void { - $s = new Select($this->orm, User::class); - - $this->assertSame(2, $s->count()); + $this->assertSame(2, (new Select($this->orm, User::class))->count()); + $this->assertSame(2, (new Select($this->orm, User::class))->with('profile')->count()); + $this->assertSame(2, (new Select($this->orm, User::class))->with('lastComment', [ + 'method' => Select\AbstractLoader::LEFT_JOIN, + ])->count()); + $this->assertSame(2, (new Select($this->orm, User::class))->with('comments')->count()); + + $schema = $this->getSchemaDefinition(); + unset($schema[Profile::class][Schema::FIND_BY_KEYS]); + $this->orm = $this->withSchema(new Schema($schema)); + $this->assertSame(2, (new Select($this->orm, User::class))->with('profile')->count()); } - public function testCountWithJoin(): void + public function testCountField(): void { - $s = new Select($this->orm, User::class); - - $s->with('comments'); + $role = $this->orm->resolveRole(User::class); + $pk = \sprintf('%s.%s', $role, 'id'); + $distinct = \sprintf('DISTINCT(%s)', $pk); + + $this->assertSame($pk, (new RootLoader($this->orm, $role))->getCountField()); + $this->assertSame($pk, $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField()); + $this->assertSame($pk, $this->joinRelation(new RootLoader($this->orm, $role), 'lastComment', [ + 'method' => Select\AbstractLoader::LEFT_JOIN, + ])->getCountField()); + $this->assertSame($distinct, $this->joinRelation(new RootLoader($this->orm, $role), 'comments')->getCountField()); + + $schema = $this->getSchemaDefinition(); + unset($schema[Profile::class][Schema::FIND_BY_KEYS]); + $this->orm = $this->withSchema(new Schema($schema)); + $this->assertSame($distinct, $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField()); + } - $this->assertSame(2, $s->count()); + private function joinRelation(RootLoader $loader, string $relation, array $options = []): RootLoader + { + $loader->loadRelation($relation, $options, true); + return $loader; } } From 936869d45bd8766d1a639fc09e0337650ea9408a Mon Sep 17 00:00:00 2001 From: Constantine Karnaukhov Date: Sat, 6 Nov 2021 23:00:58 +0400 Subject: [PATCH 3/4] chore: fix code style --- tests/ORM/SelectorTest.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/ORM/SelectorTest.php b/tests/ORM/SelectorTest.php index af921100d..06e08bfb4 100644 --- a/tests/ORM/SelectorTest.php +++ b/tests/ORM/SelectorTest.php @@ -223,12 +223,18 @@ public function testCountField(): void $this->assertSame($pk, $this->joinRelation(new RootLoader($this->orm, $role), 'lastComment', [ 'method' => Select\AbstractLoader::LEFT_JOIN, ])->getCountField()); - $this->assertSame($distinct, $this->joinRelation(new RootLoader($this->orm, $role), 'comments')->getCountField()); + $this->assertSame( + $distinct, + $this->joinRelation(new RootLoader($this->orm, $role), 'comments')->getCountField() + ); $schema = $this->getSchemaDefinition(); unset($schema[Profile::class][Schema::FIND_BY_KEYS]); $this->orm = $this->withSchema(new Schema($schema)); - $this->assertSame($distinct, $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField()); + $this->assertSame( + $distinct, + $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField() + ); } private function joinRelation(RootLoader $loader, string $relation, array $options = []): RootLoader From b7ea9a9c7f39c52bcd5af0ee30a95c76a13a3546 Mon Sep 17 00:00:00 2001 From: Constantine Karnaukhov Date: Mon, 6 Dec 2021 16:59:34 +0400 Subject: [PATCH 4/4] refactor: use count(*) for better performance when possible --- src/Select/RootLoader.php | 3 +-- tests/ORM/SelectorTest.php | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Select/RootLoader.php b/src/Select/RootLoader.php index 3e297ab55..4d8a90764 100644 --- a/src/Select/RootLoader.php +++ b/src/Select/RootLoader.php @@ -92,11 +92,10 @@ public function getPK(): string public function getCountField(): string { if ($this->isDataDuplicationPossible()) { - // @tuneyourserver solves the issue with counting on queries with joins. return sprintf('DISTINCT(%s)', $this->getPK()); } - return $this->getPK(); + return '*'; } /** diff --git a/tests/ORM/SelectorTest.php b/tests/ORM/SelectorTest.php index 06e08bfb4..fd6756ad6 100644 --- a/tests/ORM/SelectorTest.php +++ b/tests/ORM/SelectorTest.php @@ -218,9 +218,9 @@ public function testCountField(): void $pk = \sprintf('%s.%s', $role, 'id'); $distinct = \sprintf('DISTINCT(%s)', $pk); - $this->assertSame($pk, (new RootLoader($this->orm, $role))->getCountField()); - $this->assertSame($pk, $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField()); - $this->assertSame($pk, $this->joinRelation(new RootLoader($this->orm, $role), 'lastComment', [ + $this->assertSame('*', (new RootLoader($this->orm, $role))->getCountField()); + $this->assertSame('*', $this->joinRelation(new RootLoader($this->orm, $role), 'profile')->getCountField()); + $this->assertSame('*', $this->joinRelation(new RootLoader($this->orm, $role), 'lastComment', [ 'method' => Select\AbstractLoader::LEFT_JOIN, ])->getCountField()); $this->assertSame(