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

5.1 unbuffered queries #17649

Merged
merged 13 commits into from
May 11, 2024
13 changes: 9 additions & 4 deletions src/Database/Driver.php
Expand Up @@ -35,6 +35,7 @@
use Cake\Database\Schema\TableSchema;
use Cake\Database\Schema\TableSchemaInterface;
use Cake\Database\Statement\Statement;
use Closure;
use InvalidArgumentException;
use PDO;
use PDOException;
Expand Down Expand Up @@ -391,13 +392,17 @@ public function prepare(Query|string $query): StatementInterface
);
}

$typeMap = null;
if ($query instanceof SelectQuery && $query->isResultsCastingEnabled()) {
$typeMap = $query->getSelectTypeMap();
$decorators = [];
if ($query instanceof SelectQuery) {
$decorators = $query->getResultDecorators();
if ($query->isResultsCastingEnabled()) {
$typeConverter = new FieldTypeConverter($query->getSelectTypeMap(), $this);
array_unshift($decorators, Closure::fromCallable($typeConverter));
}
}

/** @var \Cake\Database\StatementInterface */
return new (static::STATEMENT_CLASS)($statement, $this, $typeMap);
return new (static::STATEMENT_CLASS)($statement, $this, $decorators);
}

/**
Expand Down
25 changes: 25 additions & 0 deletions src/Database/Driver/Mysql.php
Expand Up @@ -18,8 +18,11 @@

use Cake\Database\Driver;
use Cake\Database\DriverFeatureEnum;
use Cake\Database\Query;
use Cake\Database\Query\SelectQuery;
use Cake\Database\Schema\MysqlSchemaDialect;
use Cake\Database\Schema\SchemaDialect;
use Cake\Database\StatementInterface;
use PDO;

/**
Expand Down Expand Up @@ -157,6 +160,28 @@ public function connect(): void
}
}

/**
* @inheritDoc
*/
public function run(Query $query): StatementInterface
{
$statement = $this->prepare($query);
$query->getValueBinder()->attachTo($statement);

if ($query instanceof SelectQuery) {
try {
$this->getPdo()->setAttribute(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY, $query->isBufferedResultsEnabled());
Copy link
Member

Choose a reason for hiding this comment

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

I believe we did the the same in 4.x, so this should be ok. I'm wondering if setting the attribute per query is necessary or affects performance at all

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it's the same as 4.x, no idea about the performance implications.

Copy link
Member

Choose a reason for hiding this comment

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

Lets not overcomplicate things. If this worked in 4.x it will be good enough here too. If it creates a performance regression we can patch around it.

$this->executeStatement($statement);
} finally {
$this->getPdo()->setAttribute(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY, true);
}
} else {
$this->executeStatement($statement);
}

return $statement;
}

/**
* Returns whether php is able to use this driver for connecting to database
*
Expand Down
94 changes: 81 additions & 13 deletions src/Database/Query/SelectQuery.php
Expand Up @@ -74,7 +74,7 @@
* statement upon retrieval. Each one of the callback function will receive
* the row array as first argument.
*
* @var array<\Closure>
* @var list<\Closure>
*/
protected array $_resultDecorators = [];

Expand All @@ -85,6 +85,14 @@
*/
protected ?iterable $_results = null;

/**
* Boolean for tracking whether buffered results
* are enabled.
*
* @var bool
*/
protected bool $useBufferedResults = true;
ADmad marked this conversation as resolved.
Show resolved Hide resolved

