when we know a segment won't match any row, replace the SQL sub-expression by (1 = 0)

Segment SQL generator: un-needed joins on log_link_visit_action  are removed, making the query a bit faster

core/Plugin/Segment.php

@@ -124,6 +124,8 @@ class Segment
      * or callable: `string $valueToMatch`, `string $segment` (see {@link setSegment()}), `string $matchType`
      * (eg SegmentExpression::MATCH_EQUAL or any other match constant of this class) and `$segmentName`.
      *
+     * If the closure returns NULL, then Piwik assumes the segment sub-string will not match any visitor.
+     *
      * @param string|\Closure $sqlFilter
      * @api
      */

core/Segment.php

@@ -179,12 +179,19 @@ class Segment
                 if (isset($segment['sqlFilter'])) {
                     $value = call_user_func($segment['sqlFilter'], $value, $segment['sqlSegment'], $matchType, $name);
 
+                    if(is_null($value)) {
+                        // eg. see TableLogAction::getIdActionFromSegment() defined
+                        return array();
+                    }
+
                     // sqlFilter-callbacks might return arrays for more complex cases
                     // e.g. see TableLogAction::getIdActionFromSegment()
                     if (is_array($value) && isset($value['SQL'])) {
                         // Special case: returned value is a sub sql expression!
                         $matchType = SegmentExpression::MATCH_ACTIONS_CONTAINS;
                     }
+
+
                 }
             }
             break;

core/Segment/SegmentExpression.php

@@ -40,6 +40,8 @@ class SegmentExpression
     const INDEX_BOOL_OPERATOR = 0;
     const INDEX_OPERAND = 1;
 
+    const SQL_WHERE_DO_NOT_MATCH_ANY_ROW = "(1 = 0)";
+
     public function __construct($string)
     {
         $this->string = $string;
@@ -138,13 +140,21 @@ class SegmentExpression
             $operator = $leaf[self::INDEX_BOOL_OPERATOR];
             $operandDefinition = $leaf[self::INDEX_OPERAND];
 
-            $operand = $this->getSqlMatchFromDefinition($operandDefinition, $availableTables);
 
-            if ($operand[1] !== null) {
-                $this->valuesBind[] = $operand[1];
+            // in case we know already the segment won't match any row...
+            if($operandDefinition === array() ) { // see getCleanedExpression()
+                $operand = self::SQL_WHERE_DO_NOT_MATCH_ANY_ROW;
+
+            } else {
+                $operand = $this->getSqlMatchFromDefinition($operandDefinition, $availableTables);
+
+                if ($operand[1] !== null) {
+                    $this->valuesBind[] = $operand[1];
+                }
+
+                $operand = $operand[0];
             }
 
-            $operand = $operand[0];
             $sqlSubExpressions[] = array(
                 self::INDEX_BOOL_OPERATOR => $operator,
                 self::INDEX_OPERAND       => $operand,
@@ -381,3 +391,4 @@ class SegmentExpression
         );
     }
 }
+

core/Tracker/TableLogAction.php

@@ -177,10 +177,9 @@ class TableLogAction
             || $matchType == SegmentExpression::MATCH_NOT_EQUAL
         ) {
             $idAction = self::getModel()->getIdActionMatchingNameAndType($valueToMatch, $actionType);
-            // if the action is not found, we hack -100 to ensure it tries to match against an integer
-            // otherwise binding idaction_name to "false" returns some rows for some reasons (in case &segment=pageTitle==Větrnásssssss)
+            // Action is not found (eg. &segment=pageTitle==Větrnásssssss)
             if (empty($idAction)) {
-                $idAction = -100;
+                $idAction = null;
             }
             return $idAction;
         }

tests/PHPUnit/Integration/SegmentTest.php

