Optimized Query AST resultant of the parsing process phase 1

lib/Doctrine/ORM/Query/Parser.php

@@ -267,11 +267,11 @@ class Parser
     {
         $AST = $this->getAST();
 
-        if ($customWalkers = $this->_query->getHint(Query::HINT_CUSTOM_TREE_WALKERS)) {
+        if (($customWalkers = $this->_query->getHint(Query::HINT_CUSTOM_TREE_WALKERS)) !== false) {
             $this->_customTreeWalkers = $customWalkers;
         }
 
-        if ($customOutputWalker = $this->_query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER)) {
+        if (($customOutputWalker = $this->_query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER)) !== false) {
             $this->_customOutputWalker = $customOutputWalker;
         }
 
@@ -1786,6 +1786,12 @@ class Parser
             $conditionalTerms[] = $this->ConditionalTerm();
         }
 
+        // Phase 1 AST optimization: Prevent AST\ConditionalExpression
+        // if only one AST\ConditionalTerm is defined
+        if (count($conditionalTerms) == 1) {
+            return $conditionalTerms[0];
+        }
+
         return new AST\ConditionalExpression($conditionalTerms);
     }
 
@@ -1804,6 +1810,12 @@ class Parser
             $conditionalFactors[] = $this->ConditionalFactor();
         }
 
+        // Phase 1 AST optimization: Prevent AST\ConditionalTerm
+        // if only one AST\ConditionalFactor is defined
+        if (count($conditionalFactors) == 1) {
+            return $conditionalFactors[0];
+        }
+
         return new AST\ConditionalTerm($conditionalFactors);
     }
 
@@ -1820,11 +1832,19 @@ class Parser
             $this->match(Lexer::T_NOT);
             $not = true;
         }
+        
+        $conditionalPrimary = $this->ConditionalPrimary();
 
-        $condFactor = new AST\ConditionalFactor($this->ConditionalPrimary());
-        $condFactor->not = $not;
+        // Phase 1 AST optimization: Prevent AST\ConditionalFactor
+        // if only one AST\ConditionalPrimary is defined
+        if ( ! $not) {
+            return $conditionalPrimary;
+        }
+
+        $conditionalFactor = new AST\ConditionalFactor($conditionalPrimary);
+        $conditionalFactor->not = $not;
 
-        return $condFactor;
+        return $conditionalFactor;
     }
 
     /**
@@ -2104,6 +2124,12 @@ class Parser
             $terms[] = $this->ArithmeticTerm();
         }
 
+        // Phase 1 AST optimization: Prevent AST\SimpleArithmeticExpression
+        // if only one AST\ArithmeticTerm is defined
+        if (count($terms) == 1) {
+            return $terms[0];
+        }
+
         return new AST\SimpleArithmeticExpression($terms);
     }
 
@@ -2124,6 +2150,12 @@ class Parser
             $factors[] = $this->ArithmeticFactor();
         }
 
+        // Phase 1 AST optimization: Prevent AST\ArithmeticTerm
+        // if only one AST\ArithmeticFactor is defined
+        if (count($factors) == 1) {
+            return $factors[0];
+        }
+
         return new AST\ArithmeticTerm($factors);
     }
 
@@ -2134,14 +2166,22 @@ class Parser
      */
     public function ArithmeticFactor()
     {
-       $sign = null;
+        $sign = null;
 
-       if (($isPlus = $this->_lexer->isNextToken(Lexer::T_PLUS)) || $this->_lexer->isNextToken(Lexer::T_MINUS)) {
-           $this->match(($isPlus) ? Lexer::T_PLUS : Lexer::T_MINUS);
-           $sign = $isPlus;
-       }
+        if (($isPlus = $this->_lexer->isNextToken(Lexer::T_PLUS)) || $this->_lexer->isNextToken(Lexer::T_MINUS)) {
+            $this->match(($isPlus) ? Lexer::T_PLUS : Lexer::T_MINUS);
+            $sign = $isPlus;
+        }
+        
+        $primary = $this->ArithmeticPrimary();
+
+        // Phase 1 AST optimization: Prevent AST\ArithmeticFactor
+        // if only one AST\ArithmeticPrimary is defined
+        if ($sign === null) {
+            return $primary;
+        }
 
-        return new AST\ArithmeticFactor($this->ArithmeticPrimary(), $sign);
+        return new AST\ArithmeticFactor($primary, $sign);
     }
 
     /**

lib/Doctrine/ORM/Query/TreeWalker.php

@@ -218,6 +218,14 @@ interface TreeWalker
      */
     function walkWhereClause($whereClause);
 
