ensure query is properly optimized after replacement of scorer functions

arangodb · gnusi · Jan 10, 2019 · Dec 21, 2018 · Dec 23, 2018 · Dec 24, 2018
commit 9bd05aed64955856a499c44b543340669301989d
diff --git a/arangod/Aql/OptimizerRule.h b/arangod/Aql/OptimizerRule.h
@@ -167,15 +167,15 @@ struct OptimizerRule {
     // remove redundant filters statements
     removeFiltersCoveredByTraversal,
 
-    // remove calculations that are redundant
-    // needs to run after filter removal
-    removeUnnecessaryCalculationsRule2,
-
 #ifdef USE_IRESEARCH
     // move filters and sort conditions into views and remove them
     handleArangoSearchViewsRule,
 #endif
 
+    // remove calculations that are redundant
+    // needs to run after filter removal
+    removeUnnecessaryCalculationsRule2,
+
     // remove now obsolete path variables
     removeTraversalPathVariable,
     prepareTraversalsRule,

diff --git a/arangod/IResearch/IResearchOrderFactory.cpp b/arangod/IResearch/IResearchOrderFactory.cpp
@@ -34,7 +34,7 @@
 #include "VelocyPackHelper.h"
 #include "Aql/Ast.h"
 #include "Aql/AstNode.h"
-#include "Aql/Expression.h"
+#include "Aql/ExecutionNode.h"
 #include "Aql/Function.h"
 #include "Aql/SortCondition.h"
 #include "Basics/fasthash.h"
@@ -239,46 +239,30 @@ NS_BEGIN(arangodb)
 NS_BEGIN(iresearch)
 
 // ----------------------------------------------------------------------------
-// --SECTION--                                      OrderFactory implementation
+// --SECTION--                                    ScorerReplacer implementation
 // ----------------------------------------------------------------------------
 
-/*static*/ bool OrderFactory::scorer(
-    irs::sort::ptr* scorer,
-    arangodb::aql::AstNode const& node,
-    arangodb::iresearch::QueryContext const& ctx
-) {
-  switch (node.type) {
-    case arangodb::aql::NODE_TYPE_FCALL: // function call
-      return fromFCall(scorer, node, ctx);
-    case arangodb::aql::NODE_TYPE_FCALL_USER: // user function call
-      return fromFCallUser(scorer, node, ctx);
-    default:
-      // IResearch does not support any
-      // expressions except function calls
-      return false;
+void ScorerReplacer::replace(aql::CalculationNode& node) {
+  if (!node.expression()) {
+    return;
   }
-}
 
-/*static*/ void OrderFactory::replaceScorers(
-    OrderFactory::VarToScorers& vars,
-    OrderFactory::DedupScorers& scorers,
-    aql::Expression& expr
-) {
+  auto& expr = *node.expression();
   auto* ast = expr.ast();
 
   if (!expr.ast()) {
     // ast is not set
     return;
   }
 
-  auto* node = expr.nodeForModification();
+  auto* exprNode = expr.nodeForModification();
 
-  if (!node) {
+  if (!exprNode) {
     // node is not set
     return;
   }
 
-  auto replaceScorers = [&scorers, ast](aql::AstNode* node) {
+  auto replaceScorers = [this, ast](aql::AstNode* node) {
     if (aql::NODE_TYPE_FCALL == node->type || aql::NODE_TYPE_FCALL_USER == node->type) {
       auto* ref = getScorerRef(node->getMember(0));
 
@@ -296,13 +280,13 @@ NS_BEGIN(iresearch)
 
       Scorer const key(ref, node);
 
-      auto it = scorers.find(key);
+      auto it = _dedup.find(key);
 
-      if (it == scorers.end()) {
+      if (it == _dedup.end()) {
         // create variable
         auto* var = ast->variables()->createTemporaryVariable();
 
-        it = scorers.emplace(key, var).first;
+        it = _dedup.emplace(key, var).first;
       }
 
       return ast->createNodeReference(it->second);
@@ -312,18 +296,48 @@ NS_BEGIN(iresearch)
   };
 
   // Try to modify root node of the expression
-  auto newNode = replaceScorers(node);
+  auto newNode = replaceScorers(exprNode);
 
-  if (node != newNode) {
+  if (exprNode != newNode) {
     // simple expression, e.g LET x = BM25(d)
     expr.replaceNode(newNode);
   } else {
-    aql::Ast::traverseAndModify(node, replaceScorers);
+    aql::Ast::traverseAndModify(exprNode, replaceScorers);
+  }
+}
+
+void ScorerReplacer::extract(
+    aql::Variable const& var,
+    std::vector<Scorer>& scorers
+) {
+  for (auto it = _dedup.begin(), end = _dedup.end(); it != end; ) {
+    if (it->first.var == &var) {
+      scorers.emplace_back(it->second, it->first.node);
+      it = _dedup.erase(it);
+    } else {
+      ++it;
+    }
   }
+}
 
-  for (auto& scorer : scorers) {
-    auto& nodes = vars[scorer.first.var];
-    nodes.emplace_back(scorer.second, scorer.first.node);
+// ----------------------------------------------------------------------------
+// --SECTION--                                      OrderFactory implementation
+// ----------------------------------------------------------------------------
+
+/*static*/ bool OrderFactory::scorer(
+    irs::sort::ptr* scorer,
+    arangodb::aql::AstNode const& node,
+    arangodb::iresearch::QueryContext const& ctx
+) {
+  switch (node.type) {
+    case arangodb::aql::NODE_TYPE_FCALL: // function call
+      return fromFCall(scorer, node, ctx);
+    case arangodb::aql::NODE_TYPE_FCALL_USER: // user function call
+      return fromFCallUser(scorer, node, ctx);
+    default:
+      // IResearch does not support any
+      // expressions except function calls
+      return false;
   }
 }
 

diff --git a/arangod/IResearch/IResearchOrderFactory.h b/arangod/IResearch/IResearchOrderFactory.h
@@ -35,6 +35,7 @@ NS_BEGIN(aql)
 
 class Ast;
 struct AstNode;
+class CalculationNode;
 struct Expression;
 struct Variable;
 
@@ -54,53 +55,10 @@ NS_END // transaction
 
 NS_BEGIN(iresearch)
 
+////////////////////////////////////////////////////////////////////////////////
+/// @struct OrderFactory
+////////////////////////////////////////////////////////////////////////////////
 struct OrderFactory {
-  struct Scorer {
-    Scorer() = default;
-
-    constexpr Scorer(
-        aql::Variable const* var,
-        aql::AstNode const* node
-    ) noexcept
-      : var(var), node(node) {
-    }
-
-    constexpr bool operator==(Scorer const& rhs) const noexcept {
-      return var == rhs.var && node == rhs.node;
-    }
-
-    constexpr bool operator!=(Scorer const& rhs) const noexcept {
-      return !(*this == rhs);
-    }
-
-    aql::Variable const* var{}; // scorer variable
-    aql::AstNode const* node{}; // scorer node
-  }; // Scorer
-
-  struct ScorerHash {
-    size_t operator()(Scorer const& key) const noexcept {
-      return iresearch::hash(key.node);
-    }
-  };
-
-  struct ScorerEqualTo {
-    bool operator()(Scorer const& lhs, Scorer const& rhs) const {
-      return iresearch::equalTo(lhs.node, rhs.node);
-    }
-  };
-
-  typedef std::map<aql::Variable const*, std::vector<Scorer>> VarToScorers; // key - loop variable
-  typedef std::unordered_map<Scorer, aql::Variable const*, ScorerHash, ScorerEqualTo> DedupScorers;
-
-  ////////////////////////////////////////////////////////////////////////////////
-  /// @brief replaces scorer functions in a specified expression
-  ////////////////////////////////////////////////////////////////////////////////
-  static void replaceScorers(
-    VarToScorers& vars,
-    DedupScorers& scorers,
-    aql::Expression& expr
-  );
-
   ////////////////////////////////////////////////////////////////////////////////
   /// @brief determine if the 'node' can be converted into an iresearch scorer
   ///        if 'scorer' != nullptr then also append build iresearch scorer there
@@ -111,12 +69,84 @@ struct OrderFactory {
     iresearch::QueryContext const& ctx
   );
 
+  ////////////////////////////////////////////////////////////////////////////////
+  /// @brief determine if the 'node' can be converted into an iresearch scorer
+  ///        if 'scorer' != nullptr then also append build iresearch comparer there
+  ////////////////////////////////////////////////////////////////////////////////
   static bool comparer(
     irs::sort::ptr* scorer,
     aql::AstNode const& node
   );
+
+  OrderFactory() = delete;
 }; // OrderFactory
 
+////////////////////////////////////////////////////////////////////////////////
+/// @struct Scorer
+/// @brief represents IResearch scorer in AQL terms
+////////////////////////////////////////////////////////////////////////////////
+struct Scorer {
+  Scorer() = default;
+
+  constexpr Scorer(
+      aql::Variable const* var,
+      aql::AstNode const* node
+  ) noexcept
+    : var(var), node(node) {
+  }
+
+  constexpr bool operator==(Scorer const& rhs) const noexcept {
+    return var == rhs.var && node == rhs.node;
+  }
+
+  constexpr bool operator!=(Scorer const& rhs) const noexcept {
+    return !(*this == rhs);
+  }
+
+  aql::Variable const* var{}; // scorer variable
+  aql::AstNode const* node{}; // scorer node
+}; // Scorer
+
+////////////////////////////////////////////////////////////////////////////////
+/// @class ScorerReplacer
+/// @brief utility class that replaces scorer function call with corresponding
+///        reference access
+////////////////////////////////////////////////////////////////////////////////
+class ScorerReplacer {
+ public:
+  ScorerReplacer() = default;
+
+  ////////////////////////////////////////////////////////////////////////////////
+  /// @brief replaces all occurences of IResearch scorers in a specified node with
+  ///        corresponding reference access
+  ////////////////////////////////////////////////////////////////////////////////
+  void replace(aql::CalculationNode& node);
+
+  ////////////////////////////////////////////////////////////////////////////////
+  /// @brief extracts replacement results for a given variable
+  ////////////////////////////////////////////////////////////////////////////////
+  void extract(aql::Variable const& var, std::vector<Scorer>& scorers);
+
+ private:
+  struct ScorerHash {
+    size_t operator()(Scorer const& key) const noexcept {
+      return iresearch::hash(key.node);
+    }
+  }; // ScorerHash
+
+  struct ScorerEqualTo {
+    bool operator()(Scorer const& lhs, Scorer const& rhs) const {
+      return iresearch::equalTo(lhs.node, rhs.node);
+    }
+  }; // ScorerEqualTo
+
+  typedef std::unordered_map<
+    Scorer, aql::Variable const*, ScorerHash, ScorerEqualTo
+  > DedupScorers;
+
  DedupScorers _dedup;
+}; // ScorerReplacer
+
 NS_END // iresearch
 NS_END // arangodb
 

diff --git a/arangod/IResearch/IResearchViewNode.cpp b/arangod/IResearch/IResearchViewNode.cpp
@@ -59,7 +59,7 @@ inline bool filterConditionIsEmpty(aql::AstNode const* filterCondition) {
 
 void toVelocyPack(
     velocypack::Builder& builder,
-    std::vector<arangodb::iresearch::OrderFactory::Scorer> const& scorers,
+    std::vector<arangodb::iresearch::Scorer> const& scorers,
     bool verbose
 ) {
   VPackArrayBuilder arrayScope(&builder);
@@ -71,7 +71,7 @@ void toVelocyPack(
   }
 }
 
-std::vector<arangodb::iresearch::OrderFactory::Scorer> fromVelocyPack(
+std::vector<arangodb::iresearch::Scorer> fromVelocyPack(
     arangodb::aql::ExecutionPlan& plan,
     arangodb::velocypack::Slice const& slice
 ) {
@@ -86,7 +86,7 @@ std::vector<arangodb::iresearch::OrderFactory::Scorer> fromVelocyPack(
   auto const* vars = plan.getAst()->variables();
   TRI_ASSERT(vars);
 
-  std::vector<arangodb::iresearch::OrderFactory::Scorer> scorers;
+  std::vector<arangodb::iresearch::Scorer> scorers;
 
   size_t i = 0;
   for (auto const sortSlice : velocypack::ArrayIterator(slice)) {
@@ -328,12 +328,12 @@ IResearchViewNode::IResearchViewNode(
     aql::ExecutionPlan& plan,
     size_t id,
     TRI_vocbase_t& vocbase,
-    std::shared_ptr<const arangodb::LogicalView> const& view,
-    arangodb::aql::Variable const& outVariable,
-    arangodb::aql::AstNode* filterCondition,
-    arangodb::aql::AstNode* options,
-    std::vector<arangodb::iresearch::OrderFactory::Scorer>&& scorers)
-  : arangodb::aql::ExecutionNode(&plan, id),
+    std::shared_ptr<const LogicalView> const& view,
+    aql::Variable const& outVariable,
+    aql::AstNode* filterCondition,
+    aql::AstNode* options,
+    std::vector<Scorer>&& scorers)
+  : aql::ExecutionNode(&plan, id),
     _vocbase(vocbase),
     _view(view),
     _outVariable(&outVariable),

diff --git a/arangod/IResearch/IResearchViewNode.h b/arangod/IResearch/IResearchViewNode.h
@@ -57,7 +57,7 @@ class IResearchViewNode final : public arangodb::aql::ExecutionNode {
     aql::Variable const& outVariable,
     aql::AstNode* filterCondition,
     aql::AstNode* options,
-    std::vector<OrderFactory::Scorer>&& scorers
+    std::vector<Scorer>&& scorers
   );
 
   IResearchViewNode(
@@ -143,12 +143,12 @@ class IResearchViewNode final : public arangodb::aql::ExecutionNode {
   }
 
   /// @brief return the condition to pass to the view
-  std::vector<OrderFactory::Scorer> const& scorers() const noexcept {
+  std::vector<Scorer> const& scorers() const noexcept {
     return _scorers;
   }
 
   /// @brief set the sort condition to pass to the view
-  void scorers(std::vector<OrderFactory::Scorer>&& scorers) noexcept {
+  void scorers(std::vector<Scorer>&& scorers) noexcept {
     _scorers = std::move(scorers);
   }
 
@@ -202,7 +202,7 @@ class IResearchViewNode final : public arangodb::aql::ExecutionNode {
   aql::AstNode const* _filterCondition;
 
   /// @brief scorers related to the view
-  std::vector<OrderFactory::Scorer> _scorers;
+  std::vector<Scorer> _scorers;
 
   /// @brief list of shards involved, need this for the cluster
   std::vector<std::string> _shards;