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

Add CTE support to select in QueryBuilder #6621

Open
wants to merge 6 commits into
base: 4.3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
33 changes: 33 additions & 0 deletions docs/en/reference/query-builder.rst
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,39 @@ or QueryBuilder instances to one of the following methods:
->orderBy('field', 'DESC')
->setMaxResults(100);

Common Table Expressions
~~~~~~~~~~~

To define Common Table Expressions (CTEs) that can be used in select query.

* ``with(string $name, string|QueryBuilder $queryBuilder, ?array $columns = null)``

.. code-block:: php

<?php

$cteQueryBuilder1
->select('id')
->from('table_a')
->where('id = :id')
->setParameter('id', 1);

$cteQueryBuilder2
->select('id')
->from('table_b');

$queryBuilder
->with('cte_a', $cteQueryBuilder1)
->with('cte_b', $cteQueryBuilder2)
->select('id')
->from('cte_b', 'b')
->join('b', 'cte_a', 'a', 'a.id = b.id');

Multiple CTEs can be defined by calling the with method multiple times.

Parameters used in a CTE should be defined directly in the QueryBuilder of the CTE.
This way, the CTE builders are naturally composable.

Building Expressions
--------------------

Expand Down
6 changes: 6 additions & 0 deletions src/Platforms/AbstractPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Doctrine\DBAL\SQL\Builder\DefaultUnionSQLBuilder;
use Doctrine\DBAL\SQL\Builder\SelectSQLBuilder;
use Doctrine\DBAL\SQL\Builder\UnionSQLBuilder;
use Doctrine\DBAL\SQL\Builder\WithSQLBuilder;
use Doctrine\DBAL\SQL\Parser;
use Doctrine\DBAL\TransactionIsolationLevel;
use Doctrine\DBAL\Types;
Expand Down Expand Up @@ -802,6 +803,11 @@ public function createUnionSQLBuilder(): UnionSQLBuilder
return new DefaultUnionSQLBuilder($this);
}

public function createWithSQLBuilder(): WithSQLBuilder
{
return new WithSQLBuilder();
}

/**
* @internal
*
Expand Down
6 changes: 6 additions & 0 deletions src/Platforms/MySQL80Platform.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Doctrine\DBAL\Platforms\Keywords\KeywordList;
use Doctrine\DBAL\Platforms\Keywords\MySQL80Keywords;
use Doctrine\DBAL\SQL\Builder\SelectSQLBuilder;
use Doctrine\DBAL\SQL\Builder\WithSQLBuilder;
use Doctrine\Deprecations\Deprecation;

/**
Expand All @@ -32,4 +33,9 @@ public function createSelectSQLBuilder(): SelectSQLBuilder
{
return AbstractPlatform::createSelectSQLBuilder();
}

public function createWithSQLBuilder(): WithSQLBuilder
{
return AbstractPlatform::createWithSQLBuilder();
}
}
7 changes: 7 additions & 0 deletions src/Platforms/MySQLPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@

namespace Doctrine\DBAL\Platforms;

use Doctrine\DBAL\Platforms\Exception\NotSupported;
use Doctrine\DBAL\Platforms\Keywords\KeywordList;
use Doctrine\DBAL\Platforms\Keywords\MySQLKeywords;
use Doctrine\DBAL\Schema\Index;
use Doctrine\DBAL\SQL\Builder\WithSQLBuilder;
use Doctrine\DBAL\Types\BlobType;
use Doctrine\DBAL\Types\TextType;
use Doctrine\Deprecations\Deprecation;
Expand Down Expand Up @@ -35,6 +37,11 @@ public function getDefaultValueDeclarationSQL(array $column): string
return parent::getDefaultValueDeclarationSQL($column);
}

public function createWithSQLBuilder(): WithSQLBuilder
{
throw NotSupported::new(__METHOD__);
}

/**
* {@inheritDoc}
*/
Expand Down
23 changes: 23 additions & 0 deletions src/Query/CommonTableExpression.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Query;

use function count;
use function sprintf;

/** @internal */
final class CommonTableExpression
{
/** @param string[]|null $columns */
public function __construct(
public readonly string $name,
public readonly string|QueryBuilder $query,
public readonly ?array $columns,
) {
if ($columns !== null && count($columns) === 0) {
throw new QueryException(sprintf('Columns defined in CTE "%s" should not be an empty array.', $name));
}
}
}
86 changes: 83 additions & 3 deletions src/Query/QueryBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,13 @@
*/
private array $unionParts = [];

/**
* The common table expression parts.
*
* @var CommonTableExpression[]
*/
private array $commonTableExpressions = [];

