From 4375d6bc2159d8e2f1b5fa0281b7282b86f945df Mon Sep 17 00:00:00 2001 From: Jan Skrasek Date: Wed, 30 Oct 2024 20:19:35 +0100 Subject: [PATCH 1/3] add ignore for Orm 4.0 branch testing setup files --- tests/.gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/.gitignore b/tests/.gitignore index a3a59621..8f7e567e 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -2,3 +2,6 @@ output /tmp /test.log /databases.ini +# for 4.0 branch +/sections.ini +/config.*.neon From 67e14dc03aca6aefb1831c97af8ea9d365dce3f3 Mon Sep 17 00:00:00 2001 From: Jan Skrasek Date: Tue, 29 Oct 2024 22:46:46 +0100 Subject: [PATCH 2/3] drop QueryBuilder parameter for dbal aggregation --- src/Collection/Aggregations/Aggregator.php | 1 - src/Collection/Aggregations/AnyAggregator.php | 1 - src/Collection/Aggregations/CountAggregator.php | 1 - src/Collection/Aggregations/NoneAggregator.php | 1 - src/Collection/Aggregations/NumericAggregator.php | 1 - src/Collection/Functions/BaseNumericAggregateFunction.php | 2 +- src/Collection/Functions/Result/DbalExpressionResult.php | 5 ++--- 7 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/Collection/Aggregations/Aggregator.php b/src/Collection/Aggregations/Aggregator.php index 5ef07767..02824a0b 100644 --- a/src/Collection/Aggregations/Aggregator.php +++ b/src/Collection/Aggregations/Aggregator.php @@ -31,7 +31,6 @@ public function aggregateValues(array $values); public function aggregateExpression( - QueryBuilder $queryBuilder, DbalExpressionResult $expression, ExpressionContext $context, ): DbalExpressionResult; diff --git a/src/Collection/Aggregations/AnyAggregator.php b/src/Collection/Aggregations/AnyAggregator.php index d9964a18..84fa25e1 100644 --- a/src/Collection/Aggregations/AnyAggregator.php +++ b/src/Collection/Aggregations/AnyAggregator.php @@ -48,7 +48,6 @@ public function aggregateValues(array $values): bool public function aggregateExpression( - QueryBuilder $queryBuilder, DbalExpressionResult $expression, ExpressionContext $context, ): DbalExpressionResult diff --git a/src/Collection/Aggregations/CountAggregator.php b/src/Collection/Aggregations/CountAggregator.php index c974851e..7e2797d9 100644 --- a/src/Collection/Aggregations/CountAggregator.php +++ b/src/Collection/Aggregations/CountAggregator.php @@ -50,7 +50,6 @@ public function aggregateValues(array $values): bool public function aggregateExpression( - QueryBuilder $queryBuilder, DbalExpressionResult $expression, ExpressionContext $context, ): DbalExpressionResult diff --git a/src/Collection/Aggregations/NoneAggregator.php b/src/Collection/Aggregations/NoneAggregator.php index b1358ea9..85fde472 100644 --- a/src/Collection/Aggregations/NoneAggregator.php +++ b/src/Collection/Aggregations/NoneAggregator.php @@ -48,7 +48,6 @@ public function aggregateValues(array $values): bool public function aggregateExpression( - QueryBuilder $queryBuilder, DbalExpressionResult $expression, ExpressionContext $context, ): DbalExpressionResult diff --git a/src/Collection/Aggregations/NumericAggregator.php b/src/Collection/Aggregations/NumericAggregator.php index 795ce66f..1b69568e 100644 --- a/src/Collection/Aggregations/NumericAggregator.php +++ b/src/Collection/Aggregations/NumericAggregator.php @@ -40,7 +40,6 @@ public function aggregateValues(array $values): mixed public function aggregateExpression( - QueryBuilder $queryBuilder, DbalExpressionResult $expression, ExpressionContext $context, ): DbalExpressionResult diff --git a/src/Collection/Functions/BaseNumericAggregateFunction.php b/src/Collection/Functions/BaseNumericAggregateFunction.php index 335baa4f..811f9e3e 100644 --- a/src/Collection/Functions/BaseNumericAggregateFunction.php +++ b/src/Collection/Functions/BaseNumericAggregateFunction.php @@ -65,6 +65,6 @@ public function processDbalExpression( } return $helper->processExpression($builder, $args[0], $context, $this->aggregator) - ->applyAggregator($builder, ExpressionContext::ValueExpression); + ->applyAggregator(ExpressionContext::ValueExpression); } } diff --git a/src/Collection/Functions/Result/DbalExpressionResult.php b/src/Collection/Functions/Result/DbalExpressionResult.php index 324be11b..b1bf07f0 100644 --- a/src/Collection/Functions/Result/DbalExpressionResult.php +++ b/src/Collection/Functions/Result/DbalExpressionResult.php @@ -4,7 +4,6 @@ use Nextras\Dbal\Platforms\Data\Fqn; -use Nextras\Dbal\QueryBuilder\QueryBuilder; use Nextras\Orm\Collection\Aggregations\Aggregator; use Nextras\Orm\Collection\Expression\ExpressionContext; use Nextras\Orm\Entity\Reflection\PropertyMetadata; @@ -164,8 +163,8 @@ public function withHavingArgs(string $havingExpression, array $havingArgs): Dba /** * Applies the aggregator and returns modified expression result. */ - public function applyAggregator(QueryBuilder $queryBuilder, ExpressionContext $context): DbalExpressionResult + public function applyAggregator(ExpressionContext $context): DbalExpressionResult { - return $this->aggregator?->aggregateExpression($queryBuilder, $this, $context) ?? $this; + return $this->aggregator?->aggregateExpression($this, $context) ?? $this; } } From ad6dfe07bdf54d511814213f4dc95fd8349ebfae Mon Sep 17 00:00:00 2001 From: Jan Skrasek Date: Wed, 30 Oct 2024 20:20:36 +0100 Subject: [PATCH 3/3] rework collection functions to utilize two-phase expression processing To provide more smarted SQL rewrites, we need to know if the expression itself is in AND/OR junction and if other parts of the junction require a HAVING clause. This is possible only after getting the full expression tree. Then we collect the actual expressions. [closes #690] [closes #686] --- src/Collection/Aggregations/AnyAggregator.php | 2 +- src/Collection/DbalCollection.php | 13 ++- .../Expression/ExpressionContext.php | 4 + .../Functions/BaseCompareFunction.php | 27 +++-- .../BaseNumericAggregateFunction.php | 3 +- .../Functions/CollectionFunction.php | 1 - .../Functions/CompareEqualsFunction.php | 9 ++ .../Functions/CompareLikeFunction.php | 3 +- .../Functions/ConjunctionOperatorFunction.php | 2 - .../Functions/DisjunctionOperatorFunction.php | 2 - .../Functions/FetchPropertyFunction.php | 1 - .../Functions/JunctionFunctionTrait.php | 99 +++++++++++-------- .../Functions/Result/DbalExpressionResult.php | 40 ++++++++ .../Helpers/DbalQueryBuilderHelper.php | 4 +- .../Functions/FetchPropertyFunctionTest.php | 5 - .../Collection/collection.aggregation.phpt | 29 ++++++ .../relationships.oneHasMany.phpt | 8 +- .../Dbal/DbalValueOperatorFunctionTest.phpt | 2 +- ...ovingPrimaryTableConditionToWhenClause.sql | 35 +++++++ ...yWhenLiftingNonAggregatedJoinCondition.sql | 21 ++++ ...estHavingWithSameNamedColumnsInGroupBy.sql | 19 ++-- ...sManyTest_testJoinAcrossDifferentPaths.sql | 22 ++--- ...toredOnOneHasManyRelationshipCondition.sql | 21 ++-- ...stSameTableJoinWithImplicitAggregation.sql | 50 +++------- 24 files changed, 265 insertions(+), 157 deletions(-) create mode 100644 tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testMovingPrimaryTableConditionToWhenClause.sql create mode 100644 tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testProperGroupByWhenLiftingNonAggregatedJoinCondition.sql diff --git a/src/Collection/Aggregations/AnyAggregator.php b/src/Collection/Aggregations/AnyAggregator.php index 84fa25e1..32a4c6f2 100644 --- a/src/Collection/Aggregations/AnyAggregator.php +++ b/src/Collection/Aggregations/AnyAggregator.php @@ -52,7 +52,7 @@ public function aggregateExpression( ExpressionContext $context, ): DbalExpressionResult { - if ($context !== ExpressionContext::FilterOr) { + if ($context !== ExpressionContext::FilterOrWithHavingClause) { // When we are not in OR expression, we may simply filter the joined table by the condition. // Otherwise, we have to employ a HAVING clause with aggregation function. return $expression; diff --git a/src/Collection/DbalCollection.php b/src/Collection/DbalCollection.php index 9670c6cb..90a34f52 100644 --- a/src/Collection/DbalCollection.php +++ b/src/Collection/DbalCollection.php @@ -107,13 +107,13 @@ public function orderBy($expression, string $direction = ICollection::ASC): ICol foreach ($expression as $subExpression => $subDirection) { $collection->ordering[] = [ - $helper->processExpression($collection->queryBuilder, $subExpression, ExpressionContext::FilterAnd, null), + $helper->processExpression($collection->queryBuilder, $subExpression, null), $subDirection, ]; } } else { $collection->ordering[] = [ - $helper->processExpression($collection->queryBuilder, $expression, ExpressionContext::ValueExpression, null), + $helper->processExpression($collection->queryBuilder, $expression, null), $direction, ]; } @@ -294,15 +294,18 @@ public function getQueryBuilder(): QueryBuilder $expression = $helper->processExpression( builder: $this->queryBuilder, expression: $args, - context: ExpressionContext::FilterAnd, aggregator: null, ); + $finalContext = $expression->havingExpression === null + ? ExpressionContext::FilterAnd + : ExpressionContext::FilterAndWithHavingClause; + $expression = $expression->collect($finalContext); $joins = $expression->joins; $groupBy = $expression->groupBy; - if ($expression->expression !== null) { + if ($expression->expression !== null && $expression->args !== []) { $this->queryBuilder->andWhere($expression->expression, ...$expression->args); } - if ($expression->havingExpression !== null) { + if ($expression->havingExpression !== null && $expression->havingArgs !== []) { $this->queryBuilder->andHaving($expression->havingExpression, ...$expression->havingArgs); } if ($this->mapper->getDatabasePlatform()->getName() === MySqlPlatform::NAME) { diff --git a/src/Collection/Expression/ExpressionContext.php b/src/Collection/Expression/ExpressionContext.php index c9e00655..6e97df59 100644 --- a/src/Collection/Expression/ExpressionContext.php +++ b/src/Collection/Expression/ExpressionContext.php @@ -4,6 +4,8 @@ /** + * @internal + * * Determines if the expression is processed for AND subtree, OR subtree or as a pure expression, * e.g., a sorting expression. * @@ -13,5 +15,7 @@ enum ExpressionContext { case FilterAnd; case FilterOr; + case FilterAndWithHavingClause; + case FilterOrWithHavingClause; case ValueExpression; } diff --git a/src/Collection/Functions/BaseCompareFunction.php b/src/Collection/Functions/BaseCompareFunction.php index 71949e87..7b2a5a43 100644 --- a/src/Collection/Functions/BaseCompareFunction.php +++ b/src/Collection/Functions/BaseCompareFunction.php @@ -5,12 +5,12 @@ use Nextras\Dbal\QueryBuilder\QueryBuilder; use Nextras\Orm\Collection\Aggregations\Aggregator; -use Nextras\Orm\Collection\Expression\ExpressionContext; use Nextras\Orm\Collection\Functions\Result\ArrayExpressionResult; use Nextras\Orm\Collection\Functions\Result\DbalExpressionResult; use Nextras\Orm\Collection\Helpers\ArrayCollectionHelper; use Nextras\Orm\Collection\Helpers\DbalQueryBuilderHelper; use Nextras\Orm\Entity\IEntity; +use function array_map; use function assert; use function count; @@ -34,12 +34,7 @@ public function processArrayExpression( } if ($valueReference->aggregator !== null) { - $values = array_map( - function ($value) use ($targetValue): bool { - return $this->evaluateInPhp($value, $targetValue); - }, - $valueReference->value, - ); + $values = $this->multiEvaluateInPhp($valueReference->value, $targetValue); return new ArrayExpressionResult( value: $values, aggregator: $valueReference->aggregator, @@ -59,13 +54,12 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { assert(count($args) === 2); - $expression = $helper->processExpression($builder, $args[0], $context, $aggregator); + $expression = $helper->processExpression($builder, $args[0], $aggregator); if ($expression->valueNormalizer !== null) { $cb = $expression->valueNormalizer; @@ -81,6 +75,21 @@ public function processDbalExpression( abstract protected function evaluateInPhp(mixed $sourceValue, mixed $targetValue): bool; + /** + * @param array $values + * @return array + */ + protected function multiEvaluateInPhp(array $values, mixed $targetValue): array + { + return array_map( + function ($value) use ($targetValue): bool { + return $this->evaluateInPhp($value, $targetValue); + }, + $values, + ); + } + + /** * @param literal-string|array|null $modifier */ diff --git a/src/Collection/Functions/BaseNumericAggregateFunction.php b/src/Collection/Functions/BaseNumericAggregateFunction.php index 811f9e3e..6837d7b5 100644 --- a/src/Collection/Functions/BaseNumericAggregateFunction.php +++ b/src/Collection/Functions/BaseNumericAggregateFunction.php @@ -54,7 +54,6 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { @@ -64,7 +63,7 @@ public function processDbalExpression( throw new InvalidStateException("Cannot apply two aggregations simultaneously."); } - return $helper->processExpression($builder, $args[0], $context, $this->aggregator) + return $helper->processExpression($builder, $args[0], $this->aggregator) ->applyAggregator(ExpressionContext::ValueExpression); } } diff --git a/src/Collection/Functions/CollectionFunction.php b/src/Collection/Functions/CollectionFunction.php index cad2b1b1..5f05df69 100644 --- a/src/Collection/Functions/CollectionFunction.php +++ b/src/Collection/Functions/CollectionFunction.php @@ -46,7 +46,6 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult; } diff --git a/src/Collection/Functions/CompareEqualsFunction.php b/src/Collection/Functions/CompareEqualsFunction.php index 5fd39525..414577fb 100644 --- a/src/Collection/Functions/CompareEqualsFunction.php +++ b/src/Collection/Functions/CompareEqualsFunction.php @@ -22,6 +22,15 @@ protected function evaluateInPhp(mixed $sourceValue, mixed $targetValue): bool } + protected function multiEvaluateInPhp(array $values, mixed $targetValue): array + { + if ($targetValue === null && $values === []) { + return [true]; + } + return parent::multiEvaluateInPhp($values, $targetValue); + } + + protected function evaluateInDb( DbalExpressionResult $expression, mixed $value, diff --git a/src/Collection/Functions/CompareLikeFunction.php b/src/Collection/Functions/CompareLikeFunction.php index 03b8adc5..961126c8 100644 --- a/src/Collection/Functions/CompareLikeFunction.php +++ b/src/Collection/Functions/CompareLikeFunction.php @@ -64,13 +64,12 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { assert(count($args) === 2); - $expression = $helper->processExpression($builder, $args[0], $context, $aggregator); + $expression = $helper->processExpression($builder, $args[0], $aggregator); $likeExpression = $args[1]; assert($likeExpression instanceof LikeExpression); diff --git a/src/Collection/Functions/ConjunctionOperatorFunction.php b/src/Collection/Functions/ConjunctionOperatorFunction.php index 425b0c56..b46ccc8b 100644 --- a/src/Collection/Functions/ConjunctionOperatorFunction.php +++ b/src/Collection/Functions/ConjunctionOperatorFunction.php @@ -103,7 +103,6 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { @@ -112,7 +111,6 @@ public function processDbalExpression( helper: $helper, builder: $builder, args: $args, - context: $context, aggregator: $aggregator, ); } diff --git a/src/Collection/Functions/DisjunctionOperatorFunction.php b/src/Collection/Functions/DisjunctionOperatorFunction.php index fbaaeaef..dfa4632c 100644 --- a/src/Collection/Functions/DisjunctionOperatorFunction.php +++ b/src/Collection/Functions/DisjunctionOperatorFunction.php @@ -96,7 +96,6 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { @@ -105,7 +104,6 @@ public function processDbalExpression( helper: $helper, builder: $builder, args: $args, - context: ExpressionContext::FilterOr, aggregator: $aggregator, ); } diff --git a/src/Collection/Functions/FetchPropertyFunction.php b/src/Collection/Functions/FetchPropertyFunction.php index 7fd30db2..78d53282 100644 --- a/src/Collection/Functions/FetchPropertyFunction.php +++ b/src/Collection/Functions/FetchPropertyFunction.php @@ -159,7 +159,6 @@ public function processDbalExpression( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator = null, ): DbalExpressionResult { diff --git a/src/Collection/Functions/JunctionFunctionTrait.php b/src/Collection/Functions/JunctionFunctionTrait.php index a17be974..e9e37a06 100644 --- a/src/Collection/Functions/JunctionFunctionTrait.php +++ b/src/Collection/Functions/JunctionFunctionTrait.php @@ -61,59 +61,76 @@ protected function processQueryBuilderExpressionWithModifier( DbalQueryBuilderHelper $helper, QueryBuilder $builder, array $args, - ExpressionContext $context, ?Aggregator $aggregator, ): DbalExpressionResult { - $processedArgs = []; - $processedHavingArgs = []; - $joins = []; - $groupBy = []; - $columns = []; - [$normalized, $newAggregator] = $this->normalizeFunctions($args); if ($newAggregator !== null) { if ($aggregator !== null) throw new InvalidStateException("Cannot apply two aggregations simultaneously."); $aggregator = $newAggregator; } + $requiresHaving = false; + $expressions = []; foreach ($normalized as $collectionFunctionArgs) { - $expression = $helper->processExpression($builder, $collectionFunctionArgs, $context, $aggregator); - $expression = $expression->applyAggregator($builder, $context); - $whereArgs = $expression->getArgsForExpansion(); - if ($whereArgs !== []) { - $processedArgs[] = $whereArgs; - } - $havingArgs = $expression->getHavingArgsForExpansion(); - if ($havingArgs !== []) { - $processedHavingArgs[] = $havingArgs; + $expressions[] = $expression = $helper->processExpression($builder, $collectionFunctionArgs, $aggregator); + if ($expression->havingExpression !== null) { + $requiresHaving = true; } - $joins = array_merge($joins, $expression->joins); - $groupBy = array_merge($groupBy, $expression->groupBy); - $columns = array_merge($columns, $expression->columns); } - if ($context === ExpressionContext::FilterOr && $processedHavingArgs !== []) { - // move all where expressions to HAVING clause - return new DbalExpressionResult( - expression: null, - args: [], - joins: $helper->mergeJoins($dbalModifier, $joins), - groupBy: array_merge($groupBy, $columns), - havingExpression: $dbalModifier, - havingArgs: [array_merge($processedArgs, $processedHavingArgs)], - columns: [], - ); - } else { - return new DbalExpressionResult( - expression: $processedArgs === [] ? null : $dbalModifier, - args: $processedArgs === [] ? [] : [$processedArgs], - joins: $helper->mergeJoins($dbalModifier, $joins), - groupBy: $groupBy, - havingExpression: $processedHavingArgs === [] ? null : $dbalModifier, - havingArgs: $processedHavingArgs === [] ? [] : [$processedHavingArgs], - columns: $columns, - ); - } + return new DbalExpressionResult( + expression: $dbalModifier, + args: $expressions, + havingExpression: $requiresHaving ? $dbalModifier : null, + collectCallback: function (ExpressionContext $context) use ($helper, $dbalModifier) { + /** @var DbalExpressionResult $this */ + + $processedArgs = []; + $processedHavingArgs = []; + $joins = []; + $groupBy = []; + $columns = []; + + if ($dbalModifier === '%or') { + if ($context === ExpressionContext::FilterAnd) { + $finalContext = ExpressionContext::FilterOr; + } elseif ($context === ExpressionContext::FilterAndWithHavingClause) { + $finalContext = ExpressionContext::FilterOrWithHavingClause; + } else { + $finalContext = $context; + } + } else { + $finalContext = $context; + } + + foreach ($this->args as $arg) { + assert($arg instanceof DbalExpressionResult); + $expression = $arg->collect($finalContext)->applyAggregator($finalContext); + + $whereArgs = $expression->getArgsForExpansion(); + if ($whereArgs !== []) { + $processedArgs[] = $whereArgs; + } + $havingArgs = $expression->getHavingArgsForExpansion(); + if ($havingArgs !== []) { + $processedHavingArgs[] = $havingArgs; + } + $joins = array_merge($joins, $expression->joins); + $groupBy = array_merge($groupBy, $expression->groupBy); + $columns = array_merge($columns, $expression->columns); + } + + return new DbalExpressionResult( + expression: $processedArgs === [] ? null : $dbalModifier, + args: $processedArgs === [] ? [] : [$processedArgs], + joins: $helper->mergeJoins($dbalModifier, $joins), + groupBy: $groupBy, + havingExpression: $processedHavingArgs === [] ? null : $dbalModifier, + havingArgs: $processedHavingArgs === [] ? [] : [$processedHavingArgs], + columns: $columns, + ); + }, + ); } } diff --git a/src/Collection/Functions/Result/DbalExpressionResult.php b/src/Collection/Functions/Result/DbalExpressionResult.php index b1bf07f0..44a4bcd3 100644 --- a/src/Collection/Functions/Result/DbalExpressionResult.php +++ b/src/Collection/Functions/Result/DbalExpressionResult.php @@ -3,11 +3,13 @@ namespace Nextras\Orm\Collection\Functions\Result; +use Closure; use Nextras\Dbal\Platforms\Data\Fqn; use Nextras\Orm\Collection\Aggregations\Aggregator; use Nextras\Orm\Collection\Expression\ExpressionContext; use Nextras\Orm\Entity\Reflection\PropertyMetadata; use Nextras\Orm\Exception\InvalidStateException; +use function array_merge; use function array_unshift; use function array_values; @@ -42,6 +44,8 @@ class DbalExpressionResult * @param PropertyMetadata|null $propertyMetadata Reference to backing property of the expression. If null, the expression is no more a simple property expression. * @param (callable(mixed): mixed)|null $valueNormalizer Normalizes the value for better PHP comparison, it considers the backing property type. * @param literal-string|list|null $dbalModifier Dbal modifier for particular column. Array if multi-column. Null value means expression is a general expression. + * @param (Closure(ExpressionContext): DbalExpressionResult)|null $collectCallback + * @param-closure-this DbalExpressionResult $collectCallback */ public function __construct( public readonly string|null $expression, @@ -55,6 +59,7 @@ public function __construct( public readonly ?PropertyMetadata $propertyMetadata = null, ?callable $valueNormalizer = null, public readonly string|array|null $dbalModifier = null, + public readonly Closure|null $collectCallback = null, ) { $this->valueNormalizer = $valueNormalizer; @@ -160,6 +165,41 @@ public function withHavingArgs(string $havingExpression, array $havingArgs): Dba } + public function collect(ExpressionContext $context): DbalExpressionResult + { + if ($this->collectCallback !== null) { + $collectFun = $this->collectCallback->bindTo($this); + return $collectFun($context); + } + + // When in OR expression with HAVING clause, lift simple non aggregated conditions + // to the HAVING clause. + // For aggregated expression, it is the responsibility of the aggregator. + if ( + $context === ExpressionContext::FilterOrWithHavingClause + && $this->expression !== null + && $this->aggregator === null + ) { + return new DbalExpressionResult( + expression: null, + args: [], + joins: $this->joins, + groupBy: array_merge($this->groupBy, $this->columns), + havingExpression: $this->expression, + havingArgs: $this->args, + columns: [], + aggregator: $this->aggregator, + propertyMetadata: $this->propertyMetadata, + valueNormalizer: $this->valueNormalizer, + dbalModifier: $this->dbalModifier, + collectCallback: null, + ); + } + + return $this; + } + + /** * Applies the aggregator and returns modified expression result. */ diff --git a/src/Collection/Helpers/DbalQueryBuilderHelper.php b/src/Collection/Helpers/DbalQueryBuilderHelper.php index 6c6cbc76..39095442 100644 --- a/src/Collection/Helpers/DbalQueryBuilderHelper.php +++ b/src/Collection/Helpers/DbalQueryBuilderHelper.php @@ -8,7 +8,6 @@ use Nextras\Dbal\Platforms\Data\Fqn; use Nextras\Dbal\QueryBuilder\QueryBuilder; use Nextras\Orm\Collection\Aggregations\Aggregator; -use Nextras\Orm\Collection\Expression\ExpressionContext; use Nextras\Orm\Collection\Functions\ConjunctionOperatorFunction; use Nextras\Orm\Collection\Functions\FetchPropertyFunction; use Nextras\Orm\Collection\Functions\Result\DbalExpressionResult; @@ -76,7 +75,6 @@ public function __construct( public function processExpression( QueryBuilder $builder, array|string $expression, - ExpressionContext $context, ?Aggregator $aggregator, ): DbalExpressionResult { @@ -88,7 +86,7 @@ public function processExpression( } $collectionFunction = $this->repository->getCollectionFunction($function); - return $collectionFunction->processDbalExpression($this, $builder, $expression, $context, $aggregator); + return $collectionFunction->processDbalExpression($this, $builder, $expression, $aggregator); } diff --git a/tests/cases/integration/Collection/Functions/FetchPropertyFunctionTest.php b/tests/cases/integration/Collection/Functions/FetchPropertyFunctionTest.php index 6eb332b2..b161ec46 100644 --- a/tests/cases/integration/Collection/Functions/FetchPropertyFunctionTest.php +++ b/tests/cases/integration/Collection/Functions/FetchPropertyFunctionTest.php @@ -10,7 +10,6 @@ use Nextras\Dbal\Platforms\Data\Fqn; use Nextras\Orm\Collection\Aggregations\AnyAggregator; -use Nextras\Orm\Collection\Expression\ExpressionContext; use Nextras\Orm\Collection\Functions\FetchPropertyFunction; use Nextras\Orm\Collection\Helpers\DbalQueryBuilderHelper; use Nextras\Orm\Mapper\Dbal\DbalMapper; @@ -40,7 +39,6 @@ public function testManyHasOneJoin(): void $helper, $builder, ['author->name'], - ExpressionContext::FilterAnd, ); Assert::count(0, $expression->groupBy); Assert::count(1, $expression->joins); @@ -63,7 +61,6 @@ public function testOneHasManyJoin(): void $helper, $builder, ['books->title'], - ExpressionContext::FilterAnd, ); if ($this->section === Helper::SECTION_MSSQL) { Assert::count(5, $expression->groupBy); // contains additional columns from SELECT clause @@ -80,7 +77,6 @@ public function testOneHasManyJoin(): void $helper, $builder, ['books->title'], - ExpressionContext::FilterAnd, new AnyAggregator('any2'), ); if ($this->section === Helper::SECTION_MSSQL) { @@ -109,7 +105,6 @@ public function testOneHasOneJoin(): void $helper, $builder, ['book->title'], - ExpressionContext::FilterAnd, ); Assert::count(0, $expression->groupBy); Assert::count(1, $expression->joins); diff --git a/tests/cases/integration/Collection/collection.aggregation.phpt b/tests/cases/integration/Collection/collection.aggregation.phpt index ded7449f..3a3720c2 100644 --- a/tests/cases/integration/Collection/collection.aggregation.phpt +++ b/tests/cases/integration/Collection/collection.aggregation.phpt @@ -185,6 +185,35 @@ class CollectionAggregationTest extends DataTestCase Assert::same(2, $users->count()); Assert::same(2, $users->countStored()); } + + + public function testMovingPrimaryTableConditionToWhenClause(): void + { + $books = $this->orm->books->findBy([ + ICollection::OR, + ['title' => 'Book 1'], + [CompareGreaterThanFunction::class, [CountAggregateFunction::class, 'tags->id'], 0], + ]); + Assert::same(3, $books->count()); // book #1, #2, #3 + + $books = $this->orm->books->findBy([ + ICollection::AND, + ['title' => 'Book 1'], + [CompareGreaterThanFunction::class, [CountAggregateFunction::class, 'tags->id'], 0], + ]); + Assert::same(1, $books->count()); // book #1 + } + + + public function testProperGroupByWhenLiftingNonAggregatedJoinCondition(): void + { + $books = $this->orm->books->findBy([ + ICollection::OR, + ['author->name' => 'Writer 1'], + [CompareGreaterThanFunction::class, [CountAggregateFunction::class, 'tags->id'], 0], + ]); + Assert::same(3, $books->count()); // book #1, #2, #3 + } } diff --git a/tests/cases/integration/Relationships/relationships.oneHasMany.phpt b/tests/cases/integration/Relationships/relationships.oneHasMany.phpt index be5936ea..db67ab68 100644 --- a/tests/cases/integration/Relationships/relationships.oneHasMany.phpt +++ b/tests/cases/integration/Relationships/relationships.oneHasMany.phpt @@ -342,12 +342,12 @@ class RelationshipOneHasManyTest extends DataTestCase { $books = $this->orm->books->findBy([ ICollection::OR, - ['tags->id' => [1]], - ['tags->id' => null], // no match + ['tags->id' => [1]], // #1 + ['tags->id' => null], // matches a book without tags (#4) ]); - Assert::same(1, $books->countStored()); - Assert::same(1, $books->count()); + Assert::same(2, $books->countStored()); + Assert::same(2, $books->count()); } diff --git a/tests/cases/unit/Mapper/Dbal/DbalValueOperatorFunctionTest.phpt b/tests/cases/unit/Mapper/Dbal/DbalValueOperatorFunctionTest.phpt index b8b4d2aa..e11bc89b 100644 --- a/tests/cases/unit/Mapper/Dbal/DbalValueOperatorFunctionTest.phpt +++ b/tests/cases/unit/Mapper/Dbal/DbalValueOperatorFunctionTest.phpt @@ -41,7 +41,7 @@ class DbalValueOperatorFunctionTest extends TestCase Assert::same( $expected, - $function->processDbalExpression($helper, $builder, $expr, ExpressionContext::ValueExpression)->getArgsForExpansion() + $function->processDbalExpression($helper, $builder, $expr)->getArgsForExpansion() ); } diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testMovingPrimaryTableConditionToWhenClause.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testMovingPrimaryTableConditionToWhenClause.sql new file mode 100644 index 00000000..00a69109 --- /dev/null +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testMovingPrimaryTableConditionToWhenClause.sql @@ -0,0 +1,35 @@ +SELECT + "books".* +FROM + "books" AS "books" + LEFT JOIN "books_x_tags" AS "books_x_tags__COUNT" ON ( + "books"."id" = "books_x_tags__COUNT"."book_id" + ) + LEFT JOIN "tags" AS "tags__COUNT" ON ( + "books_x_tags__COUNT"."tag_id" = "tags__COUNT"."id" + ) +GROUP BY + "books"."title", + "books"."id" +HAVING + ("books"."title" = 'Book 1') + OR ( + COUNT("tags__COUNT"."id") > 0 + ); + +SELECT + "books".* +FROM + "books" AS "books" + LEFT JOIN "books_x_tags" AS "books_x_tags__COUNT" ON ( + "books"."id" = "books_x_tags__COUNT"."book_id" + ) + LEFT JOIN "tags" AS "tags__COUNT" ON ( + "books_x_tags__COUNT"."tag_id" = "tags__COUNT"."id" + ) +WHERE + "books"."title" = 'Book 1' +GROUP BY + "books"."id" +HAVING + COUNT("tags__COUNT"."id") > 0; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testProperGroupByWhenLiftingNonAggregatedJoinCondition.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testProperGroupByWhenLiftingNonAggregatedJoinCondition.sql new file mode 100644 index 00000000..68826eac --- /dev/null +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationTest_testProperGroupByWhenLiftingNonAggregatedJoinCondition.sql @@ -0,0 +1,21 @@ +SELECT + "books".* +FROM + "books" AS "books" + LEFT JOIN "public"."authors" AS "author" ON ( + "books"."author_id" = "author"."id" + ) + LEFT JOIN "books_x_tags" AS "books_x_tags__COUNT" ON ( + "books"."id" = "books_x_tags__COUNT"."book_id" + ) + LEFT JOIN "tags" AS "tags__COUNT" ON ( + "books_x_tags__COUNT"."tag_id" = "tags__COUNT"."id" + ) +GROUP BY + "author"."name", + "books"."id" +HAVING + ("author"."name" = 'Writer 1') + OR ( + COUNT("tags__COUNT"."id") > 0 + ); diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionHavingTest_testHavingWithSameNamedColumnsInGroupBy.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionHavingTest_testHavingWithSameNamedColumnsInGroupBy.sql index d21e0f38..3cea0c14 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionHavingTest_testHavingWithSameNamedColumnsInGroupBy.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionHavingTest_testHavingWithSameNamedColumnsInGroupBy.sql @@ -6,10 +6,7 @@ FROM "books"."id" = "books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "tags_any" ON ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" = 1 + "books_x_tags_any"."tag_id" = "tags_any"."id" ) LEFT JOIN "public"."authors" AS "author" ON ( "books"."author_id" = "author"."id" @@ -17,15 +14,11 @@ FROM LEFT JOIN "publishers" AS "publisher" ON ( "books"."publisher_id" = "publisher"."publisher_id" ) -GROUP BY - "books"."id", - "author"."name", - "publisher"."name" -HAVING - ("author"."name" = 'Writer 2') +WHERE + ("tags_any"."id" = 1) + OR ("author"."name" = 'Writer 2') OR ( "publisher"."name" = 'Nextras publisher C' ) - OR ( - COUNT("tags_any"."id") > 0 - ); +GROUP BY + "books"."id"; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasManyTest_testJoinAcrossDifferentPaths.sql b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasManyTest_testJoinAcrossDifferentPaths.sql index f0b078a2..d8dd3ab3 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasManyTest_testJoinAcrossDifferentPaths.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasManyTest_testJoinAcrossDifferentPaths.sql @@ -6,10 +6,7 @@ FROM "books"."id" = "books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "tags_any" ON ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."name" = 'Tag 1' + "books_x_tags_any"."tag_id" = "tags_any"."id" ) LEFT JOIN "books" AS "nextPart" ON ( "books"."next_part" = "nextPart"."id" @@ -18,20 +15,15 @@ FROM "nextPart"."id" = "nextPart_books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "nextPart_tags_any" ON ( - ( - "nextPart_books_x_tags_any"."tag_id" = "nextPart_tags_any"."id" - ) - AND "nextPart_tags_any"."name" = 'Tag 3' - ) -GROUP BY - "books"."id" -HAVING - ( - COUNT("tags_any"."id") > 0 + "nextPart_books_x_tags_any"."tag_id" = "nextPart_tags_any"."id" ) +WHERE + ("tags_any"."name" = 'Tag 1') OR ( - COUNT("nextPart_tags_any"."id") > 0 + "nextPart_tags_any"."name" = 'Tag 3' ) +GROUP BY + "books"."id" ORDER BY "books"."id" ASC; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testCountStoredOnOneHasManyRelationshipCondition.sql b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testCountStoredOnOneHasManyRelationshipCondition.sql index 2299c847..c76bbfa6 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testCountStoredOnOneHasManyRelationshipCondition.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testCountStoredOnOneHasManyRelationshipCondition.sql @@ -40,21 +40,18 @@ FROM "books"."id" = "books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "tags_any" ON ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" = 1 + "books_x_tags_any"."tag_id" = "tags_any"."id" ) WHERE - "books"."publisher_id" IN (1) - GROUP BY - "books"."id", - "books"."title" - HAVING - ("books"."title" = 'Book 1') - OR ( - COUNT("tags_any"."id") > 0 + ( + ("books"."title" = 'Book 1') + OR ("tags_any"."id" = 1) ) + AND ( + "books"."publisher_id" IN (1) + ) + GROUP BY + "books"."id" ) AS "temp" GROUP BY "publisher_id"; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testSameTableJoinWithImplicitAggregation.sql b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testSameTableJoinWithImplicitAggregation.sql index aee1600d..b55b98a9 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testSameTableJoinWithImplicitAggregation.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testSameTableJoinWithImplicitAggregation.sql @@ -10,28 +10,15 @@ FROM "books"."id" = "books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "tags_any" ON ( - ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" IN (1) - ) - OR ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" IS NULL - ) + "books_x_tags_any"."tag_id" = "tags_any"."id" ) - GROUP BY - "books"."id" - HAVING + WHERE ( - COUNT("tags_any"."id") > 0 - ) - OR ( - COUNT("tags_any"."id") > 0 + "tags_any"."id" IN (1) ) + OR ("tags_any"."id" IS NULL) + GROUP BY + "books"."id" ) temp; SELECT @@ -42,25 +29,12 @@ FROM "books"."id" = "books_x_tags_any"."book_id" ) LEFT JOIN "tags" AS "tags_any" ON ( - ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" IN (1) - ) - OR ( - ( - "books_x_tags_any"."tag_id" = "tags_any"."id" - ) - AND "tags_any"."id" IS NULL - ) + "books_x_tags_any"."tag_id" = "tags_any"."id" ) -GROUP BY - "books"."id" -HAVING +WHERE ( - COUNT("tags_any"."id") > 0 + "tags_any"."id" IN (1) ) - OR ( - COUNT("tags_any"."id") > 0 - ); + OR ("tags_any"."id" IS NULL) +GROUP BY + "books"."id";