Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Zend\Paginator\Adapter\DbSelect alternative solution to count, with subselect #4641

Merged
merged 4 commits into from

6 participants

@ralphschindler
Collaborator

This is a generic solution that should work across all database platforms (I've tested against the Zend_Db-Examples repository schema, and it works on Sqlite, MySQL, and Postgres.)

Ideally, in the future, we will have a module per vendor of database so that we can have platform specific solutions that take advantage of platform specific features. For example, MySQL could use SQL_CALC_FOUND_ROWS.

@ralphschindler ralphschindler Zend\Paginator\Adapter\DbSelect
First attempt at using subselect to calculate full count
c49d659
@fatmuemoo

This doesn't work on non join queries.... example:
SELECT COUNT(1) AS c FROM (SELECT FROM transaction_line WHERE transaction_id = '1') AS original_select

Commenting out the line that resets the the columns fixes this.

@ralphschindler
Collaborator

Hmm, resetting should reset columns not to empty but to a wildcard I think. I'll look into this.

@ralphschindler ralphschindler Zend\Db & Zend\Paginator
- Paginator: removed clearing of columns from select
- Db\Sql\Select: when resettings columns, reset value should be array(self::SQL_STAR)
570778c
@ralphschindler
Collaborator

In addition, I am not sure columns() needs to be reset at all, what do you think about this patch now?

@fatmuemoo

IMO, the columns don't need to be reset... looks good.

@ralphschindler
Collaborator

Yeah, I've asked in #zftalk.dev, and they agree. If we need to do any kind of performance enhancements (which is basically what resetting columns is), we can do them elsewhere at a different time (perhaps in those vendor specific modules I mentioned).

Thanks, I'll work up unit tests and get this going.

ralphschindler added some commits
@ralphschindler ralphschindler Zend\Paginator - added tests for DbSelect
Zend\Db\Sql\Select - reverted reset on column
fb50025
@ralphschindler ralphschindler Zend\Paginator
- reorganized the Mock setup for getItems and count tests to work
e04fb2a
@EvanDotPro EvanDotPro merged commit e04fb2a into from
@latch

In issue did not reset columns in select query to determine the count of rows in the grouped queries. Here there was a problem in my opinion. The count() method throws an error when original_select will be such:

SELECT *, TRIM(str_column) as str_column FROM table

PDOException: SQLSTATE[42S21]: Column already exists: 1060 Duplicate column name 'str_column'

Query is good work: SELECT *, TRIM(str_column) as str_column FROM table

But query determine the count of rows throws an error:
SELECT COUNT(1) AS "c" FROM ( SELECT *, TRIM(str_column) as str_column FROM table ) AS "original_select"

The error resulted in higher.

I think need to reset columns original_select on '*'.

$select->reset(Select::COLUMNS);
$select->columns(array(Select::SQL_STAR));

So everything will work correctly.

And why delete code which get join information, clear, and repopulate without columns?

@latch

If in join uses the same technique ( e.g. TRIM(str_column) as str_column ) appears the same error. For this you need to reset the column in join. Since it was originally.

@bdewong

To me it would seem that this change would dramatically increase the processing time in some cases. In the simplest case, I used a basic full table SELECT. In this scenario, the subquery will grab all the information from the table, when perform a count on that instead of providing just a simple count of the data we need. Is that not correct?
Thanks,
Brent

Collaborator

Perhaps, but admittedly performance was not the top priority. Correctness and cross-db platform interoperability were higher priorities. That said, there is a great PR (#5518) that will allow for custom Select objects during pagination.

As I believe the correctness problem is mainly down to problems with queries having group by, maybe we could simply check if there are any group -bys and then select the paginator strategy accordingly: without sub-select if there are no group-bys and using the sub select if there are)..?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 12, 2013
  1. @ralphschindler

    Zend\Paginator\Adapter\DbSelect

    ralphschindler authored
    First attempt at using subselect to calculate full count
  2. @ralphschindler

    Zend\Db & Zend\Paginator

    ralphschindler authored
    - Paginator: removed clearing of columns from select
    - Db\Sql\Select: when resettings columns, reset value should be array(self::SQL_STAR)
  3. @ralphschindler

    Zend\Paginator - added tests for DbSelect

    ralphschindler authored
    Zend\Db\Sql\Select - reverted reset on column
  4. @ralphschindler

    Zend\Paginator

    ralphschindler authored
    - reorganized the Mock setup for getItems and count tests to work
This page is out of date. Refresh to see the latest.
View
15 library/Zend/Paginator/Adapter/DbSelect.php
@@ -103,22 +103,15 @@ public function count()
}
$select = clone $this->select;
- $select->reset(Select::COLUMNS);
$select->reset(Select::LIMIT);
$select->reset(Select::OFFSET);
$select->reset(Select::ORDER);
- $select->reset(Select::GROUP);
- // get join information, clear, and repopulate without columns
- $joins = $select->getRawState(Select::JOINS);
- $select->reset(Select::JOINS);
- foreach ($joins as $join) {
- $select->join($join['name'], $join['on'], array(), $join['type']);
- }
-
- $select->columns(array('c' => new Expression('COUNT(1)')));
+ $countSelect = new Select;
+ $countSelect->columns(array('c' => new Expression('COUNT(1)')));
+ $countSelect->from(array('original_select' => $select));
- $statement = $this->sql->prepareStatementForSqlObject($select);
+ $statement = $this->sql->prepareStatementForSqlObject($countSelect);
$result = $statement->execute();
$row = $result->current();
View
47 tests/ZendTest/Paginator/Adapter/DbSelectTest.php
@@ -25,19 +25,31 @@ class DbSelectTest extends \PHPUnit_Framework_TestCase
/** @var \PHPUnit_Framework_MockObject_MockObject */
protected $mockSelect;
+ /** @var \PHPUnit_Framework_MockObject_MockObject */
+ protected $mockStatement;
+
+ /** @var \PHPUnit_Framework_MockObject_MockObject */
protected $mockResult;
+ /** @var \PHPUnit_Framework_MockObject_MockObject */
+ protected $mockSql;
+
/** @var DbSelect */
protected $dbSelect;
public function setup()
{
- $mockStatement = $this->getMock('Zend\Db\Adapter\Driver\StatementInterface');
$mockResult = $this->getMock('Zend\Db\Adapter\Driver\ResultInterface');
+ $this->mockResult = $mockResult;
+
+ $mockStatement = $this->getMock('Zend\Db\Adapter\Driver\StatementInterface');
+ $this->mockStatement = $mockStatement;
+
+ $this->mockStatement->expects($this->any())->method('execute')->will($this->returnValue($this->mockResult));
$mockDriver = $this->getMock('Zend\Db\Adapter\Driver\DriverInterface');
$mockDriver->expects($this->any())->method('createStatement')->will($this->returnValue($mockStatement));
- $mockStatement->expects($this->any())->method('execute')->will($this->returnValue($mockResult));
+
$mockPlatform = $this->getMock('Zend\Db\Adapter\Platform\PlatformInterface');
$mockPlatform->expects($this->any())->method('getName')->will($this->returnValue('platform'));
$mockAdapter = $this->getMockForAbstractClass(
@@ -45,9 +57,21 @@ public function setup()
array($mockDriver, $mockPlatform)
);
+ $mockSql = $this->getMock(
+ 'Zend\Db\Sql\Sql',
+ array('prepareStatementForSqlObject', 'execute'),
+ array($mockAdapter)
+ );
+ $this->mockSql = $mockSql;
+ $this->mockSql->expects($this->once())
+ ->method('prepareStatementForSqlObject')
+ ->with($this->isInstanceOf('Zend\Db\Sql\Select'))
+ ->will($this->returnValue($this->mockStatement));
+
+
$this->mockSelect = $this->getMock('Zend\Db\Sql\Select');
- $this->mockResult = $mockResult;
- $this->dbSelect = new DbSelect($this->mockSelect, $mockAdapter);
+
+ $this->dbSelect = new DbSelect($this->mockSelect, $mockSql);
}
public function testGetItems()
@@ -60,18 +84,9 @@ public function testGetItems()
public function testCount()
{
- $this->mockSelect->expects($this->once())->method('columns')->with($this->equalTo(array('c' => new Expression('COUNT(1)'))));
- $this->mockResult->expects($this->any())->method('current')->will($this->returnValue(array('c' => 5)));
-
- $this->mockSelect->expects($this->exactly(6))->method('reset'); // called for columns, limit, offset, order
- $this->mockSelect->expects($this->once())->method('getRawState')->with($this->equalTo(Select::JOINS))
- ->will($this->returnValue(array(array('name' => 'Foo', 'on' => 'On Stuff', 'columns' => array('foo', 'bar'), 'type' => Select::JOIN_INNER))));
- $this->mockSelect->expects($this->once())->method('join')->with(
- 'Foo',
- 'On Stuff',
- array(),
- Select::JOIN_INNER
- );
+ $this->mockResult->expects($this->once())->method('current')->will($this->returnValue(array('c' => 5)));
+
+ $this->mockSelect->expects($this->exactly(3))->method('reset'); // called for columns, limit, offset, order
$count = $this->dbSelect->count();
$this->assertEquals(5, $count);
Something went wrong with that request. Please try again.