From 5164862249eaf7af69f0ca6470d4f5e42784725b Mon Sep 17 00:00:00 2001 From: mattab Date: Sat, 13 Feb 2016 11:02:21 +0100 Subject: Apply offset and limit correctly to the Visitor Log SQL queries --- core/DataAccess/LogQueryBuilder.php | 35 +++++---- core/Segment.php | 7 +- tests/PHPUnit/Integration/SegmentTest.php | 126 +++++++++++++++++++++++++++++- 3 files changed, 148 insertions(+), 20 deletions(-) diff --git a/core/DataAccess/LogQueryBuilder.php b/core/DataAccess/LogQueryBuilder.php index b0f2b73ad0..c65f36bf10 100644 --- a/core/DataAccess/LogQueryBuilder.php +++ b/core/DataAccess/LogQueryBuilder.php @@ -16,7 +16,7 @@ use Piwik\Segment\SegmentExpression; class LogQueryBuilder { public function getSelectQueryString(SegmentExpression $segmentExpression, $select, $from, $where, $bind, $groupBy, - $orderBy, $limit) + $orderBy, $limitAndOffset) { if (!is_array($from)) { $from = array($from); @@ -43,11 +43,11 @@ class LogQueryBuilder if ($useSpecialConversionGroupBy) { $innerGroupBy = "CONCAT(log_conversion.idvisit, '_' , log_conversion.idgoal, '_', log_conversion.buster)"; - $sql = $this->buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit, $innerGroupBy); + $sql = $this->buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limitAndOffset, $innerGroupBy); } elseif ($joinWithSubSelect) { - $sql = $this->buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit); + $sql = $this->buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limitAndOffset); } else { - $sql = $this->buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit); + $sql = $this->buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $limitAndOffset); } return array( 'sql' => $sql, @@ -249,12 +249,12 @@ class LogQueryBuilder * @param string $where * @param string $groupBy * @param string $orderBy - * @param string $limit + * @param string $limitAndOffset * @param null|string $innerGroupBy If given, this inner group by will be used. If not, we try to detect one * @throws Exception * @return string */ - private function buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit, $innerGroupBy = null) + private function buildWrappedSelectQuery($select, $from, $where, $groupBy, $orderBy, $limitAndOffset, $innerGroupBy = null) { $matchTables = "(log_visit|log_conversion_item|log_conversion|log_action)"; preg_match_all("/". $matchTables ."\.[a-z0-9_\*]+/", $select, $matches); @@ -271,7 +271,7 @@ class LogQueryBuilder $innerFrom = $from; $innerWhere = $where; - $innerLimit = $limit; + $innerLimitAndOffset = $limitAndOffset; if (!isset($innerGroupBy) && in_array('log_visit', $matchesFrom[1])) { $innerGroupBy = "log_visit.idvisit"; @@ -280,16 +280,16 @@ class LogQueryBuilder } $innerOrderBy = "NULL"; - if ($innerLimit && $orderBy) { + if ($innerLimitAndOffset && $orderBy) { // only When LIMITing we can apply to the inner query the same ORDER BY as the parent query $innerOrderBy = $orderBy; } - if ($innerLimit) { + if ($innerLimitAndOffset) { // When LIMITing, no need to GROUP BY (GROUPing by is done before the LIMIT which is super slow when large amount of rows is matched) $innerGroupBy = false; } - $innerQuery = $this->buildSelectQuery($innerSelect, $innerFrom, $innerWhere, $innerGroupBy, $innerOrderBy, $innerLimit); + $innerQuery = $this->buildSelectQuery($innerSelect, $innerFrom, $innerWhere, $innerGroupBy, $innerOrderBy, $innerLimitAndOffset); $select = preg_replace('/'.$matchTables.'\./', 'log_inner.', $select); $from = " @@ -299,7 +299,9 @@ class LogQueryBuilder $where = false; $orderBy = preg_replace('/'.$matchTables.'\./', 'log_inner.', $orderBy); $groupBy = preg_replace('/'.$matchTables.'\./', 'log_inner.', $groupBy); - $query = $this->buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit); + + $outerLimitAndOffset = null; + $query = $this->buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $outerLimitAndOffset); return $query; } @@ -312,10 +314,10 @@ class LogQueryBuilder * @param string $where where clause * @param string $groupBy group by clause * @param string $orderBy order by clause - * @param string|int $limit limit by clause eg '5' for Limit 5 Offset 0 or '10, 5' for Limit 5 Offset 10 + * @param string|int $limitAndOffset limit by clause eg '5' for Limit 5 Offset 0 or '10, 5' for Limit 5 Offset 10 * @return string */ - private function buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $limit) + private function buildSelectQuery($select, $from, $where, $groupBy, $orderBy, $limitAndOffset) { $sql = " SELECT @@ -341,11 +343,16 @@ class LogQueryBuilder $orderBy"; } - $sql = $this->appendLimitClauseToQuery($sql, $limit); + $sql = $this->appendLimitClauseToQuery($sql, $limitAndOffset); return $sql; } + /** + * @param $sql + * @param $limit LIMIT clause eg. "10, 50" (offset 10, limit 50) + * @return string + */ private function appendLimitClauseToQuery($sql, $limit) { $limitParts = explode(',', (string) $limit); diff --git a/core/Segment.php b/core/Segment.php index d9f8d163c6..0ba687740e 100644 --- a/core/Segment.php +++ b/core/Segment.php @@ -299,12 +299,13 @@ class Segment { $segmentExpression = $this->segmentExpression; - if ($offset > 0) { - $limit = (int) $offset . ', ' . (int) $limit; + $limitAndOffset = null; + if($limit > 0) { + $limitAndOffset = (int) $offset . ', ' . (int) $limit; } return $this->segmentQueryBuilder->getSelectQueryString($segmentExpression, $select, $from, $where, $bind, - $groupBy, $orderBy, $limit); + $groupBy, $orderBy, $limitAndOffset); } /** diff --git a/tests/PHPUnit/Integration/SegmentTest.php b/tests/PHPUnit/Integration/SegmentTest.php index 7041c89202..2cd93d9d87 100644 --- a/tests/PHPUnit/Integration/SegmentTest.php +++ b/tests/PHPUnit/Integration/SegmentTest.php @@ -829,9 +829,129 @@ class SegmentTest extends IntegrationTestCase AND ( log_link_visit_action.custom_var_k1 = ? ) ORDER BY NULL - LIMIT 33 - ) AS log_inner - LIMIT 33", + LIMIT 0, 33 + ) AS log_inner", + "bind" => array(1, 'Test')); + + $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query)); + } + + + public function test_getSelectQuery_whenLimitAndOffset_outerQueryShouldNotHaveOffset() + { + $select = 'sum(log_visit.visit_total_time) as sum_visit_length'; + $from = 'log_visit'; + $where = 'log_visit.idvisit = ?'; + $bind = array(1); + + $segment = 'customVariablePageName1==Test'; + $segment = new Segment($segment, $idSites = array()); + + $orderBy = false; + $groupBy = false; + $limit = 33; + $offset = 10; + + $query = $segment->getSelectQuery($select, $from, $where, $bind, $orderBy, $groupBy, $limit, $offset); + + $expected = array( + "sql" => " + SELECT + sum(log_inner.visit_total_time) as sum_visit_length + FROM + ( + SELECT + log_visit.visit_total_time + FROM + " . Common::prefixTable('log_visit') . " AS log_visit + LEFT JOIN " . Common::prefixTable('log_link_visit_action') . " AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit + WHERE + ( log_visit.idvisit = ? ) + AND + ( log_link_visit_action.custom_var_k1 = ? ) + ORDER BY NULL + LIMIT 10, 33 + ) AS log_inner", + "bind" => array(1, 'Test')); + + $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query)); + } + + public function test_getSelectQuery_whenOffsetIsZero() + { + $select = 'sum(log_visit.visit_total_time) as sum_visit_length'; + $from = 'log_visit'; + $where = 'log_visit.idvisit = ?'; + $bind = array(1); + + $segment = 'customVariablePageName1==Test'; + $segment = new Segment($segment, $idSites = array()); + + $orderBy = false; + $groupBy = false; + $limit = 33; + $offset = 0; + + $query = $segment->getSelectQuery($select, $from, $where, $bind, $orderBy, $groupBy, $limit, $offset); + + $expected = array( + "sql" => " + SELECT + sum(log_inner.visit_total_time) as sum_visit_length + FROM + ( + SELECT + log_visit.visit_total_time + FROM + " . Common::prefixTable('log_visit') . " AS log_visit + LEFT JOIN " . Common::prefixTable('log_link_visit_action') . " AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit + WHERE + ( log_visit.idvisit = ? ) + AND + ( log_link_visit_action.custom_var_k1 = ? ) + ORDER BY NULL + LIMIT 0, 33 + ) AS log_inner", + "bind" => array(1, 'Test')); + + $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query)); + } + + public function test_getSelectQuery_whenLimitIsZero() + { + $select = 'sum(log_visit.visit_total_time) as sum_visit_length'; + $from = 'log_visit'; + $where = 'log_visit.idvisit = ?'; + $bind = array(1); + + $segment = 'customVariablePageName1==Test'; + $segment = new Segment($segment, $idSites = array()); + + $orderBy = false; + $groupBy = false; + $limit = 0; + $offset = 10; + + $query = $segment->getSelectQuery($select, $from, $where, $bind, $orderBy, $groupBy, $limit, $offset); + + $expected = array( + "sql" => " + SELECT + sum(log_inner.visit_total_time) as sum_visit_length + FROM + ( + SELECT + log_visit.visit_total_time + FROM + " . Common::prefixTable('log_visit') . " AS log_visit + LEFT JOIN " . Common::prefixTable('log_link_visit_action') . " AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit + WHERE + ( log_visit.idvisit = ? ) + AND + ( log_link_visit_action.custom_var_k1 = ? ) + GROUP BY log_visit.idvisit + ORDER BY NULL + ) AS log_inner", "bind" => array(1, 'Test')); $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query)); -- cgit v1.2.3 From 055839d047c1d0b3ba36a5447e40ebee507a0b74 Mon Sep 17 00:00:00 2001 From: mattab Date: Sat, 13 Feb 2016 11:58:44 +0100 Subject: Apply offset and limit correctly to the Visitor Log SQL queries --- plugins/Live/tests/Integration/ModelTest.php | 9 ++++----- tests/PHPUnit/Integration/SegmentTest.php | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/plugins/Live/tests/Integration/ModelTest.php b/plugins/Live/tests/Integration/ModelTest.php index 355f1fb777..828d3865f0 100644 --- a/plugins/Live/tests/Integration/ModelTest.php +++ b/plugins/Live/tests/Integration/ModelTest.php @@ -53,7 +53,7 @@ class ModelTest extends IntegrationTestCase AND log_visit.visit_last_action_time >= ? AND log_visit.visit_last_action_time <= ? ORDER BY idsite, visit_last_action_time DESC - LIMIT 100 + LIMIT 0, 100 ) AS sub GROUP BY sub.idvisit ORDER BY sub.visit_last_action_time DESC @@ -93,7 +93,7 @@ class ModelTest extends IntegrationTestCase AND log_visit.visit_last_action_time >= ? AND log_visit.visit_last_action_time <= ? ORDER BY visit_last_action_time DESC - LIMIT 100 + LIMIT 0, 100 ) AS sub GROUP BY sub.idvisit ORDER BY sub.visit_last_action_time DESC @@ -154,7 +154,7 @@ class ModelTest extends IntegrationTestCase $period = 'month', $date = '2010-01-01', $segment = 'customVariablePageName1==Test', - $offset = 0, + $offset = 10, $limit = 100, $visitorId = 'abc', $minTimestamp = false, @@ -175,10 +175,9 @@ class ModelTest extends IntegrationTestCase AND log_visit.visit_last_action_time <= ? ) AND ( log_link_visit_action.custom_var_k1 = ? ) ORDER BY idsite, visit_last_action_time DESC - LIMIT 100 + LIMIT 10, 100 ) AS log_inner ORDER BY idsite, visit_last_action_time DESC - LIMIT 100 ) AS sub GROUP BY sub.idvisit ORDER BY sub.visit_last_action_time DESC diff --git a/tests/PHPUnit/Integration/SegmentTest.php b/tests/PHPUnit/Integration/SegmentTest.php index 2cd93d9d87..9f626e7969 100644 --- a/tests/PHPUnit/Integration/SegmentTest.php +++ b/tests/PHPUnit/Integration/SegmentTest.php @@ -1353,7 +1353,7 @@ class SegmentTest extends IntegrationTestCase $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query)); } - // se https://github.com/piwik/piwik/issues/9194 + // see https://github.com/piwik/piwik/issues/9194 public function test_getSelectQuery_whenQueryingLogConversionWithSegmentThatUsesLogLinkVisitActionAndLogVisit_shouldUseSubselectGroupedByIdVisitAndBuster() { $select = 'log_conversion.idgoal AS `idgoal`, -- cgit v1.2.3