Merge pull request #9774 from piwik/offset_visitor_log

Apply offset and limit correctly to the Visitor Log SQL queries

4 files changed, 153 insertions, 26 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/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 7041c89202..9f626e7969 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));
@@ -1233,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`,