adress review comments

arangodb · gnusi · Sep 10, 2021 · Sep 3, 2021 · Sep 6, 2021 · Sep 6, 2021
commit 4cd83313a5417d2cf3dbeaf17ad813c22cd325c5
diff --git a/arangod/Aql/IResearchViewExecutor.cpp b/arangod/Aql/IResearchViewExecutor.cpp
@@ -151,7 +151,7 @@ IResearchViewExecutorInfos::IResearchViewExecutorInfos(
     Variable const& outVariable, aql::AstNode const& filterCondition,
     std::pair<bool, bool> volatility, IResearchViewExecutorInfos::VarInfoMap const& varInfoMap,
     int depth, IResearchViewNode::ViewValuesRegisters&& outNonMaterializedViewRegs,
-    iresearch::CountApproximate countApproximate, bool allowFiltersMerge)
+    iresearch::CountApproximate countApproximate, iresearch::FilterOptimization filterOptimization)
     : _scoreRegisters(std::move(scoreRegisters)),
       _reader(std::move(reader)),
       _query(query),
@@ -169,7 +169,7 @@ IResearchViewExecutorInfos::IResearchViewExecutorInfos(
       _outNonMaterializedViewRegs(std::move(outNonMaterializedViewRegs)),
       _countApproximate(countApproximate),
       _filterConditionIsEmpty(::filterConditionIsEmpty(&_filterCondition)),
-      _allowFiltersMerge(allowFiltersMerge) {
+      _filterOptimization(filterOptimization) {
   TRI_ASSERT(_reader != nullptr);
   std::tie(_documentOutReg, _collectionPointerReg) = std::visit(
       overload{[&](aql::IResearchViewExecutorInfos::MaterializeRegisters regs) {
@@ -602,7 +602,7 @@ void IResearchViewExecutorBase<Impl, Traits>::reset() {
   ExecutionPlan const* plan = &infos().plan();
   iresearch::QueryContext const queryCtx = { &_trx, plan, plan->getAst(),
                                             &_ctx, _reader.get(), &infos().outVariable(),
-                                            infos().allowFiltersMerge()};
+                                            infos().filterOptimization()};
 
   if (infos().volatileFilter() || !_isInitialized) {  // `_volatileSort` implies `_volatileFilter`
     irs::Or root;

diff --git a/arangod/Aql/IResearchViewExecutor.h b/arangod/Aql/IResearchViewExecutor.h
@@ -90,7 +90,7 @@ class IResearchViewExecutorInfos {
       Variable const& outVariable, aql::AstNode const& filterCondition,
       std::pair<bool, bool> volatility, VarInfoMap const& varInfoMap, int depth,
       iresearch::IResearchViewNode::ViewValuesRegisters&& outNonMaterializedViewRegs,
-      iresearch::CountApproximate, bool);
+      iresearch::CountApproximate, iresearch::FilterOptimization);
 
   auto getDocumentRegister() const noexcept -> RegisterId;
   auto getCollectionRegister() const noexcept -> RegisterId;
@@ -111,7 +111,7 @@ class IResearchViewExecutorInfos {
   bool volatileSort() const noexcept;
   bool volatileFilter() const noexcept;
   iresearch::CountApproximate countApproximate() const noexcept { return _countApproximate; }
-  bool allowFiltersMerge() const noexcept { return _allowFiltersMerge; }
+  iresearch::FilterOptimization filterOptimization() const noexcept { return _filterOptimization; }
 
   // first - sort
   // second - number of sort conditions to take into account
@@ -138,7 +138,7 @@ class IResearchViewExecutorInfos {
   iresearch::IResearchViewNode::ViewValuesRegisters _outNonMaterializedViewRegs;
   iresearch::CountApproximate _countApproximate;
   bool _filterConditionIsEmpty;
-  bool _allowFiltersMerge;
+  iresearch::FilterOptimization _filterOptimization;
 };  // IResearchViewExecutorInfos
 
 class IResearchViewStats {

diff --git a/arangod/Aql/IResearchViewNode.cpp b/arangod/Aql/IResearchViewNode.cpp
@@ -180,8 +180,8 @@ void toVelocyPack(velocypack::Builder& builder, IResearchViewNode::Options const
     }
   }
 
-  if (!options.allowFiltersMerge) {
-    builder.add("allowFiltersMerge", VPackValue(options.allowFiltersMerge));
+  if (options.filterOptimization != arangodb::iresearch::FilterOptimization::MAX) {
+    builder.add("filterOptimization", VPackValue(static_cast<int64_t>(options.filterOptimization)));
   }
 }
 
@@ -288,15 +288,16 @@ bool fromVelocyPack(velocypack::Slice optionsSlice, IResearchViewNode::Options&
     }
   }
 
-  // allowFiltersMerge
+  // filterOptimization
   {
-    auto const optionSlice = optionsSlice.get("allowFiltersMerge");
+    auto const optionSlice = optionsSlice.get("filterOptimization");
     if (!optionSlice.isNone()) {
-      // 'allowFiltersMerge' is optional
-      if (!optionSlice.isBool()) {
+      // 'filterOptimization' is optional. Missing means MAX
+      if (!optionSlice.isNumber()) {
         return false;
       }
-      options.allowFiltersMerge = optionSlice.getBool();
+      options.filterOptimization =
+        static_cast<arangodb::iresearch::FilterOptimization>(optionSlice.getNumber<int>());
     }
   }
 
@@ -456,14 +457,15 @@ bool parseOptions(aql::QueryContext& query, LogicalView const& view, aql::AstNod
          return true;
        }},
        // cppcheck-suppress constStatement
-       {"allowFiltersMerge", [](aql::QueryContext& /*query*/, LogicalView const& /*view*/,
+       {"filterOptimization", [](aql::QueryContext& /*query*/, LogicalView const& /*view*/,
                                   aql::AstNode const& value,
                                   IResearchViewNode::Options& options, std::string& error) {
-         if (!value.isValueType(aql::VALUE_TYPE_BOOL)) {
-           error = "bool value expected for option 'allowFiltersMerge'";
+         if (!value.isValueType(aql::VALUE_TYPE_INT)) {
+           error = "int value expected for option 'filterOptimization'";
            return false;
          }
-         options.allowFiltersMerge = value.getBoolValue();
+         options.filterOptimization =
+           static_cast<arangodb::iresearch::FilterOptimization>(value.getIntValue());
          return true;
        }}};
 
@@ -1688,7 +1690,7 @@ std::unique_ptr<aql::ExecutionBlock> IResearchViewNode::createBlock(
                                         getDepth(),
                                         std::move(outNonMaterializedViewRegs),
                                         _options.countApproximate,
-                                        allowFiltersMerge()};
+                                        filterOptimization()};
 
     return std::make_tuple(materializeType, std::move(executorInfos), std::move(registerInfos));
   };

diff --git a/arangod/Aql/IResearchViewNode.h b/arangod/Aql/IResearchViewNode.h
@@ -29,6 +29,7 @@
 #include "Aql/ExecutionNodeId.h"
 #include "Aql/LateMaterializedOptimizerRulesCommon.h"
 #include "Aql/types.h"
+#include "IResearch/IResearchFilterOptimization.h"
 #include "IResearch/IResearchOrderFactory.h"
 #include "IResearch/IResearchViewSort.h"
 #include "IResearch/IResearchViewStoredValues.h"
@@ -91,8 +92,8 @@ class IResearchViewNode final : public arangodb::aql::ExecutionNode {
     /// @brief skipAll method for view
     CountApproximate countApproximate{CountApproximate::Exact};
 
-    /// @brief allow merging some filters during optimization
-    bool allowFiltersMerge{true};
+    /// @brief iresearch filters optimization level
+    FilterOptimization filterOptimization {FilterOptimization::MAX};
   };  // Options
 
   IResearchViewNode(aql::ExecutionPlan& plan, aql::ExecutionNodeId id, TRI_vocbase_t& vocbase,
@@ -156,7 +157,12 @@ class IResearchViewNode final : public arangodb::aql::ExecutionNode {
 
   // we could merge if it is allowed in general and there are no scores - as changing
   // filters will affect score and we will lose backward compatibility
-  bool allowFiltersMerge() const noexcept { return _options.allowFiltersMerge && _scorers.empty(); }
+  FilterOptimization filterOptimization() const noexcept {
+    if (!_scorers.empty()) {
+      return FilterOptimization::None;
+    }
+    return _options.filterOptimization;
+  }
 
   /// @brief set the scorers to pass to the view
   void scorers(std::vector<Scorer>&& scorers) noexcept {

diff --git a/arangod/Aql/IResearchViewOptimizerRules.cpp b/arangod/Aql/IResearchViewOptimizerRules.cpp
@@ -24,6 +24,7 @@

 #include "IResearchViewOptimizerRules.h"
 
+#include "Aql/AqlFunctionFeature.h"
 #include "Aql/CalculationNodeVarFinder.h"
 #include "Aql/ClusterNodes.h"
 #include "Aql/Condition.h"
@@ -77,13 +78,11 @@ inline IResearchViewStoredValues const& storedValues(arangodb::LogicalView const
   return viewImpl.storedValues();
 }
 
-/// @brief Moves all STARTS_WITH in every AND node to the bottom
-///        to make it possible later optimize out STARTS_WITH covered by LEVENSHTEIN_MATCH
+/// @brief Moves all FCALLs in every AND node to the bottom of the AND node
 /// @param condition SEARCH condition node
-/// @param plan current query plan
-void pushStartsWithToBack(arangodb::aql::AstNode& condition) {
+/// @param starts_with Function to push
+void pushFuncToBack(arangodb::aql::AstNode& condition, arangodb::aql::Function const* starts_with) {
   auto numMembers = condition.numMembers();
-  constexpr irs::string_ref STARTS_WITH {"STARTS_WITH"};
   for (size_t memberIdx = 0; memberIdx < numMembers; ++memberIdx) {
     auto current = condition.getMemberUnchecked(memberIdx);
     TRI_ASSERT(current);
@@ -94,31 +93,31 @@ void pushStartsWithToBack(arangodb::aql::AstNode& condition) {
       do {
         auto candidate = current->getMemberUnchecked(movePoint);
         if (candidate->type != arangodb::aql::AstNodeType::NODE_TYPE_FCALL ||
-            arangodb::iresearch::getFuncName(*candidate) != STARTS_WITH) {
+            static_cast<arangodb::aql::Function const*>(candidate->getData()) != starts_with) {
           break;
         }
       } while ((--movePoint) != 0);
       for (size_t andMemberIdx = 0; andMemberIdx < movePoint; ++andMemberIdx) {
         auto andMember = current->getMemberUnchecked(andMemberIdx);
         if (andMember->type == arangodb::aql::AstNodeType::NODE_TYPE_FCALL &&
-            arangodb::iresearch::getFuncName(*andMember) == STARTS_WITH) {
+            static_cast<arangodb::aql::Function const*>(andMember->getData()) == starts_with) {
           TEMPORARILY_UNLOCK_NODE(current);
           auto tmp = current->getMemberUnchecked(movePoint);
           current->changeMember(movePoint--, andMember);
           current->changeMember(andMemberIdx, tmp);
           while (movePoint > andMemberIdx &&
                  current->getMemberUnchecked(movePoint)->type ==
                      arangodb::aql::AstNodeType::NODE_TYPE_FCALL &&
-                 arangodb::iresearch::getFuncName(
-                     *current->getMemberUnchecked(movePoint)) == STARTS_WITH) {
+                 static_cast<arangodb::aql::Function const*>(
+                     current->getMemberUnchecked(movePoint)->getData()) == starts_with) {
             --movePoint;
           }
         } else {
-          pushStartsWithToBack(*andMember);
+          pushFuncToBack(*andMember, starts_with);
         }
       }
     } else {
-      pushStartsWithToBack(*current);
+      pushFuncToBack(*current, starts_with);
     }
   }
 }
@@ -178,9 +177,12 @@ bool optimizeSearchCondition(IResearchViewNode& viewNode, arangodb::aql::QueryCo
 
   // check filter condition if present
   if (searchCondition.root()) {
-    if (viewNode.allowFiltersMerge()) {
+    if (viewNode.filterOptimization() != arangodb::iresearch::FilterOptimization::None) {
       // we could benefit from merging STARTS_WITH and LEVENSHTEIN_MATCH
-      pushStartsWithToBack(*searchCondition.root());
+      auto& server = plan.getAst()->query().vocbase().server();
+      auto starts_with = server.getFeature<AqlFunctionFeature>().byName("STARTS_WITH");
+      TRI_ASSERT(starts_with);
+      pushFuncToBack(*searchCondition.root(), starts_with);
     }
     auto filterCreated = FilterFactory::filter(
       nullptr,

diff --git a/arangod/CMakeLists.txt b/arangod/CMakeLists.txt
@@ -66,6 +66,7 @@ add_library(arango_iresearch
   IResearch/IResearchExpressionContext.cpp IResearch/IResearchExpressionContext.h
   IResearch/IResearchFeature.cpp IResearch/IResearchFeature.h
   IResearch/IResearchFilterFactory.cpp IResearch/IResearchFilterFactory.h
+  IResearch/IResearchFilterOptimization.cpp IResearch/IResearchFilterOptimization.h
   IResearch/IResearchIdentityAnalyzer.cpp IResearch/IResearchIdentityAnalyzer.h
   IResearch/IResearchKludge.cpp IResearch/IResearchKludge.h
   IResearch/IResearchLink.cpp IResearch/IResearchLink.h

diff --git a/arangod/IResearch/AqlHelper.h b/arangod/IResearch/AqlHelper.h
@@ -28,6 +28,7 @@
 #include "Aql/AstNode.h"
 #include "Aql/SortCondition.h"
 #include "VelocyPackHelper.h"
+#include "IResearch/IResearchFilterOptimization.h"
 
 #include "search/sort.hpp"
 #include "utils/noncopyable.hpp"
@@ -242,7 +243,7 @@ struct QueryContext {
   irs::index_reader const* index;
   aql::Variable const* ref;
   /// @brief allow optimize away/modify some conditions during filter building
-  bool allowFiltersMerge {false};
+  FilterOptimization  filterOptimization {FilterOptimization::MAX};
 };  // QueryContext
 
 ////////////////////////////////////////////////////////////////////////////////

diff --git a/arangod/IResearch/IResearchFilterFactory.cpp b/arangod/IResearch/IResearchFilterFactory.cpp
@@ -3572,7 +3572,7 @@ Result fromFuncStartsWith(
 
     // Try to optimize us away
     if (!isMultiPrefix && !prefixes.empty() &&
-         ctx.allowFiltersMerge && filter->type() == irs::type<irs::And>::id()) {
+         ctx.filterOptimization != FilterOptimization::None && filter->type() == irs::type<irs::And>::id()) {
       for (auto& f : *filter) {
         if (f.type() == irs::type<irs::by_edit_distance>::id()) {
           auto& levenshtein = static_cast<irs::by_edit_distance&>(f);

diff --git a/arangod/IResearch/IResearchFilterFactory.h b/arangod/IResearch/IResearchFilterFactory.h
@@ -24,6 +24,8 @@
 
 #pragma once
 
+#include "IResearchFilterOptimization.h"
+
 #include "VocBase/voc-types.h"
 
 #include "search/filter.hpp"

diff --git a/arangod/IResearch/IResearchFilterOptimization.cpp b/arangod/IResearch/IResearchFilterOptimization.cpp
@@ -0,0 +1,23 @@
+////////////////////////////////////////////////////////////////////////////////
+/// DISCLAIMER
+///
+/// Copyright 2021 ArangoDB GmbH, Cologne, Germany
+///
+/// Licensed under the Apache License, Version 2.0 (the "License");
+/// you may not use this file except in compliance with the License.
+/// You may obtain a copy of the License at
+///
+///     http://www.apache.org/licenses/LICENSE-2.0
+///
+/// Unless required by applicable law or agreed to in writing, software
+/// distributed under the License is distributed on an "AS IS" BASIS,
+/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+/// See the License for the specific language governing permissions and
+/// limitations under the License.
+///
+/// Copyright holder is ArangoDB GmbH, Cologne, Germany
+///
+/// @author Andrei Lobov
+////////////////////////////////////////////////////////////////////////////////
+
+#include "IResearch/IResearchFilterOptimization.h"
diff --git a/arangod/IResearch/IResearchFilterOptimization.h b/arangod/IResearch/IResearchFilterOptimization.h
@@ -0,0 +1,33 @@
+////////////////////////////////////////////////////////////////////////////////
+/// DISCLAIMER
+///
+/// Copyright 2021 ArangoDB GmbH, Cologne, Germany
+///
+/// Licensed under the Apache License, Version 2.0 (the "License");
+/// you may not use this file except in compliance with the License.
+/// You may obtain a copy of the License at
+///
+///     http://www.apache.org/licenses/LICENSE-2.0
+///
+/// Unless required by applicable law or agreed to in writing, software
+/// distributed under the License is distributed on an "AS IS" BASIS,
+/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+/// See the License for the specific language governing permissions and
+/// limitations under the License.
+///
+/// Copyright holder is ArangoDB GmbH, Cologne, Germany
+///
+/// @author Andrei Lobov
+////////////////////////////////////////////////////////////////////////////////
+#pragma once
+
+namespace arangodb {
+namespace iresearch {
+
+enum class FilterOptimization : int {
+  MAX = -1,
+  None = 0
+};
+
+}
+} //namespace arangodb
diff --git a/tests/IResearch/IResearchQueryOptimization-test.cpp b/tests/IResearch/IResearchQueryOptimization-test.cpp
@@ -12039,7 +12039,7 @@ TEST_P(IResearchQueryOptimizationTest, mergeLevenshteinStartsWith) {
       vocbase(),
       "FOR d IN testView SEARCH LEVENSHTEIN_MATCH(d.name, 'bar', 2, false, 63, 'foo') "
       "AND STARTS_WITH(d.name, 'foo') "
-      "OPTIONS {\"allowFiltersMerge\": true } "
+      "OPTIONS {\"filterOptimization\": -1 } "
       "RETURN d",
       expected);
   }
@@ -12273,7 +12273,7 @@ TEST_P(IResearchQueryOptimizationTest, mergeLevenshteinStartsWith) {
     assertFilterOptimized(
       vocbase(),
       "FOR d IN testView SEARCH LEVENSHTEIN_MATCH(d.name, 'bar', 2, false, 63, 'foo') "
-      "AND STARTS_WITH(d.name, 'foo') OPTIONS {\"allowFiltersMerge\": true } SORT BM25(d) "
+      "AND STARTS_WITH(d.name, 'foo') OPTIONS {\"filterOptimization\": -1} SORT BM25(d) "
       "RETURN d",
       expected);
   }
@@ -12302,7 +12302,7 @@ TEST_P(IResearchQueryOptimizationTest, mergeLevenshteinStartsWith) {
       vocbase(),
       "FOR d IN testView SEARCH LEVENSHTEIN_MATCH(d.name, 'bar', 2, false, 63, 'foo') "
       "AND STARTS_WITH(d.name, 'foo') "
-      "OPTIONS {\"allowFiltersMerge\": false } "
+      "OPTIONS {\"filterOptimization\": 0 } "
       "RETURN d",
       expected);
   }
@@ -12342,7 +12342,7 @@ TEST_P(IResearchQueryOptimizationTest, mergeLevenshteinStartsWith) {
       vocbase(),
       "FOR d IN testView SEARCH LEVENSHTEIN_MATCH(d.name, 'bar', 2, false, 63, 'foo') "
       "AND STARTS_WITH(d.name, ['foo', 'boo'], 2) "
-      "OPTIONS {\"allowFiltersMerge\": false } "
+      "OPTIONS {\"filterOptimization\": 0 } "
       "RETURN d",
       expected);
   }

diff --git a/tests/IResearch/IResearchViewCountApproximate-test.cpp b/tests/IResearch/IResearchViewCountApproximate-test.cpp
@@ -509,7 +509,8 @@ TEST_F(IResearchViewCountApproximateTest, directSkipAllForMergeExecutorExact) {
       viewNode.outVariable(), viewNode.filterCondition(), {false, false},
       viewNode.getRegisterPlan()->varInfo, 0,
       arangodb::iresearch::IResearchViewNode::ViewValuesRegisters{},
-      arangodb::iresearch::CountApproximate::Exact, true);
+      arangodb::iresearch::CountApproximate::Exact,
+      arangodb::iresearch::FilterOptimization::MAX);
 
   std::vector<arangodb::aql::ExecutionBlock*> emptyExecutors;
   arangodb::aql::DependencyProxy<arangodb::aql::BlockPassthrough::Disable> dummyProxy(emptyExecutors, 0);
@@ -579,7 +580,8 @@ TEST_F(IResearchViewCountApproximateTest, directSkipAllForMergeExecutorExactEmpt
                                                 viewNode.getRegisterPlan()->varInfo,
                                                 0,
                                                 arangodb::iresearch::IResearchViewNode::ViewValuesRegisters{},
-                                                arangodb::iresearch::CountApproximate::Exact, true);
+                                                arangodb::iresearch::CountApproximate::Exact,
+                                                arangodb::iresearch::FilterOptimization::MAX);
 
   std::vector<arangodb::aql::ExecutionBlock*> emptyExecutors;
   arangodb::aql::DependencyProxy<arangodb::aql::BlockPassthrough::Disable> dummyProxy(emptyExecutors, 0);
@@ -650,7 +652,8 @@ TEST_F(IResearchViewCountApproximateTest, directSkipAllForMergeExecutorCost) {
                                                 viewNode.getRegisterPlan()->varInfo,
                                                 0,
                                                 arangodb::iresearch::IResearchViewNode::ViewValuesRegisters{},
-                                                arangodb::iresearch::CountApproximate::Cost, true);
+                                                arangodb::iresearch::CountApproximate::Cost,
+                                                arangodb::iresearch::FilterOptimization::MAX);
 
   std::vector<arangodb::aql::ExecutionBlock*> emptyExecutors;
   arangodb::aql::DependencyProxy<arangodb::aql::BlockPassthrough::Disable> dummyProxy(emptyExecutors, 0);