Change foreign filters to subselect instead of exists

app/bundles/EmailBundle/Entity/Stat.php

@@ -149,7 +149,8 @@ public static function loadMetadata(ORM\ClassMetadata $metadata)
             ->addIndex(['tracking_hash'], 'stat_email_hash_search')
             ->addIndex(['source', 'source_id'], 'stat_email_source_search')
             ->addIndex(['date_sent'], 'email_date_sent')
-            ->addIndex(['date_read', 'lead_id'], 'email_date_read_lead');
+            ->addIndex(['date_read', 'lead_id'], 'email_date_read_lead')
+            ->addIndex(['lead_id', 'date_read'], 'email_lead_date_read');
 
         $builder->addId();
 

app/bundles/LeadBundle/Segment/Query/ContactSegmentQueryBuilder.php

@@ -158,7 +158,7 @@ public function addNewContactsRestrictions(QueryBuilder $queryBuilder, $segmentI
         $setHaving = (count($parts['groupBy']) || !is_null($parts['having']));
 
         $tableAlias = $this->generateRandomParameterName();
-        $queryBuilder->leftJoin('l', MAUTIC_TABLE_PREFIX.'lead_lists_leads', $tableAlias, $tableAlias.'.lead_id = l.id');
+        $queryBuilder->leftJoin('l', MAUTIC_TABLE_PREFIX.'lead_lists_leads', $tableAlias, $tableAlias.'.lead_id = l.id', 'FORCE INDEX (PRIMARY)');
         $queryBuilder->addSelect($tableAlias.'.lead_id AS '.$tableAlias.'_lead_id');
 
         $expression = $queryBuilder->expr()->eq($tableAlias.'.leadlist_id', $segmentId);

app/bundles/LeadBundle/Segment/Query/Filter/ForeignValueFilterQueryBuilder.php

@@ -46,63 +46,82 @@ public function applyQuery(QueryBuilder $queryBuilder, ContactSegmentFilter $fil
 
         $subQueryBuilder = $queryBuilder->getConnection()->createQueryBuilder();
         $subQueryBuilder
-            ->select('NULL')->from($filter->getTable(), $tableAlias)
-            ->andWhere($tableAlias.'.lead_id = l.id');
+            ->select($tableAlias.'.lead_id')->from($filter->getTable(), $tableAlias);
 
         if (!is_null($filter->getWhere())) {
             $subQueryBuilder->andWhere(str_replace(str_replace(MAUTIC_TABLE_PREFIX, '', $filter->getTable()).'.', $tableAlias.'.', $filter->getWhere()));
         }
 
         switch ($filterOperator) {
             case 'empty':
-                $subQueryBuilder->andWhere($subQueryBuilder->expr()->isNull($tableAlias.'.'.$filter->getField()));
-                $queryBuilder->addLogic($queryBuilder->expr()->exists($subQueryBuilder->getSQL()), $filter->getGlue());
-                break;
             case 'notEmpty':
-                $subQueryBuilder->andWhere($subQueryBuilder->expr()->isNotNull($tableAlias.'.'.$filter->getField()));
-                $queryBuilder->addLogic($queryBuilder->expr()->exists($subQueryBuilder->getSQL()), $filter->getGlue());
+                //Using a join table is necessary for empty/not empty to work properly
+                //An Exists or IN subquery would not provide the proper result if there is no record for the lead in the foreign table
+                //If we join the foreign table by lead_id and check for the filtered field for Null/notnull, the result will contain both those leads that have no record in the foreign table and those that have but with the value of NULL
+                //Most often if a lead has not or has not done something that is usually stored in a foreign table, there is no record for that data
+                //As empty/not empty you want to check whether that record is present or not
+
+                $tableAlias = $this->generateRandomParameterName();
+
+                $queryBuilder->leftJoin(
+                    $queryBuilder->getTableAlias(MAUTIC_TABLE_PREFIX.'leads'),
+                    $filter->getTable(),
+                    $tableAlias,
+                    $tableAlias.'.lead_id = l.id'
+                );
+
+                if ($filterOperator == 'empty') {
+                    $queryBuilder->addLogic($queryBuilder->expr()->isNull($tableAlias.'.'.$filter->getField()), $filter->getGlue());
+                } else {
+                    $queryBuilder->addLogic($queryBuilder->expr()->isNotNull($tableAlias.'.'.$filter->getField()), $filter->getGlue());
+                }
                 break;
             case 'notIn':
-                // The use of NOT EXISTS here requires the use of IN instead of NOT IN to prevent a "double negative."
-                // We are not using EXISTS...NOT IN because it results in including everyone who has at least one entry that doesn't
-                // match the criteria. For example, with tags, if the contact has the tag in the filter but also another tag, they'll
-                // be included in the results which is not what we want.
-                $expression = $subQueryBuilder->expr()->in(
-                    $tableAlias.'.'.$filter->getField(),
-                    $filterParametersHolder
+                $expression = $subQueryBuilder->expr()->andX(
+                    $subQueryBuilder->expr()->in($tableAlias.'.'.$filter->getField(), $filterParametersHolder),
+                    $subQueryBuilder->expr()->isNotNull($tableAlias.'.lead_id')
                 );
 
                 $subQueryBuilder->andWhere($expression);
-                $queryBuilder->addLogic($queryBuilder->expr()->notExists($subQueryBuilder->getSQL()), $filter->getGlue());
+                $queryBuilder->addLogic($queryBuilder->expr()->notIn('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
                 break;
             case 'neq':
-                $expression = $subQueryBuilder->expr()->orX(
+                $expression = $subQueryBuilder->expr()->andX(
                     $subQueryBuilder->expr()->eq($tableAlias.'.'.$filter->getField(), $filterParametersHolder),
-                    $subQueryBuilder->expr()->isNull($tableAlias.'.'.$filter->getField())
+                    $subQueryBuilder->expr()->isNotNull($tableAlias.'.lead_id')
                 );
 
                 $subQueryBuilder->andWhere($expression);
 
-                $queryBuilder->addLogic($queryBuilder->expr()->notExists($subQueryBuilder->getSQL()), $filter->getGlue());
+                $queryBuilder->addLogic($queryBuilder->expr()->notIn('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
+
                 break;
             case 'notLike':
-                $expression = $subQueryBuilder->expr()->orX(
-                    $subQueryBuilder->expr()->isNull($tableAlias.'.'.$filter->getField()),
-                    $subQueryBuilder->expr()->like($tableAlias.'.'.$filter->getField(), $filterParametersHolder)
+                $expression = $subQueryBuilder->expr()->andX(
+                    $subQueryBuilder->expr()->like($tableAlias.'.'.$filter->getField(), $filterParametersHolder),
+                    $subQueryBuilder->expr()->isNotNull($tableAlias.'.lead_id')
                 );
 
                 $subQueryBuilder->andWhere($expression);
 
-                $queryBuilder->addLogic($queryBuilder->expr()->notExists($subQueryBuilder->getSQL()), $filter->getGlue());
+                $queryBuilder->addLogic($queryBuilder->expr()->notIn('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
                 break;
             case 'regexp':
+                $expression = $tableAlias.'.'.$filter->getField().' REGEXP '.$filterParametersHolder;
+
+                $subQueryBuilder->andWhere($expression);
+
+                $queryBuilder->addLogic($queryBuilder->expr()->in('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
+                break;
             case 'notRegexp':
-                $not        = ($filterOperator === 'notRegexp') ? ' NOT' : '';
-                $expression = $tableAlias.'.'.$filter->getField().$not.' REGEXP '.$filterParametersHolder;
+                $expression = $subQueryBuilder->expr()->andX(
+                    $tableAlias.'.'.$filter->getField().'  REGEXP '.$filterParametersHolder,
+                    $subQueryBuilder->expr()->isNotNull($tableAlias.'.lead_id')
+                );
 
                 $subQueryBuilder->andWhere($expression);
 
-                $queryBuilder->addLogic($queryBuilder->expr()->exists($subQueryBuilder->getSQL()), $filter->getGlue());
+                $queryBuilder->addLogic($queryBuilder->expr()->notIn('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
                 break;
             default:
                 $expression = $subQueryBuilder->expr()->$filterOperator(
@@ -111,7 +130,7 @@ public function applyQuery(QueryBuilder $queryBuilder, ContactSegmentFilter $fil
                 );
                 $subQueryBuilder->andWhere($expression);
 
-                $queryBuilder->addLogic($queryBuilder->expr()->exists($subQueryBuilder->getSQL()), $filter->getGlue());
+                $queryBuilder->addLogic($queryBuilder->expr()->in('l.id', $subQueryBuilder->getSQL()), $filter->getGlue());
         }
 
         $queryBuilder->setParametersPairs($parameters, $filterParameters);

app/bundles/LeadBundle/Segment/Query/QueryBuilder.php

@@ -642,14 +642,16 @@ public function insert($insert = null)
      *
      * @param string      $from  the table
      * @param string|null $alias the alias of the table
+     * @param string|null index hint for the table
      *
      * @return $this this QueryBuilder instance
      */
-    public function from($from, $alias = null)
+    public function from($from, $alias = null, $indexHint = '')
     {
         return $this->add('from', [
-            'table' => $from,
-            'alias' => $alias,
+            'table'     => $from,
+            'alias'     => $alias,
+            'indexHint' => $indexHint,
         ], true);
     }
 
@@ -670,9 +672,9 @@ public function from($from, $alias = null)
      *
      * @return $this this QueryBuilder instance
      */
-    public function join($fromAlias, $join, $alias, $condition = null)
+    public function join($fromAlias, $join, $alias, $condition = null, $indexHint = '')
     {
-        return $this->innerJoin($fromAlias, $join, $alias, $condition);
+        return $this->innerJoin($fromAlias, $join, $alias, $condition, $indexHint);
     }
 
     /**
@@ -692,14 +694,15 @@ public function join($fromAlias, $join, $alias, $condition = null)
      *
      * @return $this this QueryBuilder instance
      */
-    public function innerJoin($fromAlias, $join, $alias, $condition = null)
+    public function innerJoin($fromAlias, $join, $alias, $condition = null, $indexHint = '')
     {
         return $this->add('join', [
             $fromAlias => [
                 'joinType'      => 'inner',
                 'joinTable'     => $join,
                 'joinAlias'     => $alias,
                 'joinCondition' => $condition,
+                'joinIndexHint' => $indexHint,
             ],
         ], true);
     }
@@ -721,14 +724,15 @@ public function innerJoin($fromAlias, $join, $alias, $condition = null)
      *
      * @return $this this QueryBuilder instance
      */
-    public function leftJoin($fromAlias, $join, $alias, $condition = null)
+    public function leftJoin($fromAlias, $join, $alias, $condition = null, $indexHint = '')
     {
         return $this->add('join', [
             $fromAlias => [
                 'joinType'      => 'left',
                 'joinTable'     => $join,
                 'joinAlias'     => $alias,
                 'joinCondition' => $condition,
+                'joinIndexHint' => $indexHint,
             ],
         ], true);
     }
@@ -750,14 +754,15 @@ public function leftJoin($fromAlias, $join, $alias, $condition = null)
      *
      * @return $this this QueryBuilder instance
      */
-    public function rightJoin($fromAlias, $join, $alias, $condition = null)
+    public function rightJoin($fromAlias, $join, $alias, $condition = null, $indexHint = '')
     {
         return $this->add('join', [
             $fromAlias => [
                 'joinType'      => 'right',
                 'joinTable'     => $join,
                 'joinAlias'     => $alias,
                 'joinCondition' => $condition,
+                'joinIndexHint' => $indexHint,
             ],
         ], true);
     }
@@ -1187,10 +1192,18 @@ private function getFromClauses()
         // Loop through all FROM clauses
         foreach ($this->sqlParts['from'] as $from) {
             if ($from['alias'] === null) {
-                $tableSql       = $from['table'];
+                if (!empty($from['indexHint'])) {
+                    $tableSql       = $from['table'].' '.$from['indexHint'];
+                } else {
+                    $tableSql       = $from['table'];
+                }
                 $tableReference = $from['table'];
             } else {
-                $tableSql       = $from['table'].' '.$from['alias'];
+                if (!empty($from['indexHint'])) {
+                    $tableSql       = $from['table'].' '.$from['alias'].' '.$from['indexHint'];
+                } else {
+                    $tableSql       = $from['table'].' '.$from['alias'];
+                }
                 $tableReference = $from['alias'];
             }
 
@@ -1365,7 +1378,7 @@ private function getSQLForJoins($fromAlias, array &$knownAliases)
                     throw QueryException::nonUniqueAlias($join['joinAlias'], array_keys($knownAliases));
                 }
                 $sql .= ' '.strtoupper($join['joinType'])
-                    .' JOIN '.$join['joinTable'].' '.$join['joinAlias']
+                    .' JOIN '.$join['joinTable'].' '.$join['joinAlias'].(!empty($join['joinIndexHint']) ? ' '.$join['joinIndexHint'] : '')
                     .' ON '.((string) $join['joinCondition']);
                 $knownAliases[$join['joinAlias']] = true;
             }

app/migrations/Version20190919095312.php

@@ -0,0 +1,52 @@
+<?php
+
+/*
+ * @copyright   2016 Mautic Contributors. All rights reserved
+ * @author      Mautic
+ *
+ * @link        http://mautic.org
+ *
+ * @license     GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html
+ */
+
+namespace Mautic\Migrations;
+
+use Doctrine\DBAL\Migrations\SkipMigrationException;
+use Doctrine\DBAL\Schema\Schema;
+use Mautic\CoreBundle\Doctrine\AbstractMauticMigration;
+
+/**
+ * Auto-generated Migration: Please modify to your needs!
+ */
+class Version20190919095312 extends AbstractMauticMigration
+{
+    /**
+     * @param Schema $schema
+     *
+     * @throws SkipMigrationException
+     * @throws \Doctrine\DBAL\Schema\SchemaException
+     */
+    public function preUp(Schema $schema)
+    {
+        $table = $schema->getTable("{$this->prefix}email_stats");
+        if ($table->hasIndex("{$this->prefix}email_lead_date_read")) {
+            throw new SkipMigrationException('Schema includes this migration');
+        }
+    }
+
+    /**
+     * @param Schema $schema
+     */
+    public function up(Schema $schema)
+    {
+        $this->addSql("alter table {$this->prefix}email_stats add index {$this->prefix}email_lead_date_read (lead_id, date_read)");
+    }
+
+    /**
+     * @param Schema $schema
+     */
+    public function down(Schema $schema)
+    {
+        $this->addSql("alter table {$this->prefix}email_stats drop index {$this->prefix}email_lead_date_read (lead_id, date_read)");
+    }
+}