/**
* The query cache profile used for caching results.
*/
Expand Down Expand Up @@ -302,14 +309,47 @@
*/
public function executeQuery(): Result
{
[$params, $types] = $this->buildParametersAndTypes();

return $this->connection->executeQuery(
$this->getSQL(),
$this->params,
$this->types,
$params,
$types,
$this->resultCacheProfile,
);
}

/**
* Build then return parameters and types for the query.
*
* @return array{
* list<mixed>|array<string, mixed>,
* WrapperParameterTypeArray,
* } The parameters and types for the query.
*/
private function buildParametersAndTypes(): array
{
$cteParams = $cteParamTypes = [];

foreach ($this->commonTableExpressions as $cte) {
if (! $cte->query instanceof self) {
continue;

Check warning on line 336 in src/Query/QueryBuilder.php

View check run for this annotation

Codecov / codecov/patch

src/Query/QueryBuilder.php#L336

Added line #L336 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Code coverage complains about this check not beeing covered.

I'd say additional tests using plain sql strings as parts instead of QueryBuilder instances should be added anyway, and would cover this line here two (guessing).

Copy link
Author

Choose a reason for hiding this comment

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

I've added a test with a sql string as cte query

}

$cteParams = array_merge($cteParams, $cte->query->params);
Copy link
Contributor

Choose a reason for hiding this comment

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

This overrides named placehoders, if two part QueryBuilder instances auto creates named placeholders, specially when QueryBuilder->createNamedParameter() are used with automatic name generation based on a counter:

        $cteQueryBuilder1 = $this->connection->createQueryBuilder();
        $cteQueryBuilder1->select('id AS virtual_id')
            ->from('for_update')
            ->where('virtual_id = ' . $cteQueryBuilder1->createNamedParameter(123, ParameterType::INTEGER));

        $cteQueryBuilder2 = $this->connection->createQueryBuilder();
        $cteQueryBuilder2->select('id AS virtual_id')
            ->from('cte_a')
            ->where('virtual_id = ' . $cteQueryBuilder2->createNamedParameter(987, ParameterType::INTEGER));

would merge them in a weired way. Additionally, the tests related to named placeholder only covers manual defining them with setting parameter, not using the create methods along with autogeneration and using in two sub-querybuilder instances which gets merged.

/**
* Creates a new named parameter and bind the value $value to it.
*
* This method provides a shortcut for {@see Statement::bindValue()}
* when using prepared statements.
*
* The parameter $value specifies the value that you want to bind. If
* $placeholder is not provided createNamedParameter() will automatically
* create a placeholder for you. An automatic placeholder will be of the
* name ':dcValue1', ':dcValue2' etc.
*
* Example:
* <code>
* $value = 2;
* $q->eq( 'id', $q->createNamedParameter( $value ) );
* $stmt = $q->executeQuery(); // executed with 'id = 2'
* </code>
*
* @link http://www.zetacomponents.org
*
* @param string|null $placeHolder The name to bind with. The string must start with a colon ':'.
*
* @return string the placeholder name used.
*/
public function createNamedParameter(
mixed $value,
string|ParameterType|Type|ArrayParameterType $type = ParameterType::STRING,
?string $placeHolder = null,
): string {
if ($placeHolder === null) {
$this->boundCounter++;
$placeHolder = ':dcValue' . $this->boundCounter;
}
$this->setParameter(substr($placeHolder, 1), $value, $type);
return $placeHolder;
}

Tests related to the placeholder merging and usage is not sufficient enough.

Copy link
Author

Choose a reason for hiding this comment

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

That does not work with default (auto) placeholder because of the boundCounter value which start to 1 for each CTE.
But it is ok when setting placeholder

->where($cteQueryBuilder1->expr()->eq(
    'id',
    $cteQueryBuilder1->createNamedParameter(1, ParameterType::INTEGER, ':id1'),
));

->where($cteQueryBuilder2->expr()->in(
    'id',
    $cteQueryBuilder2->createNamedParameter([1, 2], ArrayParameterType::INTEGER, ':id2'),
));     

I've added some tests for this

Copy link
Contributor

Choose a reason for hiding this comment

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

My whole point is, that starting this leads to more things some developers needs to know beside a simple "use the top QueryBuilder" instance. Not a fan of starting this and leave it half way broken and unfinished. But that is more a personal taste:

->where($cteQueryBuilder1->expr()->eq(
    'id',
    $cteQueryBuilder1->createNamedParameter(1, ParameterType::INTEGER, ':id'),
));

->where($cteQueryBuilder2->expr()->in(
    'id',
    $cteQueryBuilder2->createNamedParameter([1, 2], ArrayParameterType::INTEGER, ':id'),
));  

In that case database error should occor and be a little bit of help that something is off when merging the CTE parts on query execution.

->where($cteQueryBuilder1->expr()->eq(
    'id',
    $cteQueryBuilder1->createNamedParameter(1, ParameterType::INTEGER, ':id'),
));

->where($cteQueryBuilder2->expr()->in(
    'id',
    $cteQueryBuilder2->createNamedParameter(2, ParameterType::INTEGER, ':id'),
));  

would merge that and use 2 for both places for the same :id without any notices - and will give a hard time to debug and find this.

Askin me the whole merging should vanish again until it could be implemented completly in all parts and with a concept behind it. But that is not my decision.

Let's see what @derrabus would say.

Copy link
Author

@nio-dtp nio-dtp Dec 13, 2024

Choose a reason for hiding this comment

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

I agree, it could give a hard time to debug this. We could prevent this by adding a check during the merge process and reject a parameter's name that is already used.

$cteParamTypes = array_merge($cteParamTypes, $cte->query->types);
}

if (count($cteParams) === 0) {
return [$this->params, $this->types];
}

return [
array_merge($cteParams, $this->params),
array_merge($cteParamTypes, $this->types),
];
}