/**
* The Type map for fields in the select clause
*
Expand All @@ -111,13 +119,6 @@
{
if ($this->_results === null || $this->_dirty) {
$this->_results = $this->execute()->fetchAll(StatementInterface::FETCH_TYPE_ASSOC);
if ($this->_resultDecorators) {
Copy link
Member

Choose a reason for hiding this comment

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

Moving this to the statement seems good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Statement was already decorating the records using the typemap so moving user provided decorators there too makes sense, so all decorations are now in one place.

foreach ($this->_results as &$row) {
foreach ($this->_resultDecorators as $decorator) {
$row = $decorator($row);
}
}
}
}

return $this->_results;
Expand Down Expand Up @@ -761,17 +762,21 @@
* iterated without having to call all() manually, thus making it look like
* a result set instead of the query itself.
*
* @return \Traversable

Check failure on line 765 in src/Database/Query/SelectQuery.php

View workflow job for this annotation

GitHub Actions / Coding Standard & Static Analysis

InvalidReturnType

src/Database/Query/SelectQuery.php:765:16: InvalidReturnType: The declared return type 'Traversable' for Cake\Database\Query\SelectQuery::getIterator is incorrect, got 'ArrayIterator<array-key, mixed>|Cake\Database\StatementInterface|Traversable<array-key|mixed, mixed>' (see https://psalm.dev/011)
*/
public function getIterator(): Traversable
{
/** @var \Traversable|array $results */
$results = $this->all();
if (is_array($results)) {
return new ArrayIterator($results);
if ($this->useBufferedResults) {
/** @var \Traversable|array $results */
$results = $this->all();
if (is_array($results)) {
return new ArrayIterator($results);
}

return $results;
}

return $results;
return $this->execute();

Check failure on line 779 in src/Database/Query/SelectQuery.php

View workflow job for this annotation

GitHub Actions / Coding Standard & Static Analysis

Method Cake\Database\Query\SelectQuery::getIterator() should return Traversable but returns Cake\Database\StatementInterface.

Check failure on line 779 in src/Database/Query/SelectQuery.php

View workflow job for this annotation

GitHub Actions / Coding Standard & Static Analysis

InvalidReturnStatement

src/Database/Query/SelectQuery.php:779:16: InvalidReturnStatement: The inferred type 'Cake\Database\StatementInterface' does not match the declared return type 'Traversable' for Cake\Database\Query\SelectQuery::getIterator (see https://psalm.dev/128)
}

/**
Expand Down Expand Up @@ -816,6 +821,69 @@
return $this;
}

/**
* Get result decorators.
*
* @return array
*/
public function getResultDecorators(): array
{
return $this->_resultDecorators;
}

/**
* Enables/Disables buffered results.
ADmad marked this conversation as resolved.
Show resolved Hide resolved
*
* When enabled the results returned by this query will be
* buffered. This enables you to iterate a result set multiple times, or
* both cache and iterate it.
*
* When disabled it will consume less memory as fetched results are not
* remembered for future iterations.
*
* @return $this
*/
public function enableBufferedResults()
{
$this->_dirty();
$this->useBufferedResults = true;

return $this;
}

/**
* Disables buffered results.
*
* Disabling buffering will consume less memory as fetched results are not
* remembered for future iterations.
*
* @return $this
*/
public function disableBufferedResults()
{
$this->_dirty();
$this->useBufferedResults = false;

return $this;
}

/**
* Returns whether buffered results are enabled/disabled.
*
* When enabled the results returned by this query will be
* buffered. This enables you to iterate a result set multiple times, or
* both cache and iterate it.
*
* When disabled it will consume less memory as fetched results are not
* remembered for future iterations.
*
* @return bool
*/
public function isBufferedResultsEnabled(): bool
othercorey marked this conversation as resolved.
Show resolved Hide resolved
{
return $this->useBufferedResults;
}

/**
* Sets the TypeMap class where the types for each of the fields in the
* select clause are stored.
Expand Down
52 changes: 30 additions & 22 deletions src/Database/Statement/Statement.php
Expand Up @@ -17,16 +17,16 @@
namespace Cake\Database\Statement;

use Cake\Database\Driver;
use Cake\Database\FieldTypeConverter;
use Cake\Database\StatementInterface;
use Cake\Database\TypeFactory;
use Cake\Database\TypeInterface;
use Cake\Database\TypeMap;
use Generator;
use InvalidArgumentException;
use IteratorAggregate;
use PDO;
use PDOStatement;

class Statement implements StatementInterface
class Statement implements StatementInterface, IteratorAggregate

Check failure on line 29 in src/Database/Statement/Statement.php

View workflow job for this annotation

GitHub Actions / Coding Standard & Static Analysis

MissingTemplateParam

src/Database/Statement/Statement.php:29:48: MissingTemplateParam: Cake\Database\Statement\Statement has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
{
/**
* @var array<string, int>
Expand All @@ -42,16 +42,6 @@
*/
protected Driver $_driver;

/**
* @var \PDOStatement
*/
protected PDOStatement $statement;

/**
* @var \Cake\Database\FieldTypeConverter|null
*/
protected ?FieldTypeConverter $typeConverter;

/**
* Cached bound parameters used for logging
*
Expand All @@ -62,16 +52,14 @@
/**
* @param \PDOStatement $statement PDO statement
* @param \Cake\Database\Driver $driver Database driver
* @param \Cake\Database\TypeMap|null $typeMap Results type map
* @param list<\Closure> $resultDecorators Results decorators
*/
public function __construct(
PDOStatement $statement,
protected PDOStatement $statement,
Driver $driver,
?TypeMap $typeMap = null,
protected array $resultDecorators = [],
) {
$this->_driver = $driver;
$this->statement = $statement;
$this->typeConverter = $typeMap !== null ? new FieldTypeConverter($typeMap, $driver) : null;
}

/**
Expand Down Expand Up @@ -170,8 +158,8 @@
return false;
}

if ($this->typeConverter !== null) {
return ($this->typeConverter)($row);
foreach ($this->resultDecorators as $decorator) {
$row = $decorator($row);
}

return $row;
Expand Down Expand Up @@ -206,8 +194,8 @@
$mode = $this->convertMode($mode);
$rows = $this->statement->fetchAll($mode);

if ($this->typeConverter !== null) {
return array_map($this->typeConverter, $rows);
foreach ($this->resultDecorators as $decorator) {
$rows = array_map($decorator, $rows);
}

return $rows;
Expand Down Expand Up @@ -297,4 +285,24 @@
{
return $this->statement->queryString;
}

/**
* Get the inner iterator
*
* @return \Generator
*/
public function getIterator(): Generator
{
$this->statement->setFetchMode(PDO::FETCH_ASSOC);

foreach ($this->statement as $row) {
foreach ($this->resultDecorators as $decorator) {
$row = $decorator($row);
}

yield $row;
}

$this->closeCursor();
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see how this and fetchAll() should co-exist. Won't the statement be empty if we call one or the other?

Copy link
Member Author

Choose a reason for hiding this comment

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

One needs to pick one of the options, either call fetchAll() or iterate the instance.

}