From ef9ea2c21d3902e5747e7416a6dbfba630922cd3 Mon Sep 17 00:00:00 2001 From: Jan Date: Wed, 20 Nov 2019 13:42:10 +0100 Subject: [PATCH] fixed issue #10440: Incorrect sorting with sort criteria partially covered by index (#10443) * fixed issue #10440: Incorrect sorting with sort criteria partially covered by index * Update CHANGELOG --- CHANGELOG | 3 +++ arangod/Aql/OptimizerRules.cpp | 6 ------ arangod/Aql/SortCondition.cpp | 12 ++++-------- .../aql/aql-optimizer-rule-use-index-for-sort.js | 15 ++++++--------- 4 files changed, 13 insertions(+), 23 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 896a79b031..d44b1f9266 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,9 @@ v3.5.3 (XXXX-XX-XX) ------------------- +* Fixed issue #10440: Incorrect sorting with sort criteria partially covered + by index. + * Make the timeouts for replication requests (for active failover and master-slave replication configurable via startup options: diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 2a527722aa..810f3e88f7 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -3235,12 +3235,6 @@ struct SortToIndexNode final : public WalkerWorker { // sorted GatherNode in the cluster indexNode->needsGatherNodeSort(true); _modified = true; - } else if (numCovered > 0 && sortCondition.isUnidirectional()) { - // remove the first few attributes if they are constant - SortNode* sortNode = - ExecutionNode::castTo(_plan->getNodeById(_sortNode->id())); - sortNode->removeConditions(numCovered); - _modified = true; } } } diff --git a/arangod/Aql/SortCondition.cpp b/arangod/Aql/SortCondition.cpp index 2628e8029e..08c0331172 100644 --- a/arangod/Aql/SortCondition.cpp +++ b/arangod/Aql/SortCondition.cpp @@ -187,25 +187,21 @@ size_t SortCondition::coveredAttributes( } // no match - bool isConstant = false; - if (isContained(indexAttributes, field.attributes) && isContained(_constAttributes, field.attributes)) { // no field match, but a constant attribute - isConstant = true; ++fieldsPosition; ++numCovered; + continue; } - if (!isConstant && isContained(_constAttributes, indexAttributes[i])) { + if (isContained(_constAttributes, indexAttributes[i])) { // no field match, but a constant attribute - isConstant = true; ++i; // next index field + continue; } - if (!isConstant) { - break; - } + break; } TRI_ASSERT(numCovered <= _fields.size()); diff --git a/tests/js/server/aql/aql-optimizer-rule-use-index-for-sort.js b/tests/js/server/aql/aql-optimizer-rule-use-index-for-sort.js index 56b535c2da..c613714b29 100644 --- a/tests/js/server/aql/aql-optimizer-rule-use-index-for-sort.js +++ b/tests/js/server/aql/aql-optimizer-rule-use-index-for-sort.js @@ -189,17 +189,16 @@ function optimizerRuleTestSuite() { [ "FOR v IN " + colName + " FILTER v.b == 1 SORT v.b, v.a RETURN 1", false, true ], [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.b, v.a RETURN 1", true, false ], [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.a, v.b RETURN 1", true, false ], - [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.a, v.c RETURN 1", true, true ], + [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.a, v.c RETURN 1", false, true ], [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.b, v.a RETURN 1", true, false ], - [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.a, v.b, v.c RETURN 1", true, true ] + [ "FOR v IN " + colName + " FILTER v.a == 1 && v.b == 1 SORT v.a, v.b, v.c RETURN 1", false, true ] ]; queries.forEach(function(query) { var result = AQL_EXPLAIN(query[0]); if (query[1]) { assertNotEqual(-1, removeAlwaysOnClusterRules(result.plan.rules).indexOf(ruleName), query[0]); - } - else { + } else { assertEqual(-1, removeAlwaysOnClusterRules(result.plan.rules).indexOf(ruleName), query[0]); } if (query[2]) { @@ -402,7 +401,6 @@ function optimizerRuleTestSuite() { // place since the index can't fullfill all of the sorting criteria. //////////////////////////////////////////////////////////////////////////////// testSortMoreThanIndexed: function () { - var query = "FOR v IN " + colName + " FILTER v.a == 1 SORT v.a, v.c RETURN [v.a, v.b, v.c]"; // no index can be used for v.c -> sort has to remain in place! var XPresult; @@ -431,7 +429,7 @@ function optimizerRuleTestSuite() { QResults[2] = AQL_EXECUTE(query, { }, paramIndexFromSort_IndexRange).json; XPresult = AQL_EXPLAIN(query, { }, paramIndexFromSort_IndexRange); - assertEqual([ ruleName, secondRuleName ], removeAlwaysOnClusterRules(XPresult.plan.rules).sort()); + assertEqual([ secondRuleName ], removeAlwaysOnClusterRules(XPresult.plan.rules).sort()); // The sortnode and its calculation node should not have been removed. hasSortNode(XPresult); hasCalculationNodes(XPresult, 4); @@ -1124,7 +1122,7 @@ function optimizerRuleTestSuite() { testSortModifyFilterCondition : function () { var query = "FOR v IN " + colName + " FILTER v.a == 123 SORT v.a, v.xxx RETURN v"; var rules = AQL_EXPLAIN(query).plan.rules; - assertNotEqual(-1, rules.indexOf(ruleName)); + assertEqual(-1, rules.indexOf(ruleName)); assertNotEqual(-1, rules.indexOf(secondRuleName)); assertNotEqual(-1, rules.indexOf("remove-filter-covered-by-index")); @@ -1135,9 +1133,8 @@ function optimizerRuleTestSuite() { ++seen; assertFalse(node.reverse); } else if (node.type === "SortNode") { - // first sort condition (v.a) should have been removed because it is const ++seen; - assertEqual(1, node.elements.length); + assertEqual(2, node.elements.length); } }); assertEqual(2, seen);