/**
* Executes an SQL statement and returns the number of affected rows.
*
Expand Down Expand Up @@ -557,6 +597,36 @@
return $this;
}

/**
* Add a Common Table Expression to be used for a select query.
*
* <code>
* // WITH cte_name AS (SELECT id AS column1 FROM table_a)
* $qb = $conn->createQueryBuilder()
* ->with('cte_name', 'SELECT id AS column1 FROM table_a');
*
* // WITH cte_name(column1) AS (SELECT id AS column1 FROM table_a)
* $qb = $conn->createQueryBuilder()
* ->with('cte_name', 'SELECT id AS column1 FROM table_a', ['column1']);
* </code>
*
* @param string $name The name of the CTE
* @param string[]|null $columns The optional columns list to select in the CTE.
morozov marked this conversation as resolved.
Show resolved Hide resolved
* If not provided, the columns are inferred from the CTE.
*
* @return $this This QueryBuilder instance.
*
* @throws QueryException Setting an empty array as columns is not allowed.
*/
public function with(string $name, string|QueryBuilder $part, ?array $columns = null): self
Copy link
Member

@morozov morozov Dec 21, 2024

Choose a reason for hiding this comment

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

Maybe if the outer builder will ignore the parameters bound to the inner one, it shouldn't accept QueryBuilder and should only accept string? This way, the caller will have to call toSql() on the inner builder and thereby acknowledge that its parameters won't be used.

{
$this->commonTableExpressions[] = new CommonTableExpression($name, $part, $columns);

$this->sql = null;

return $this;
}

/**
* Specifies an item that is to be returned in the query result.
* Replaces any previously specified selections, if any.
Expand Down Expand Up @@ -1266,7 +1336,15 @@
throw new QueryException('No SELECT expressions given. Please use select() or addSelect().');
}

return $this->connection->getDatabasePlatform()
$databasePlatform = $this->connection->getDatabasePlatform();
$selectParts = [];
if (count($this->commonTableExpressions) > 0) {
$selectParts[] = $databasePlatform
->createWithSQLBuilder()
->buildSQL(...$this->commonTableExpressions);
}

$selectParts[] = $databasePlatform
->createSelectSQLBuilder()
->buildSQL(
new SelectQuery(
Expand All @@ -1281,6 +1359,8 @@
$this->forUpdate,
),
);

return implode(' ', $selectParts);
}

/**
Expand Down
31 changes: 31 additions & 0 deletions src/SQL/Builder/WithSQLBuilder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\SQL\Builder;

use Doctrine\DBAL\Query\CommonTableExpression;

use function array_merge;
use function count;
use function implode;

final class WithSQLBuilder
{
public function buildSQL(CommonTableExpression $firstExpression, CommonTableExpression ...$otherExpressions): string
{
$cteParts = [];

foreach (array_merge([$firstExpression], $otherExpressions) as $part) {
$ctePart = [$part->name];
if ($part->columns !== null && count($part->columns) > 0) {
$ctePart[] = '(' . implode(', ', $part->columns) . ')';
}

$ctePart[] = ' AS (' . $part->query . ')';
$cteParts[] = implode('', $ctePart);
}

return 'WITH ' . implode(', ', $cteParts);
}
}
Loading