+    /**
+     * Walks down a ConditionalExpression AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalExpression
+     * @return string The SQL.
+     */
+    function walkConditionalExpression($condExpr);
+
     /**
      * Walks down a ConditionalTerm AST node, thereby generating the appropriate SQL.
      *
@@ -234,6 +242,14 @@ interface TreeWalker
      */
     function walkConditionalFactor($factor);
 
+    /**
+     * Walks down a ConditionalPrimary AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalPrimary
+     * @return string The SQL.
+     */
+    function walkConditionalPrimary($primary);
+
     /**
      * Walks down an ExistsExpression AST node, thereby generating the appropriate SQL.
      *

lib/Doctrine/ORM/Query/TreeWalkerAdapter.php

@@ -252,6 +252,14 @@ abstract class TreeWalkerAdapter implements TreeWalker
      */
     public function walkWhereClause($whereClause) {}
 
+    /**
+     * Walks down a ConditionalExpression AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalExpression
+     * @return string The SQL.
+     */
+    public function walkConditionalExpression($condExpr) {}
+
     /**
      * Walks down a ConditionalTerm AST node, thereby generating the appropriate SQL.
      *
@@ -268,6 +276,14 @@ abstract class TreeWalkerAdapter implements TreeWalker
      */
     public function walkConditionalFactor($factor) {}
 
