-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
5.1 unbuffered queries #17649
Changes from 12 commits
f691d99
f661b8f
cfeca32
7b84af7
47db92a
765615d
c22706c
6032873
a5f17eb
de5c56b
ee026f9
6b10a6c
5fec99a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,7 @@ class SelectQuery extends Query implements IteratorAggregate | |
* 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 = []; | ||
|
||
|
@@ -85,6 +85,14 @@ class SelectQuery extends Query implements IteratorAggregate | |
*/ | ||
protected ?iterable $_results = null; | ||
|
||
/** | ||
* Boolean for tracking whether buffered results | ||
* are enabled. | ||
* | ||
* @var bool | ||
*/ | ||
protected bool $bufferedResults = true; | ||
|
||
/** | ||
* The Type map for fields in the select clause | ||
* | ||
|
@@ -111,13 +119,6 @@ public function all(): iterable | |
{ | ||
if ($this->_results === null || $this->_dirty) { | ||
$this->_results = $this->execute()->fetchAll(StatementInterface::FETCH_TYPE_ASSOC); | ||
if ($this->_resultDecorators) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this to the statement seems good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
foreach ($this->_results as &$row) { | ||
foreach ($this->_resultDecorators as $decorator) { | ||
$row = $decorator($row); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return $this->_results; | ||
|
@@ -765,13 +766,17 @@ public function unionAll(Query|string $query, bool $overwrite = false) | |
*/ | ||
public function getIterator(): Traversable | ||
{ | ||
/** @var \Traversable|array $results */ | ||
$results = $this->all(); | ||
if (is_array($results)) { | ||
return new ArrayIterator($results); | ||
if ($this->bufferedResults) { | ||
/** @var \Traversable|array $results */ | ||
$results = $this->all(); | ||
if (is_array($results)) { | ||
return new ArrayIterator($results); | ||
} | ||
|
||
return $results; | ||
} | ||
|
||
return $results; | ||
return $this->execute(); | ||
} | ||
|
||
/** | ||
|
@@ -816,6 +821,69 @@ public function decorateResults(?Closure $callback, bool $overwrite = false) | |
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->bufferedResults = 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->bufferedResults = 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->bufferedResults; | ||
} | ||
|
||
/** | ||
* Sets the TypeMap class where the types for each of the fields in the | ||
* select clause are stored. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,10 @@ | |
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 PDO; | ||
use PDOStatement; | ||
|
@@ -42,16 +41,6 @@ class Statement implements StatementInterface | |
*/ | ||
protected Driver $_driver; | ||
|
||
/** | ||
* @var \PDOStatement | ||
*/ | ||
protected PDOStatement $statement; | ||
|
||
/** | ||
* @var \Cake\Database\FieldTypeConverter|null | ||
*/ | ||
protected ?FieldTypeConverter $typeConverter; | ||
|
||
/** | ||
* Cached bound parameters used for logging | ||
* | ||
|
@@ -62,16 +51,14 @@ class Statement implements StatementInterface | |
/** | ||
* @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; | ||
} | ||
|
||
/** | ||
|
@@ -170,8 +157,8 @@ public function fetch(string|int $mode = PDO::FETCH_NUM): mixed | |
return false; | ||
} | ||
|
||
if ($this->typeConverter !== null) { | ||
return ($this->typeConverter)($row); | ||
foreach ($this->resultDecorators as $decorator) { | ||
$row = $decorator($row); | ||
} | ||
|
||
return $row; | ||
|
@@ -206,8 +193,8 @@ public function fetchAll(string|int $mode = PDO::FETCH_NUM): array | |
$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; | ||
|
@@ -297,4 +284,24 @@ public function queryString(): string | |
{ | ||
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(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.