fixed invalid optimization, added test cases

CHANGELOG

@@ -1,6 +1,11 @@
 v2.4.0 (XXXX-XX-XX)
 -------------------
 
+* added more optimizations of constants values in AQL FILTER conditions
+
+* fixed invalid or-to-in optimization for FILTERs containing comparisons
+  with boolean values
+
 * fixed replication of `_graphs` collection
 
 * added AQL list functions `PUSH`, `POP`, `UNSHIFT`, `SHIFT`, `REMOVE_VALUES`,

UnitTests/Makefile.unittests

@@ -555,6 +555,7 @@ SHELL_SERVER_AQL = @top_srcdir@/js/server/tests/aql-arithmetic.js \
 			@top_srcdir@/js/server/tests/aql-modify-noncluster-serializetest.js \
 			@top_srcdir@/js/server/tests/aql-operators.js \
 			@top_srcdir@/js/server/tests/aql-optimizer-dynamic-bounds.js \
+			@top_srcdir@/js/server/tests/aql-optimizer-filters.js \
 			@top_srcdir@/js/server/tests/aql-optimizer-rule-interchange-adjacent-enumerations-noncluster.js \
 			@top_srcdir@/js/server/tests/aql-optimizer-rule-move-calculations-up.js \
 			@top_srcdir@/js/server/tests/aql-optimizer-rule-move-filters-up.js \

arangod/Aql/Ast.cpp