+    /**
+     * Walks down a ConditionalPrimary AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalPrimary
+     * @return string The SQL.
+     */
+    public function walkConditionalPrimary($primary) {}
+
     /**
      * Walks down an ExistsExpression AST node, thereby generating the appropriate SQL.
      *

lib/Doctrine/ORM/Query/TreeWalkerChain.php

@@ -355,6 +355,19 @@ class TreeWalkerChain implements TreeWalker
         }
     }
 
+    /**
+     * Walks down a ConditionalExpression AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalExpression
+     * @return string The SQL.
+     */
+    public function walkConditionalExpression($condExpr)
+    {
+        foreach ($this->_walkers as $walker) {
+            $walker->walkConditionalExpression($condExpr);
+        }
+    }
+
     /**
      * Walks down a ConditionalTerm AST node, thereby generating the appropriate SQL.
      *
@@ -381,6 +394,19 @@ class TreeWalkerChain implements TreeWalker
         }
     }
 
+    /**
+     * Walks down a ConditionalPrimary AST node, thereby generating the appropriate SQL.
+     *
+     * @param ConditionalPrimary
+     * @return string The SQL.
+     */
+    public function walkConditionalPrimary($condPrimary)
+    {
+        foreach ($this->_walkers as $walker) {
+            $walker->walkConditionalPrimary($condPrimary);
+        }
+    }
+
     /**
      * Walks down an ExistsExpression AST node, thereby generating the appropriate SQL.
      *

tests/Doctrine/Tests/ORM/Functional/CustomTreeWalkersTest.php

@@ -39,20 +39,43 @@ class CustomTreeWalkersTest extends \Doctrine\Tests\OrmFunctionalTestCase
         $this->useModelSet('cms');
         parent::setUp();
     }
-    
+
+    public function assertSqlGeneration($dqlToBeTested, $sqlToBeConfirmed)
+    {
+        try {
+            $query = $this->_em->createQuery($dqlToBeTested);
+            $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, array('Doctrine\Tests\ORM\Functional\CustomTreeWalker'))
+                  ->useQueryCache(false);
+
+            parent::assertEquals($sqlToBeConfirmed, $query->getSql());
+            $query->free();
+        } catch (\Exception $e) {
+            $this->fail($e->getMessage());
+        }
+    }
+
     public function testSupportsQueriesWithoutWhere()
     {
-        
-        $q = $this->_em->createQuery('select u from Doctrine\Tests\Models\CMS\CmsUser u where u.name = :name or u.name = :otherName');
-        $q->setHint(Query::HINT_CUSTOM_TREE_WALKERS, array('Doctrine\Tests\ORM\Functional\CustomTreeWalker'));
-        
-        $this->assertEquals("SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE (c0_.name = ? OR c0_.name = ?) AND c0_.id = 1", $q->getSql());
-        
-        $q->setDql('select u from Doctrine\Tests\Models\CMS\CmsUser u');
-        $this->assertEquals("SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.id = 1", $q->getSql());
-        
-        $q->setDql('select u from Doctrine\Tests\Models\CMS\CmsUser u where u.name = :name');
-        $this->assertEquals("SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.name = ? AND c0_.id = 1", $q->getSql());
+        $this->assertSqlGeneration(
+            'select u from Doctrine\Tests\Models\CMS\CmsUser u where u.name = :name or u.name = :otherName',
+            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE (c0_.name = ? OR c0_.name = ?) AND c0_.id = 1"
+        );
+    }
+    
+    public function testSupportsQueriesWithSimpleConditionalExpressions()
+    {
+        $this->assertSqlGeneration(
+            'select u from Doctrine\Tests\Models\CMS\CmsUser u where u.name = :name',
+            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.name = ? AND c0_.id = 1"
+        );
+    }
+
+    public function testSupportsQueriesWithMultipleConditionalExpressions()
+    {
+        $this->assertSqlGeneration(
+            'select u from Doctrine\Tests\Models\CMS\CmsUser u',
+            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.id = 1"
+        );
     }
 }
 
@@ -62,6 +85,7 @@ class CustomTreeWalker extends Query\TreeWalkerAdapter
     {
         // Get the DQL aliases of all the classes we want to modify
         $dqlAliases = array();
+
         foreach ($this->_getQueryComponents() as $dqlAlias => $comp) {
             // Hard-coded check just for demonstration: We want to modify the query if
             // it involves the CmsUser class.
@@ -84,10 +108,19 @@ class CustomTreeWalker extends Query\TreeWalkerAdapter
             $factors[] = $factor;
         }
         
-        if ($selectStatement->whereClause !== null) {
+        if (($whereClause = $selectStatement->whereClause) !== null) {
             // There is already a WHERE clause, so append the conditions
-             
-            $existingTerms = $selectStatement->whereClause->conditionalExpression->conditionalTerms;
+            $condExpr = $whereClause->conditionalExpression;
+
+            // Since Phase 1 AST optimizations were included, we need to re-add the ConditionalExpression
+            if ( ! ($condExpr instanceof Query\AST\ConditionalExpression)) {
+                $condExpr = new Query\AST\ConditionalExpression(array($condExpr));
+
+                $whereClause->conditionalExpression = $condExpr;
+            }
+            
+            $existingTerms = $whereClause->conditionalExpression->conditionalTerms;
+            
             if (count($existingTerms) > 1) {
                 // More than one term, so we need to wrap all these terms in a single root term
                 // i.e: "WHERE u.name = :foo or u.other = :bar" => "WHERE (u.name = :foo or u.other = :bar) AND <our condition>"
@@ -100,8 +133,15 @@ class CustomTreeWalker extends Query\TreeWalkerAdapter
                 $selectStatement->whereClause->conditionalExpression->conditionalTerms = array($term);
             } else {
                 // Just one term so we can simply append our factors to that term
-                
                 $singleTerm = $selectStatement->whereClause->conditionalExpression->conditionalTerms[0];
+
+                // Since Phase 1 AST optimizations were included, we need to re-add the ConditionalExpression
+                if ( ! ($singleTerm instanceof Query\AST\ConditionalTerm)) {
+                    $singleTerm = new Query\AST\ConditionalTerm(array($singleTerm));
+
+                    $selectStatement->whereClause->conditionalExpression->conditionalTerms[0] = $singleTerm;
+                }
+
                 $singleTerm->conditionalFactors = array_merge($singleTerm->conditionalFactors, $factors);
                 $selectStatement->whereClause->conditionalExpression->conditionalTerms = array($singleTerm);
             }

tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php

@@ -177,7 +177,7 @@ class SelectSqlGenerationTest extends \Doctrine\Tests\OrmTestCase
     {
         $this->assertSqlGeneration(
             'SELECT u FROM Doctrine\Tests\Models\CMS\CmsUser u WHERE ((u.id + 5000) * u.id + 3) < 10000000',
-            'SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE ((c0_.id + 5000) * c0_.id + 3) < 10000000'
+            'SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE (c0_.id + 5000) * c0_.id + 3 < 10000000'
         );
     }
 
@@ -456,7 +456,7 @@ class SelectSqlGenerationTest extends \Doctrine\Tests\OrmTestCase
     {
         $this->assertSqlGeneration(
             "select u from Doctrine\Tests\Models\CMS\CmsUser u where u.id > 10 and u.id < 42 and ((u.id * 2) > 5)",
-            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.id > 10 AND c0_.id < 42 AND ((c0_.id * 2) > 5)"
+            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE c0_.id > 10 AND c0_.id < 42 AND (c0_.id * 2 > 5)"
         );
     }
 
@@ -464,7 +464,7 @@ class SelectSqlGenerationTest extends \Doctrine\Tests\OrmTestCase
     {
         $this->assertSqlGeneration(
             "select u from Doctrine\Tests\Models\CMS\CmsUser u where (u.id > 10) and (u.id < 42 and ((u.id * 2) > 5)) or u.id <> 42",
-            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE (c0_.id > 10) AND (c0_.id < 42 AND ((c0_.id * 2) > 5)) OR c0_.id <> 42"
+            "SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3 FROM cms_users c0_ WHERE (c0_.id > 10) AND (c0_.id < 42 AND (c0_.id * 2 > 5)) OR c0_.id <> 42"
         );
     }