@@ -505,20 +505,15 @@ class SegmentTest extends IntegrationTestCase
 
     public function test_getSelectQuery_whenPageUrlExists_asStatementAND()
     {
+        $pageUrlFoundInDb = 'example.com/page.html?hello=world';
+
+        $actionIdFoundInDb = $this->insertPageUrlAsAction($pageUrlFoundInDb);
+
         $select = 'log_visit.*';
         $from = 'log_visit';
         $where = false;
         $bind = array();
 
-        $pageUrlFoundInDb = 'example.com/page.html?hello=world';
-
-        TableLogAction::loadIdsAction( array(
-            'idaction_url' => array($pageUrlFoundInDb, Action::TYPE_PAGE_URL)
-        ));
-
-        $actionIdFoundInDb = Db::fetchOne("SELECT idaction from " . Common::prefixTable('log_action') . " WHERE name = ?", $pageUrlFoundInDb);
-        $this->assertNotEmpty( $actionIdFoundInDb, "Action $pageUrlFoundInDb was not found in the ". Common::prefixTable('log_action') ." table.");
-
         $segment = 'visitServerHour==3;pageUrl==' . urlencode($pageUrlFoundInDb);
         $segment = new Segment($segment, $idSites = array());
 
@@ -559,21 +554,13 @@ class SegmentTest extends IntegrationTestCase
 
         $expected = array(
             "sql"  => "
-                SELECT
-                    log_inner.*
-                FROM
-                    (
                 SELECT
                     log_visit.*
                 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 HOUR(log_visit.visit_last_action_time) = ?
-                      AND log_link_visit_action.idaction_url = ?
-                GROUP BY log_visit.idvisit
-                ORDER BY NULL
-                    ) AS log_inner",
-            "bind" => array(12, -100));
+                      AND (1 = 0) ",
+            "bind" => array(12));
 
         $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query));
     }
@@ -590,6 +577,34 @@ class SegmentTest extends IntegrationTestCase
 
         $query = $segment->getSelectQuery($select, $from, $where, $bind);
 
+        $expected = array(
+            "sql"  => "
+                SELECT
+                    log_visit.*
+                FROM
+                    " . Common::prefixTable('log_visit') . " AS log_visit
+                WHERE (HOUR(log_visit.visit_last_action_time) = ?
+                      OR (1 = 0) )",
+            "bind" => array(12));
+
+        $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query));
+    }
+
+    public function test_getSelectQuery_whenPageUrlDoesNotExist_asBothStatements_OR_AND()
+    {
+        $pageUrlFoundInDb = 'example.com/found-in-db';
+        $actionIdFoundInDb = $this->insertPageUrlAsAction($pageUrlFoundInDb);
+
+        $select = 'log_visit.*';
+        $from = 'log_visit';
+        $where = false;
+        $bind = array();
+
+        $segment = 'visitServerHour==12,pageUrl==xyz;pageUrl==abcdefg,pageUrl=='.urlencode($pageUrlFoundInDb);
+        $segment = new Segment($segment, $idSites = array());
+
+        $query = $segment->getSelectQuery($select, $from, $where, $bind);
+
         $expected = array(
             "sql"  => "
                 SELECT
@@ -602,11 +617,13 @@ class SegmentTest extends IntegrationTestCase
                     " . 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 (HOUR(log_visit.visit_last_action_time) = ?
-                      OR log_link_visit_action.idaction_url = ? )
+                        OR (1 = 0))
+                      AND ((1 = 0)
+                        OR   log_link_visit_action.idaction_url = ? )
                 GROUP BY log_visit.idvisit
                 ORDER BY NULL
                     ) AS log_inner",
-            "bind" => array(12, -100));
+            "bind" => array(12, $actionIdFoundInDb));
 
         $this->assertEquals($this->removeExtraWhiteSpaces($expected), $this->removeExtraWhiteSpaces($query));
     }
@@ -617,4 +634,20 @@ class SegmentTest extends IntegrationTestCase
             'Piwik\Access' => new FakeAccess()
         );
     }
+
+    /**
+     * @param $pageUrlFoundInDb
+     * @return string
+     * @throws Exception
+     */
+    private function insertPageUrlAsAction($pageUrlFoundInDb)
+    {
+        TableLogAction::loadIdsAction(array(
+            'idaction_url' => array($pageUrlFoundInDb, Action::TYPE_PAGE_URL)
+        ));
+
+        $actionIdFoundInDb = Db::fetchOne("SELECT idaction from " . Common::prefixTable('log_action') . " WHERE name = ?", $pageUrlFoundInDb);
+        $this->assertNotEmpty($actionIdFoundInDb, "Action $pageUrlFoundInDb was not found in the " . Common::prefixTable('log_action') . " table.");
+        return $actionIdFoundInDb;
+    }
 }