@@ -905,7 +905,7 @@ void Ast::injectBindParameters (BindParameters& parameters) {
 
 AstNode* Ast::replaceVariables (AstNode* node,
                                 std::unordered_map<VariableId, Variable const*> const& replacements) {
-  auto func = [&](AstNode* node, void*) -> AstNode* {
+  auto visitor = [&](AstNode* node, void*) -> AstNode* {
     if (node == nullptr) {
       return nullptr;
     }
@@ -926,7 +926,7 @@ AstNode* Ast::replaceVariables (AstNode* node,
     return node;
   };
 
-  return traverse(node, func, nullptr);
+  return traverse(node, visitor, nullptr);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -934,7 +934,19 @@ AstNode* Ast::replaceVariables (AstNode* node,
 ////////////////////////////////////////////////////////////////////////////////
 
 void Ast::optimize () {
-  auto func = [&](AstNode* node, void*) -> AstNode* {
+  auto preVisitor = [&](AstNode const* node, void* data) -> void {
+    if (node->type == NODE_TYPE_FILTER) {
+      *(static_cast<bool*>(data)) = true;
+    }
+  };
+
+  auto postVisitor = [&](AstNode const* node, void* data) -> void {
+    if (node->type == NODE_TYPE_FILTER) {
+      *(static_cast<bool*>(data)) = false;
+    }
+  };
+
+  auto visitor = [&](AstNode* node, void* data) -> AstNode* {
     if (node == nullptr) {
       return nullptr;
     }
@@ -952,7 +964,7 @@ void Ast::optimize () {
     // binary operators
     if (node->type == NODE_TYPE_OPERATOR_BINARY_AND ||
         node->type == NODE_TYPE_OPERATOR_BINARY_OR) {
-      return optimizeBinaryOperatorLogical(node);
+      return optimizeBinaryOperatorLogical(node, *(static_cast<bool*>(data)));
     }
     
     if (node->type == NODE_TYPE_OPERATOR_BINARY_EQ ||
@@ -1007,8 +1019,9 @@ void Ast::optimize () {
     return node;
   };
 
-  // optimization
+  // run the optimizations
-  _root = traverse(_root, func, nullptr);
+  bool canModifyResultType = false;
+  _root = traverse(_root, preVisitor, visitor, postVisitor, &canModifyResultType);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1016,7 +1029,7 @@ void Ast::optimize () {
 ////////////////////////////////////////////////////////////////////////////////
 
 std::unordered_set<Variable*> Ast::getReferencedVariables (AstNode const* node) {
-  auto func = [&](AstNode const* node, void* data) -> void {
+  auto visitor = [&](AstNode const* node, void* data) -> void {
     if (node == nullptr) {
       return;
     }
@@ -1037,7 +1050,7 @@ std::unordered_set<Variable*> Ast::getReferencedVariables (AstNode const* node)
   };
 
   std::unordered_set<Variable*> result;  
-  traverse(node, func, &result); 
+  traverse(node, visitor, &result); 
 
   return result;
 }
@@ -1287,7 +1300,8 @@ AstNode* Ast::optimizeUnaryOperatorLogical (AstNode* node) {
 /// @brief optimizes the binary logical operators && and ||
 ////////////////////////////////////////////////////////////////////////////////
 
-AstNode* Ast::optimizeBinaryOperatorLogical (AstNode* node) {
+AstNode* Ast::optimizeBinaryOperatorLogical (AstNode* node,
+                                             bool canModifyResultType) {
   TRI_ASSERT(node != nullptr);
   TRI_ASSERT(node->type == NODE_TYPE_OPERATOR_BINARY_AND ||
              node->type == NODE_TYPE_OPERATOR_BINARY_OR);
@@ -1301,8 +1315,8 @@ AstNode* Ast::optimizeBinaryOperatorLogical (AstNode* node) {
   }
 
   if (lhs->isConstant()) {
+    // left operand is a constant value
     if (node->type == NODE_TYPE_OPERATOR_BINARY_AND) {
-      // left operand is a constant value
       if (lhs->isFalse()) {
         // return it if it is falsey
         return lhs;
@@ -1312,7 +1326,6 @@ AstNode* Ast::optimizeBinaryOperatorLogical (AstNode* node) {
       return rhs;
     }
     else if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) {
-      // left operand is a constant value
       if (lhs->isTrue()) {
         // return it if it is trueish
         return lhs;
@@ -1323,6 +1336,29 @@ AstNode* Ast::optimizeBinaryOperatorLogical (AstNode* node) {
     }
   }
 
+  if (canModifyResultType) {
+    if (rhs->isConstant() && ! lhs->canThrow()) {
+      // right operand is a constant value
+      if (node->type == NODE_TYPE_OPERATOR_BINARY_AND) {
+        if (rhs->isFalse()) {
+          return createNodeValueBool(false);
+        }
+
+        // right-operand was trueish, now return it
+        return lhs;
+      }
+      else if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) {
+        if (rhs->isTrue()) {
+          // return it if it is trueish
+          return createNodeValueBool(true);
+        }
+
+        // right-operand was falsey, now return left operand
+        return lhs;
+      }
+    }
+  }
+
   // default case
   return node;
 }
@@ -1815,11 +1851,44 @@ AstNode* Ast::nodeFromJson (TRI_json_t const* json) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-/// @brief traverse the AST
+/// @brief traverse the AST, using pre- and post-order visitors
 ////////////////////////////////////////////////////////////////////////////////
 
 AstNode* Ast::traverse (AstNode* node, 
-                        std::function<AstNode*(AstNode*, void*)> func,
+                        std::function<void(AstNode const*, void*)> preVisitor,
+                        std::function<AstNode*(AstNode*, void*)> visitor,
+                        std::function<void(AstNode const*, void*)> postVisitor,
+                        void* data) {
+  if (node == nullptr) {
+    return nullptr;
+  }
+ 
+  preVisitor(node, data);
+  size_t const n = node->numMembers();
+
+  for (size_t i = 0; i < n; ++i) {
+    auto member = node->getMember(i);
+
+    if (member != nullptr) {
+      AstNode* result = traverse(member, preVisitor, visitor, postVisitor, data);
+
+      if (result != node) {
+        node->changeMember(i, result);
+      }
+    }
+  }
+
+  auto result = visitor(node, data);
+  postVisitor(node, data);
+  return result;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief traverse the AST, using a visitor
+////////////////////////////////////////////////////////////////////////////////
+
+AstNode* Ast::traverse (AstNode* node, 
+                        std::function<AstNode*(AstNode*, void*)> visitor,
                         void* data) {
   if (node == nullptr) {
     return nullptr;
@@ -1831,7 +1900,7 @@ AstNode* Ast::traverse (AstNode* node,
     auto member = node->getMember(i);
 
     if (member != nullptr) {
-      AstNode* result = traverse(member, func, data);
+      AstNode* result = traverse(member, visitor, data);
 
       if (result != node) {
         node->changeMember(i, result);
@@ -1839,15 +1908,15 @@ AstNode* Ast::traverse (AstNode* node,
     }
   }
 
-  return func(node, data);
+  return visitor(node, data);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-/// @brief traverse the AST, with const nodes
+/// @brief traverse the AST using a visitor, with const nodes
 ////////////////////////////////////////////////////////////////////////////////
 
 void Ast::traverse (AstNode const* node, 
-                    std::function<void(AstNode const*, void*)> func,
+                    std::function<void(AstNode const*, void*)> visitor,
                     void* data) {
   if (node == nullptr) {
     return;
@@ -1859,11 +1928,11 @@ void Ast::traverse (AstNode const* node,
     auto member = node->getMember(i);
 
     if (member != nullptr) {
-      traverse(const_cast<AstNode const*>(member), func, data);
+      traverse(const_cast<AstNode const*>(member), visitor, data);
     }
   }
 
-  func(node, data);
+  visitor(node, data);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

arangod/Aql/Ast.h

@@ -543,7 +543,7 @@ namespace triagens {
 /// @brief optimizes the binary logical operators && and ||
 ////////////////////////////////////////////////////////////////////////////////
 
-        AstNode* optimizeBinaryOperatorLogical (AstNode*);
+        AstNode* optimizeBinaryOperatorLogical (AstNode*, bool);
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief optimizes the binary relational operators <, <=, >, >=, ==, != and IN
@@ -602,7 +602,17 @@ namespace triagens {
         AstNode* nodeFromJson (TRI_json_t const*);
 
 ////////////////////////////////////////////////////////////////////////////////
-/// @brief traverse the AST
+/// @brief traverse the AST, using pre- and post-order visitors
+////////////////////////////////////////////////////////////////////////////////
+
+        static AstNode* traverse (AstNode*,
+                                  std::function<void(AstNode const*, void*)>,
+                                  std::function<AstNode*(AstNode*, void*)>,
+                                  std::function<void(AstNode const*, void*)>,
+                                  void*);
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief traverse the AST using a visitor
 ////////////////////////////////////////////////////////////////////////////////
 
         static AstNode* traverse (AstNode*,
@@ -610,7 +620,7 @@ namespace triagens {
                                   void*);
 
 ////////////////////////////////////////////////////////////////////////////////
-/// @brief traverse the AST, with const nodes
+/// @brief traverse the AST using a visitor, with const nodes
 ////////////////////////////////////////////////////////////////////////////////
 
         static void traverse (AstNode const*,

arangod/Aql/OptimizerRules.cpp

@@ -2608,7 +2608,7 @@ struct CommonNodeFinder {
   bool find (AstNode const*  node, 
              AstNodeType     condition,
              AstNode const*& commonNode,
-             std::string&    commonName ) {
+             std::string&    commonName) {
   
     if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) {
       return (find(node->getMember(0), condition, commonNode, commonName) 
@@ -2728,7 +2728,7 @@ struct OrToInConverter {
   }
 
   bool canConvertExpression (AstNode const* node) {
-    if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_EQ, commonNode, commonName)){
+    if (finder.find(node, NODE_TYPE_OPERATOR_BINARY_EQ, commonNode, commonName)){
       return canConvertExpressionWalker(node);
     }
     return false;
@@ -2762,9 +2762,7 @@ struct OrToInConverter {
              node->type == NODE_TYPE_INDEXED_ACCESS) {
       // get a string representation of the node for comparisons 
       return (node->toString() == commonName);
-    } else if (node->isBoolValue()) {
+    } 
-      return true;
-    }
 
     return false;
   }

js/server/tests/aql-optimizer-filters.js

@@ -0,0 +1,144 @@
+/*jshint strict: false, maxlen: 500 */
+/*global require, assertEqual, assertNotEqual, AQL_EXPLAIN, AQL_EXECUTE */
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief tests for filters
+///
+/// @file
+///
+/// DISCLAIMER
+///
+/// Copyright 2010-2012 triagens GmbH, Cologne, Germany
+///
+/// Licensed under the Apache License, Version 2.0 (the "License");
+/// you may not use this file except in compliance with the License.
+/// You may obtain a copy of the License at
+///
+///     http://www.apache.org/licenses/LICENSE-2.0
+///
+/// Unless required by applicable law or agreed to in writing, software
+/// distributed under the License is distributed on an "AS IS" BASIS,
+/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+/// See the License for the specific language governing permissions and
+/// limitations under the License.
+///
+/// Copyright holder is triAGENS GmbH, Cologne, Germany
+///
+/// @author Jan Steemann
+/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
+////////////////////////////////////////////////////////////////////////////////
+
+var jsunity = require("jsunity");
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test suite
+////////////////////////////////////////////////////////////////////////////////
+
+function optimizerFiltersTestSuite () {
+
+  return {
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief filters that should be optimized away
+////////////////////////////////////////////////////////////////////////////////
+
+    testOptimizeAwayFalseFilter : function () {
+      var queries = [ 
+        "FOR i IN 1..10 FILTER i == 1 && false RETURN i",
+        "FOR i IN 1..10 FILTER (i == 1 || i == 2) && false RETURN i",
+        "FOR i IN 1..10 FILTER false && i == 1 RETURN i",
+        "FOR i IN 1..10 FILTER false && (i == 1 || i == 2) RETURN i",
+        "FOR i IN 1..10 FILTER IS_STRING(i) && false RETURN i",
+        "FOR i IN 1..10 FILTER false && IS_STRING(i) RETURN i",
+        "FOR i IN 1..10 FILTER false && RAND() RETURN i"
+      ];
+
+      queries.forEach(function(query) {
+        var plan = AQL_EXPLAIN(query).plan;
+        var nodeTypes = plan.nodes.map(function(node) {
+          return node.type;
+        });
+
+        assertEqual("SingletonNode", nodeTypes[0], query);
+        assertNotEqual(-1, nodeTypes.indexOf("NoResultsNode"), query);
+        assertEqual("ReturnNode", nodeTypes[nodeTypes.length - 1], query);
+
+        var results = AQL_EXECUTE(query).json;
+        assertEqual([ ], results, query);
+      });
+    },
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief filters that should be optimized away
+////////////////////////////////////////////////////////////////////////////////
+
+    testOptimizeAwayTrueFilter : function () {
+      var queries = [ 
+        "FOR i IN 1..10 FILTER i == 1 || true RETURN i",
+        "FOR i IN 1..10 FILTER i == 1 || i == 2 || true RETURN i",
+        "FOR i IN 1..10 FILTER true || i == 1 RETURN i",
+        "FOR i IN 1..10 FILTER true || i == 1 || i == 2 RETURN i",
+        "FOR i IN 1..10 FILTER IS_STRING(i) || true RETURN i",
+        "FOR i IN 1..10 FILTER true || IS_STRING(i) RETURN i"
+      ];
+
+      queries.forEach(function(query) {
+        var plan = AQL_EXPLAIN(query).plan;
+        var nodeTypes = plan.nodes.map(function(node) {
+          return node.type;
+        });
+
+        assertEqual("SingletonNode", nodeTypes[0], query);
+        assertEqual(-1, nodeTypes.indexOf("FilterNode"), query);
+        assertEqual("ReturnNode", nodeTypes[nodeTypes.length - 1], query);
+
+        var results = AQL_EXECUTE(query).json;
+        assertEqual([ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ], results, query);
+      });
+    },
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief filters that should not be optimized away
+////////////////////////////////////////////////////////////////////////////////
+
+    testKeepFilter : function () {
+      var queries = [ 
+        "FOR i IN 1..10 FILTER false || i == 1 RETURN i",
+        "FOR i IN 1..10 FILTER i == 1 || false RETURN i",
+        "FOR i IN 1..10 FILTER IS_STRING(i) || false RETURN i",
+        "FOR i IN 1..10 FILTER false || IS_STRING(i) RETURN i",
+        "FOR i IN 1..10 FILTER i == 1 || i == 2 RETURN i",
+        "FOR i IN 1..10 FILTER i == 1 || (i == 2 && true) RETURN i",
+        "FOR i IN 1..10 FILTER true && i == 1 RETURN i",
+        "FOR i IN 1..10 FILTER true && (i == 1 || i == 2) RETURN i",
+        "FOR i IN 1..10 FILTER IS_STRING(i) && true RETURN i",
+        "FOR i IN 1..10 FILTER true && IS_STRING(i) RETURN i"
+      ];
+
+      queries.forEach(function(query) {
+        var plan = AQL_EXPLAIN(query).plan;
+        var nodeTypes = plan.nodes.map(function(node) {
+          return node.type;
+        });
+
+        assertEqual("SingletonNode", nodeTypes[0], query);
+        assertNotEqual(-1, nodeTypes.indexOf("FilterNode"), query);
+        assertEqual("ReturnNode", nodeTypes[nodeTypes.length - 1], query);
+      });
+    }
+
+  };
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief executes the test suite
+////////////////////////////////////////////////////////////////////////////////
+
+jsunity.run(optimizerFiltersTestSuite);
+
+return jsunity.done();
+
+// Local Variables:
+// mode: outline-minor
+// outline-regexp: "^\\(/// @brief\\|/// @addtogroup\\|// --SECTION--\\|/// @page\\|/// @}\\)"
+// End:

js/server/tests/aql-optimizer-rule-replace-or-with-in.js

@@ -386,9 +386,15 @@ function NewAqlReplaceORWithINTestSuite () {
         + "FILTER i.a.b == a || i.a.b == b RETURN i";
 
       isRuleUsed(query, {});
-
     },
-    
+
+    testFiresLongAttributes : function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.a.b.c.d == 1 || x.a.b.c.d == 2 || 3 == x.a.b.c.d RETURN x";
+
+      isRuleUsed(query, {});
+    },
+
     testDudCommonConstant1: function () {
       var query = "LET x = {a:@a} FOR v IN " + replace.name() 
         + " FILTER x.a == v.value || x.a == v._key RETURN v._key";
@@ -421,6 +427,48 @@ function NewAqlReplaceORWithINTestSuite () {
       ruleIsNotUsed(query, {});
     },
 
+    testDudDifferentAttributes2: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || x == 2 RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
+    testDudDifferentAttributes3: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || 2 == x.val2 RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
+    testDudDifferentAttributes4: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || 2 == x.val2 || 3 == x.val1 RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
+    testDudDifferentAttributes5: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || 2 == x.val1 || 3 == x.val1 || 4 == x.val2 RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
+    testDudDifferentAttributesWithBool1: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || x == true RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
+    testDudDifferentAttributesWithBool2: function () {
+      var query = 
+        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || x.val2 == true RETURN x";
+
+      ruleIsNotUsed(query, {});
+    },
+
     testDudDifferentVariables : function () {
       var query = 
         "FOR y IN " + replace.name() + " FOR x IN " + replace.name() 
@@ -428,13 +476,6 @@ function NewAqlReplaceORWithINTestSuite () {
 
       ruleIsNotUsed(query, {});
     },
-    
-    testDudDifferentAttributes2: function () {
-      var query = 
-        "FOR x IN " + replace.name() + " FILTER x.val1 == 1 || x == 2 RETURN x";
-
-      ruleIsNotUsed(query, {});
-    },
 
     testDudNoOR1: function () {
       var query = 
@@ -469,9 +510,15 @@ function NewAqlReplaceORWithINTestSuite () {
 
     testDudAttributeIsList: function () {
       var query = 
-        "LET x = {a:1,b:2} FOR v IN " + replace.name() 
+        "LET x = {a:1, b:2} FOR v IN " + replace.name() 
         + " FILTER v.x[0] == x.a || v.x[1] == 3 SORT v.value RETURN v.value";
       ruleIsNotUsed(query, {});
+    },
+
+    testDudNested: function () {
+      var query = 
+        "FOR i IN [ { att1: 'foo', att2: true }, { att1: 'bar', att2: false } ] FILTER i.att1 == 'bar' || i.att2 == true RETURN i";
+      ruleIsNotUsed(query, {});
     }
   };
 }