diff --git a/CHANGELOG b/CHANGELOG index 1435fe2f00..2815fdfbbb 100644 --- a/CHANGELOG +++ b/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`, diff --git a/UnitTests/Makefile.unittests b/UnitTests/Makefile.unittests index 5a7510744a..e626b7907a 100755 --- a/UnitTests/Makefile.unittests +++ b/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 \ diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index e7d9b69b52..bc3f597066 100644 --- a/arangod/Aql/Ast.cpp +++ b/arangod/Aql/Ast.cpp @@ -905,7 +905,7 @@ void Ast::injectBindParameters (BindParameters& parameters) { AstNode* Ast::replaceVariables (AstNode* node, std::unordered_map 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(data)) = true; + } + }; + + auto postVisitor = [&](AstNode const* node, void* data) -> void { + if (node->type == NODE_TYPE_FILTER) { + *(static_cast(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(data))); } if (node->type == NODE_TYPE_OPERATOR_BINARY_EQ || @@ -1007,8 +1019,9 @@ void Ast::optimize () { return node; }; - // optimization - _root = traverse(_root, func, nullptr); + // run the optimizations + bool canModifyResultType = false; + _root = traverse(_root, preVisitor, visitor, postVisitor, &canModifyResultType); } //////////////////////////////////////////////////////////////////////////////// @@ -1016,7 +1029,7 @@ void Ast::optimize () { //////////////////////////////////////////////////////////////////////////////// std::unordered_set 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 Ast::getReferencedVariables (AstNode const* node) }; std::unordered_set 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 func, + std::function preVisitor, + std::function visitor, + std::function 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 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 func, + std::function 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(member), func, data); + traverse(const_cast(member), visitor, data); } } - func(node, data); + visitor(node, data); } //////////////////////////////////////////////////////////////////////////////// diff --git a/arangod/Aql/Ast.h b/arangod/Aql/Ast.h index 79536713f2..dc317c6224 100644 --- a/arangod/Aql/Ast.h +++ b/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, + std::function, + std::function, + 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*, diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 163314319c..377aca91f7 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/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; } diff --git a/js/server/tests/aql-optimizer-filters.js b/js/server/tests/aql-optimizer-filters.js new file mode 100644 index 0000000000..d8058f06d4 --- /dev/null +++ b/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: diff --git a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js index 438ffde479..a966f4b1cb 100644 --- a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js +++ b/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, {}); } }; }