remove sorts from GatherNode if not required (#5019)

arangod/Aql/ExecutionNode.cpp

@@ -130,7 +130,7 @@ void ExecutionNode::getSortElements(SortElementVector& elements,
     Variable* v = Variable::varFromVPack(plan->getAst(), it, "inVariable");
     elements.emplace_back(v, ascending);
     // Is there an attribute path?
-    VPackSlice path = it.get("paths");
+    VPackSlice path = it.get("path");
     if (path.isArray()) {
       // Get a list of strings out and add to the path:
       auto& element = elements.back();

arangod/Aql/IndexNode.cpp

@@ -28,7 +28,7 @@
 #include "Aql/ExecutionPlan.h"
 #include "Aql/IndexBlock.h"
 #include "Aql/Query.h"
-#include "Logger/Logger.h"
+#include "Basics/VelocyPackHelper.h"
 #include "Transaction/Methods.h"
 
 #include <velocypack/Iterator.h>
@@ -48,7 +48,8 @@ IndexNode::IndexNode(ExecutionPlan* plan, size_t id, TRI_vocbase_t* vocbase,
         _collection(collection),
         _indexes(indexes),
         _condition(condition),
-        _reverse(reverse) {
+        _reverse(reverse),
+        _needsGatherNodeSort(false) {
   TRI_ASSERT(_vocbase != nullptr);
   TRI_ASSERT(_collection != nullptr);
   TRI_ASSERT(_condition != nullptr);
@@ -63,7 +64,9 @@ IndexNode::IndexNode(ExecutionPlan* plan, arangodb::velocypack::Slice const& bas
           base.get("collection").copyString())),
       _indexes(),
       _condition(nullptr),
-      _reverse(base.get("reverse").getBoolean()) {
+      _reverse(base.get("reverse").getBoolean()),
+      _needsGatherNodeSort(basics::VelocyPackHelper::readBooleanValue(base, "needsGatherNodeSort", false)) {
+
   TRI_ASSERT(_vocbase != nullptr);
   TRI_ASSERT(_collection != nullptr);
 
@@ -121,6 +124,7 @@ void IndexNode::toVelocyPackHelper(VPackBuilder& nodes, bool verbose) const {
   nodes.add(VPackValue("condition"));
   _condition->toVelocyPack(nodes, verbose);
   nodes.add("reverse", VPackValue(_reverse));
+  nodes.add("needsGatherNodeSort", VPackValue(_needsGatherNodeSort));
 
   // And close it:
   nodes.close();
@@ -146,6 +150,8 @@ ExecutionNode* IndexNode::clone(ExecutionPlan* plan, bool withDependencies,
   auto c = new IndexNode(plan, _id, _vocbase, _collection, outVariable,
                          _indexes, _condition->clone(), _reverse);
 
+  c->needsGatherNodeSort(_needsGatherNodeSort);
+
   cloneHelper(c, withDependencies, withProperties);
 
   return static_cast<ExecutionNode*>(c);

arangod/Aql/IndexNode.h

@@ -78,6 +78,11 @@ class IndexNode : public ExecutionNode, public DocumentProducingNode {
   /// @brief set reverse mode  
   void reverse(bool value) { _reverse = value; }
 
+  /// @brief whether or not the index node needs a post sort of the results
+  /// of multiple shards in the cluster
+  bool needsGatherNodeSort() const { return _needsGatherNodeSort; }
+  void needsGatherNodeSort(bool value) { _needsGatherNodeSort = value; }
+
   /// @brief export to VelocyPack
   void toVelocyPackHelper(arangodb::velocypack::Builder&,
                           bool) const override final;
@@ -126,6 +131,9 @@ class IndexNode : public ExecutionNode, public DocumentProducingNode {
 
   /// @brief the index sort order - this is the same order for all indexes
   bool _reverse;
+
+  /// @brief the index sort order - this is the same order for all indexes
+  bool _needsGatherNodeSort;
 };
 
 }  // namespace arangodb::aql

arangod/Aql/OptimizerRules.cpp

@@ -1943,6 +1943,7 @@ struct SortToIndexNode final : public WalkerWorker<ExecutionNode> {
         // sort condition is fully covered by index... now we can remove the
         // sort node from the plan
         _plan->unlinkNode(_plan->getNodeById(_sortNode->id()));
+        indexNode->needsGatherNodeSort(true); 
         _modified = true;
         handled = true;
       }
@@ -2017,6 +2018,7 @@ struct SortToIndexNode final : public WalkerWorker<ExecutionNode> {
             // no need to sort
             _plan->unlinkNode(_plan->getNodeById(_sortNode->id()));
             indexNode->reverse(sortCondition.isDescending());
+            indexNode->needsGatherNodeSort(true); 
             _modified = true;
           } else if (numCovered > 0 && sortCondition.isUnidirectional()) {
             // remove the first few attributes if they are constant
@@ -2746,7 +2748,7 @@ void arangodb::aql::scatterInClusterRule(Optimizer* opt,
 
       // Using Index for sort only works if all indexes are equal.
       auto first = allIndexes[0].getIndex();
-      if (first->isSorted()) {
+      if (first->isSorted() && idxNode->needsGatherNodeSort()) {
         for (auto const& path : first->fieldNames()) {
           elements.emplace_back(sortVariable, !isSortReverse, path);
         }

js/server/tests/aql/aql-gather-block-cluster.js

@@ -1,5 +1,5 @@
 /*jshint globalstrict:false, strict:false, maxlen: 500 */
-/*global assertEqual, assertTrue, AQL_EXPLAIN, AQL_EXECUTE */
+/*global assertEqual, assertNotEqual, assertTrue, assertFalse, AQL_EXPLAIN, AQL_EXECUTE */
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief tests for optimizer rules
@@ -84,6 +84,205 @@ function gatherBlockTestSuite () {
       c2 = null;
       c3 = null;
     },
+
+    testSingleShard : function () {
+      let query = "FOR doc IN " + cn2 + " SORT doc.Hallo RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertTrue(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual([], gatherNode.elements); // no sorting in GatherNode
+    },
+    
+    testSingleShardDescending : function () {
+      let query = "FOR doc IN " + cn2 + " SORT doc.Hallo DESC RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = 99999999999;
+      result.forEach(function(value) {
+        assertTrue(value <= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertFalse(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual([], gatherNode.elements); // no sorting in GatherNode
+    },
+
+    testSingleShardWithIndex : function () {
+      c2.ensureIndex({ type: "skiplist", fields: ["Hallo"] });
+      let query = "FOR doc IN " + cn2 + " SORT doc.Hallo RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have no sort but a gather node
+      assertNotEqual(-1, nodeTypes.indexOf("IndexNode"));
+      let indexNode = nodes[nodeTypes.indexOf("IndexNode")];
+      assertFalse(indexNode.reverse);
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual([], gatherNode.elements); // no sorting in GatherNode
+    },
+    
+    testSingleShardWithIndexDescending : function () {
+      c2.ensureIndex({ type: "skiplist", fields: ["Hallo"] });
+      let query = "FOR doc IN " + cn2 + " SORT doc.Hallo DESC RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = 99999999999;
+      result.forEach(function(value) {
+        assertTrue(value <= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have no sort but a gather node
+      assertNotEqual(-1, nodeTypes.indexOf("IndexNode"));
+      let indexNode = nodes[nodeTypes.indexOf("IndexNode")];
+      assertTrue(indexNode.reverse);
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual([], gatherNode.elements); // no sorting in GatherNode
+    },
+    
+    testMultipleShards : function () {
+      let query = "FOR doc IN " + cn1 + " SORT doc.Hallo RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must do sorting in GatherNode
+      assertTrue(gatherNode.elements[0].ascending);
+    },
+    
+    testMultipleShardsDescending : function () {
+      let query = "FOR doc IN " + cn1 + " SORT doc.Hallo DESC RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = 99999999999;
+      result.forEach(function(value) {
+        assertTrue(value <= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertEqual(-1, nodeTypes.indexOf("IndexNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length); // must do sorting in SortNode
+      assertFalse(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must do sorting in GatherNode
+      assertFalse(gatherNode.elements[0].ascending);
+    },
+    
+    testMultipleShardsWithIndex : function () {
+      c1.ensureIndex({ type: "skiplist", fields: ["Hallo"] });
+      let query = "FOR doc IN " + cn1 + " SORT doc.Hallo RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have no sort but a gather node
+      assertNotEqual(-1, nodeTypes.indexOf("IndexNode"));
+      let indexNode = nodes[nodeTypes.indexOf("IndexNode")];
+      assertFalse(indexNode.reverse);
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must do sorting in GatherNode
+      assertEqual(["Hallo"], gatherNode.elements[0].path); 
+      assertTrue(gatherNode.elements[0].ascending);
+    },
+    
+    testMultipleShardsWithIndexDescending : function () {
+      c1.ensureIndex({ type: "skiplist", fields: ["Hallo"] });
+      let query = "FOR doc IN " + cn1 + " SORT doc.Hallo DESC RETURN doc.Hallo";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      let last = 99999999999;
+      result.forEach(function(value) {
+        assertTrue(value <= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have no sort but a gather node
+      assertNotEqual(-1, nodeTypes.indexOf("IndexNode"));
+      let indexNode = nodes[nodeTypes.indexOf("IndexNode")];
+      assertTrue(indexNode.reverse);
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must do sorting in GatherNode
+      assertEqual(["Hallo"], gatherNode.elements[0].path); 
+      assertFalse(gatherNode.elements[0].ascending);
+    },
+    
+    testMultipleShardsCollect : function () {
+      c1.ensureIndex({ type: "skiplist", fields: ["Hallo"] });
+      let query = "FOR doc IN " + cn1 + " FILTER doc.Hallo == 10 COLLECT WITH COUNT INTO length RETURN length";
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have no sort but a gather node
+      assertNotEqual(-1, nodeTypes.indexOf("IndexNode"));
+      let indexNode = nodes[nodeTypes.indexOf("IndexNode")];
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(0, gatherNode.elements.length); // no sorting here
+    },
     
     testSubqueryValuePropagation : function () {
       c3.truncate();