From 949f85497ca20ef7304b851a702d332891f08b09 Mon Sep 17 00:00:00 2001 From: Jan Date: Mon, 9 Sep 2019 20:35:35 +0200 Subject: [PATCH] improve handling of FILTERs with constant expressions (#9942) --- arangod/Aql/ExecutionPlan.cpp | 23 +++++++++-- ...timizer-rule-remove-unnecessary-filters.js | 41 ++++++++++++------- tests/js/server/aql/aql-profiler.js | 4 +- 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/arangod/Aql/ExecutionPlan.cpp b/arangod/Aql/ExecutionPlan.cpp index fc7c888d4a..2f9cea8933 100644 --- a/arangod/Aql/ExecutionPlan.cpp +++ b/arangod/Aql/ExecutionPlan.cpp @@ -1173,9 +1173,26 @@ ExecutionNode* ExecutionPlan::fromNodeFilter(ExecutionNode* previous, AstNode co en = registerNode(new FilterNode(this, nextId(), v)); } else { // operand is some misc expression - auto calc = createTemporaryCalculation(expression, previous); - en = registerNode(new FilterNode(this, nextId(), getOutVariable(calc))); - previous = calc; + if (expression->isTrue()) { + // filter expression is known to be always true, so + // remove the filter entirely + return previous; + } + + // note: if isTrue() is false above, it is not necessarily the case that + // isFalse() is true next. The reason is that isTrue() and isFalse() only + // return true in case of absolulete certainty. this requires expressions + // to be based on query compile-time values only, but it will not work + // for expressions that need to be evaluated at query runtime + if (expression->isFalse()) { + // filter expression is known to be always false, so + // replace the FILTER with a NoResultsNode + en = registerNode(new NoResultsNode(this, nextId())); + } else { + auto calc = createTemporaryCalculation(expression, previous); + en = registerNode(new FilterNode(this, nextId(), getOutVariable(calc))); + previous = calc; + } } return addDependency(previous, en); diff --git a/tests/js/server/aql/aql-optimizer-rule-remove-unnecessary-filters.js b/tests/js/server/aql/aql-optimizer-rule-remove-unnecessary-filters.js index a3ff049a30..44e6deaace 100644 --- a/tests/js/server/aql/aql-optimizer-rule-remove-unnecessary-filters.js +++ b/tests/js/server/aql/aql-optimizer-rule-remove-unnecessary-filters.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false, maxlen: 500 */ -/*global assertEqual, assertTrue, AQL_EXPLAIN, AQL_EXECUTE */ +/*global assertEqual, assertTrue, assertFalse, AQL_EXPLAIN, AQL_EXECUTE */ //////////////////////////////////////////////////////////////////////////////// /// @brief tests for optimizer rules @@ -101,21 +101,30 @@ function optimizerRuleTestSuite () { testRuleHasEffect : function () { var queries = [ - "FOR i IN 1..10 FILTER true RETURN i", - "FOR i IN 1..10 FILTER 1 < 9 RETURN i", - "FOR i IN 1..10 LET a = 1 FILTER a == 1 RETURN i", - "FOR i IN 1..10 LET a = 1 LET b = 1 FILTER a == b RETURN i", - "FOR i IN 1..10 LET a = 1 LET b = 2 FILTER a != b RETURN i", - "FOR i IN 1..10 FILTER false RETURN i", - "FOR i IN 1..10 LET a = 1 FILTER a == 9 RETURN i", - "FOR i IN 1..10 LET a = 1 FILTER a != 1 RETURN i", - "FOR i IN 1..10 FILTER 1 == 1 && 2 == 2 RETURN 1", - "FOR i IN 1..10 FILTER 1 != 1 && 2 != 2 RETURN 1" + [ "FOR i IN 1..2 FILTER true RETURN i", true ], + [ "FOR i IN 1..2 FILTER 1 > 9 RETURN i", false ], + [ "FOR i IN 1..2 FILTER 1 < 9 RETURN i", true ], + [ "FOR i IN 1..2 LET a = 1 FILTER a == 1 RETURN i", true ], + [ "FOR i IN 1..2 LET a = 1 LET b = 1 FILTER a == b RETURN i", true ], + [ "FOR i IN 1..2 LET a = 1 LET b = 1 FILTER a != b RETURN i", false ], + [ "FOR i IN 1..2 LET a = 1 LET b = 2 FILTER a != b RETURN i", true ], + [ "FOR i IN 1..2 LET a = 1 LET b = 2 FILTER a == b RETURN i", false ], + [ "FOR i IN 1..2 FILTER false RETURN i", false ], + [ "FOR i IN 1..2 LET a = 1 FILTER a == 9 RETURN i", false ], + [ "FOR i IN 1..2 LET a = 1 FILTER a != 1 RETURN i", false ], + [ "FOR i IN 1..2 FILTER 1 == 1 && 2 == 2 RETURN i", true ], + [ "FOR i IN 1..2 FILTER 1 != 1 && 2 != 2 RETURN i", false ], ]; queries.forEach(function(query) { - var result = AQL_EXPLAIN(query, { }, paramEnabled); - assertEqual([ ruleName ], result.plan.rules); + var result = AQL_EXPLAIN(query[0], { }, paramEnabled); + assertEqual([ ], result.plan.rules, query); + result = AQL_EXECUTE(query[0], { }, paramEnabled).json; + if (query[1]) { + assertEqual([ 1, 2 ], result, query); + } else { + assertEqual([ ], result, query); + } }); }, @@ -138,7 +147,8 @@ function optimizerRuleTestSuite () { plans.forEach(function(plan) { var result = AQL_EXPLAIN(plan[0], { }, paramMore); - assertTrue(result.plan.rules.indexOf(ruleName) !== -1, plan[0]); + // rule will not fire anymore for constant filters + assertFalse(result.plan.rules.indexOf(ruleName) !== -1, plan[0]); assertEqual(plan[1], helper.getCompactPlan(result).map(function(node) { return node.type; }), plan[0]); }); }, @@ -169,7 +179,8 @@ function optimizerRuleTestSuite () { assertTrue(isEqual(resultDisabled, resultEnabled), query[0]); assertTrue(planDisabled.plan.rules.indexOf(ruleName) === -1, query[0]); - assertTrue(planEnabled.plan.rules.indexOf(ruleName) !== -1, query[0]); + // rule will not fire anymore for constant filters + assertTrue(planEnabled.plan.rules.indexOf(ruleName) === -1, query[0]); assertEqual(resultDisabled, query[1]); assertEqual(resultEnabled, query[1]); diff --git a/tests/js/server/aql/aql-profiler.js b/tests/js/server/aql/aql-profiler.js index 25726ecabb..a14329fd13 100644 --- a/tests/js/server/aql/aql-profiler.js +++ b/tests/js/server/aql/aql-profiler.js @@ -295,9 +295,7 @@ function ahuacatlProfilerTestSuite () { const genNodeList = (rows, batches) => [ {type: SingletonBlock, calls: 1, items: 1}, {type: CalculationBlock, calls: 1, items: 1}, - {type: CalculationBlock, calls: 1, items: 1}, {type: EnumerateListBlock, calls: batches, items: rows}, - {type: FilterBlock, calls: batches, items: rows}, {type: ReturnBlock, calls: batches, items: rows}, ]; profHelper.runDefaultChecks({query, genNodeList, options}); @@ -497,8 +495,8 @@ function ahuacatlProfilerTestSuite () { const genNodeList = () => [ {type: SingletonBlock, calls: 0, items: 0}, {type: CalculationBlock, calls: 0, items: 0}, + {type: EnumerateListBlock, calls: 0, items: 0}, {type: NoResultsBlock, calls: 1, items: 0}, - {type: EnumerateListBlock, calls: 1, items: 0}, {type: ReturnBlock, calls: 1, items: 0}, ];