From daac8ee3e78b2caa26f4434dd85c5895082a51b6 Mon Sep 17 00:00:00 2001
From: jsteemann <jan@arangodb.com>
Date: Thu, 14 Nov 2019 22:00:04 +0100
Subject: [PATCH] fixed issue #10440: Incorrect sorting with sort criteria
 partially covered by index

---
 arangod/Aql/OptimizerRules.cpp                    |  6 ------
 arangod/Aql/SortCondition.cpp                     | 12 ++++--------
 .../aql/aql-optimizer-rule-use-index-for-sort.js  | 15 ++++++---------
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp
index f7af804c4fd7..60a0cdaad774 100644
--- a/arangod/Aql/OptimizerRules.cpp
+++ b/arangod/Aql/OptimizerRules.cpp
@@ -3233,12 +3233,6 @@ struct SortToIndexNode final : public WalkerWorker<ExecutionNode> {
             // 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<SortNode*>(_plan->getNodeById(_sortNode->id()));
-            sortNode->removeConditions(numCovered);
-            _modified = true;
           }
         }
       }
diff --git a/arangod/Aql/SortCondition.cpp b/arangod/Aql/SortCondition.cpp
index b5bb540e9e2f..4e2112174fba 100644
--- a/arangod/Aql/SortCondition.cpp
+++ b/arangod/Aql/SortCondition.cpp
@@ -189,25 +189,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 90e63faaa4b1..624a1af322e7 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
@@ -196,17 +196,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]) {
@@ -409,7 +408,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;
@@ -438,7 +436,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);
@@ -1193,7 +1191,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"));
 
@@ -1204,9 +1202,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);