Removed unused template parameter for IdExecutor

arangodb · goedderz · Nov 15, 2019 · Nov 15, 2019 · Nov 18, 2019 · Nov 18, 2019
commit a6a1a022b9c65f5328af40bc9e8158137f32d6fc
diff --git a/arangod/Aql/DistributeConsumerNode.cpp b/arangod/Aql/DistributeConsumerNode.cpp
@@ -65,7 +65,7 @@ std::unique_ptr<ExecutionBlock> DistributeConsumerNode::createBlock(
              getRegisterPlan()->nrRegs[getDepth()]);
   IdExecutorInfos infos(getRegisterPlan()->nrRegs[getDepth()], calcRegsToKeep(),
                         getRegsToClear(), _distributeId, _isResponsibleForInitializeCursor);
-  return std::make_unique<ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>>>(
+  return std::make_unique<ExecutionBlockImpl<IdExecutor<SingleRowFetcher<BlockPassthrough::Enable>>>>(
       &engine, this, std::move(infos));
 }
 

diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp
@@ -519,7 +519,7 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<Executor>::shutdown(int err
 namespace arangodb::aql {
 // TODO -- remove this specialization when cpp 17 becomes available
 template <>
-std::pair<ExecutionState, Result> ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, ConstFetcher>>::initializeCursor(
+std::pair<ExecutionState, Result> ExecutionBlockImpl<IdExecutor<ConstFetcher>>::initializeCursor(
     InputAqlItemRow const& input) {
   // reinitialize the DependencyProxy
   _dependencyProxy.reset();
@@ -630,7 +630,7 @@ std::pair<ExecutionState, Result> ExecutionBlockImpl<SubqueryExecutor<false>>::s
 
 template <>
 std::pair<ExecutionState, Result> ExecutionBlockImpl<
-    IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>>::shutdown(int errorCode) {
+    IdExecutor<SingleRowFetcher<BlockPassthrough::Enable>>>::shutdown(int errorCode) {
   if (this->infos().isResponsibleForInitializeCursor()) {
     return ExecutionBlock::shutdown(errorCode);
   }
@@ -870,9 +870,8 @@ template class ::arangodb::aql::ExecutionBlockImpl<IResearchViewExecutor<false,
 template class ::arangodb::aql::ExecutionBlockImpl<IResearchViewExecutor<true, false>>;
 template class ::arangodb::aql::ExecutionBlockImpl<IResearchViewMergeExecutor<false, false>>;
 template class ::arangodb::aql::ExecutionBlockImpl<IResearchViewMergeExecutor<true, false>>;
-template class ::arangodb::aql::ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, ConstFetcher>>;
-template class ::arangodb::aql::ExecutionBlockImpl<
-    IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>>;
+template class ::arangodb::aql::ExecutionBlockImpl<IdExecutor<ConstFetcher>>;
+template class ::arangodb::aql::ExecutionBlockImpl<IdExecutor<SingleRowFetcher<BlockPassthrough::Enable>>>;
 template class ::arangodb::aql::ExecutionBlockImpl<IndexExecutor>;
 template class ::arangodb::aql::ExecutionBlockImpl<LimitExecutor>;
 

diff --git a/arangod/Aql/ExecutionEngine.cpp b/arangod/Aql/ExecutionEngine.cpp
@@ -709,7 +709,7 @@ ExecutionEngine* ExecutionEngine::instantiateFromPlan(QueryRegistry& queryRegist
 
     bool const returnInheritedResults = !arangodb::ServerState::isDBServer(role);
     if (returnInheritedResults) {
-      auto returnNode = dynamic_cast<ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>*>(root);
+      auto returnNode = dynamic_cast<ExecutionBlockImpl<IdExecutor<void>>*>(root);
       TRI_ASSERT(returnNode != nullptr);
       engine->resultRegister(returnNode->getOutputRegisterId());
     } else {

diff --git a/arangod/Aql/ExecutionNode.cpp b/arangod/Aql/ExecutionNode.cpp
@@ -1283,8 +1283,8 @@ std::unique_ptr<ExecutionBlock> SingletonNode::createBlock(
 
   IdExecutorInfos infos(nrRegs, std::move(toKeep), getRegsToClear());
 
-  return std::make_unique<ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, ConstFetcher>>>(
-          &engine, this, std::move(infos));
+  return std::make_unique<ExecutionBlockImpl<IdExecutor<ConstFetcher>>>(&engine, this,
+                                                                        std::move(infos));
 }
 
 /// @brief toVelocyPack, for SingletonNode
@@ -2170,8 +2170,8 @@ std::unique_ptr<ExecutionBlock> ReturnNode::createBlock(
       returnInheritedResults ? getRegisterPlan()->nrRegs[getDepth()] : 1;
 
   if (returnInheritedResults) {
-    return std::make_unique<ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>>(
-        &engine, this, inputRegister, _count);
+    return std::make_unique<ExecutionBlockImpl<IdExecutor<void>>>(&engine, this,
+                                                                  inputRegister, _count);
   } else {
     TRI_ASSERT(!returnInheritedResults);
     ReturnExecutorInfos infos(inputRegister, numberInputRegisters,

diff --git a/arangod/Aql/IdExecutor.cpp b/arangod/Aql/IdExecutor.cpp
@@ -37,7 +37,7 @@
 using namespace arangodb;
 using namespace arangodb::aql;
 
-ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::ExecutionBlockImpl(ExecutionEngine* engine,
+ExecutionBlockImpl<IdExecutor<void>>::ExecutionBlockImpl(ExecutionEngine* engine,
                                                          ExecutionNode const* node,
                                                          RegisterId outputRegister, bool doCount)
     : ExecutionBlock(engine, node),
@@ -50,7 +50,7 @@ ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::ExecutionBlockIm
   }
 }
 
-std::pair<ExecutionState, size_t> ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::skipSome(size_t atMost) {
+std::pair<ExecutionState, size_t> ExecutionBlockImpl<IdExecutor<void>>::skipSome(size_t atMost) {
   traceSkipSomeBegin(atMost);
   if (isDone()) {
     return traceSkipSomeEnd(ExecutionState::DONE, 0);
@@ -67,7 +67,7 @@ std::pair<ExecutionState, size_t> ExecutionBlockImpl<IdExecutor<BlockPassthrough
   return traceSkipSomeEnd(state, skipped);
 }
 
-std::pair<ExecutionState, SharedAqlItemBlockPtr> ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::getSome(size_t atMost) {
+std::pair<ExecutionState, SharedAqlItemBlockPtr> ExecutionBlockImpl<IdExecutor<void>>::getSome(size_t atMost) {
   traceGetSomeBegin(atMost);
   if (isDone()) {
     return traceGetSomeEnd(ExecutionState::DONE, nullptr);
@@ -86,32 +86,32 @@ std::pair<ExecutionState, SharedAqlItemBlockPtr> ExecutionBlockImpl<IdExecutor<B
   return traceGetSomeEnd(state, block);
 }
 
-bool aql::ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::isDone() const noexcept {
+bool aql::ExecutionBlockImpl<IdExecutor<void>>::isDone() const noexcept {
   // I'd like to assert this in the constructor, but the dependencies are
   // added after construction.
   TRI_ASSERT(!_dependencies.empty());
   return _currentDependency >= _dependencies.size();
 }
 
-RegisterId ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::getOutputRegisterId() const noexcept {
+RegisterId ExecutionBlockImpl<IdExecutor<void>>::getOutputRegisterId() const noexcept {
   return _outputRegister;
 }
 
-ExecutionBlock& ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::currentDependency() const {
+ExecutionBlock& ExecutionBlockImpl<IdExecutor<void>>::currentDependency() const {
   TRI_ASSERT(_dependencies[_currentDependency] != nullptr);
   return *_dependencies[_currentDependency];
 }
 
-void ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::nextDependency() noexcept {
+void ExecutionBlockImpl<IdExecutor<void>>::nextDependency() noexcept {
   ++_currentDependency;
 }
 
-bool ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::doCount() const noexcept {
+bool ExecutionBlockImpl<IdExecutor<void>>::doCount() const noexcept {
   return _doCount;
 }
 
-void ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>>::countStats(SharedAqlItemBlockPtr& block) {
+void ExecutionBlockImpl<IdExecutor<void>>::countStats(SharedAqlItemBlockPtr& block) {
   if (doCount() && block != nullptr) {
     CountStats stats;
     stats.setCounted(block->size());
@@ -137,19 +137,19 @@ bool IdExecutorInfos::isResponsibleForInitializeCursor() const {
   return _isResponsibleForInitializeCursor;
 }
 
-template <BlockPassthrough usePassThrough, class UsedFetcher>
-IdExecutor<usePassThrough, UsedFetcher>::IdExecutor(Fetcher& fetcher, IdExecutorInfos& infos)
+template <class UsedFetcher>
+IdExecutor<UsedFetcher>::IdExecutor(Fetcher& fetcher, IdExecutorInfos& infos)
     : _fetcher(fetcher) {
   if (!infos.distributeId().empty()) {
     _fetcher.setDistributeId(infos.distributeId());
   }
 }
 
-template <BlockPassthrough usePassThrough, class UsedFetcher>
-IdExecutor<usePassThrough, UsedFetcher>::~IdExecutor() = default;
+template <class UsedFetcher>
+IdExecutor<UsedFetcher>::~IdExecutor() = default;
 
-template <BlockPassthrough usePassThrough, class UsedFetcher>
-std::pair<ExecutionState, NoStats> IdExecutor<usePassThrough, UsedFetcher>::produceRows(OutputAqlItemRow& output) {
+    template <class UsedFetcher>
+std::pair<ExecutionState, NoStats> IdExecutor<UsedFetcher>::produceRows(OutputAqlItemRow& output) {
   ExecutionState state = ExecutionState::HASMORE;
   NoStats stats;
   InputAqlItemRow inputRow = InputAqlItemRow{CreateInvalidInputRowHint{}};
@@ -184,34 +184,13 @@ std::pair<ExecutionState, NoStats> IdExecutor<usePassThrough, UsedFetcher>::prod
   return {state, stats};
 }
 
-template <BlockPassthrough usePassThrough, class UsedFetcher>
-template <BlockPassthrough allowPass, typename>
-std::tuple<ExecutionState, NoStats, size_t> IdExecutor<usePassThrough, UsedFetcher>::skipRows(size_t atMost) {
-  ExecutionState state;
-  size_t skipped;
-  std::tie(state, skipped) = _fetcher.skipRows(atMost);
-  return {state, NoStats{}, skipped};
-}
-
-template <BlockPassthrough usePassThrough, class UsedFetcher>
-template <BlockPassthrough allowPass, typename>
-std::tuple<ExecutionState, typename IdExecutor<usePassThrough, UsedFetcher>::Stats, SharedAqlItemBlockPtr>
-IdExecutor<usePassThrough, UsedFetcher>::fetchBlockForPassthrough(size_t atMost) {
+template <class UsedFetcher>
+std::tuple<ExecutionState, typename IdExecutor<UsedFetcher>::Stats, SharedAqlItemBlockPtr>
+IdExecutor<UsedFetcher>::fetchBlockForPassthrough(size_t atMost) {
   auto rv = _fetcher.fetchBlockForPassthrough(atMost);
   return {rv.first, {}, std::move(rv.second)};
 }
 
-template class ::arangodb::aql::IdExecutor<BlockPassthrough::Enable, ConstFetcher>;
+template class ::arangodb::aql::IdExecutor<ConstFetcher>;
 // ID can always pass through
-template class ::arangodb::aql::IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>;
-// Local gather does NOT want to passThrough
-template class ::arangodb::aql::IdExecutor<BlockPassthrough::Disable, SingleRowFetcher<BlockPassthrough::Disable>>;
-
-template std::tuple<ExecutionState, typename IdExecutor<BlockPassthrough::Enable, ConstFetcher>::Stats, SharedAqlItemBlockPtr>
-IdExecutor<BlockPassthrough::Enable, ConstFetcher>::fetchBlockForPassthrough<BlockPassthrough::Enable, void>(size_t atMost);
-
-template std::tuple<ExecutionState, typename IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>::Stats, SharedAqlItemBlockPtr>
-IdExecutor<BlockPassthrough::Enable, SingleRowFetcher<BlockPassthrough::Enable>>::fetchBlockForPassthrough<BlockPassthrough::Enable, void>(size_t atMost);
-
-template std::tuple<ExecutionState, NoStats, size_t>
-IdExecutor<BlockPassthrough::Disable, SingleRowFetcher<BlockPassthrough::Disable>>::skipRows<BlockPassthrough::Disable, void>(size_t atMost);
+template class ::arangodb::aql::IdExecutor<SingleRowFetcher<BlockPassthrough::Enable>>;
diff --git a/arangod/Aql/IdExecutor.h b/arangod/Aql/IdExecutor.h
@@ -31,7 +31,20 @@
 #include <tuple>
 #include <utility>
 
-// TODO Clean up unused variants of the IdExecutor - some of them aren't in use anymore.
+// There are currently three variants of IdExecutor in use:
+//
+// - IdExecutor<void>
+//     This is a variant of the ReturnBlock. It can optionally count and holds
+//     an output register id.
+// - IdExecutor<ConstFetcher>
+//     This is the SingletonBlock.
+// - IdExecutor<SingleRowFetcher<BlockPassthrough::Enable>>
+//     This is the DistributeConsumerBlock. It holds a distributeId and honors
+//     isResponsibleForInitializeCursor.
+//
+// The last variant using the SingleRowFetcher could be replaced by the (faster)
+// void variant. It only has to learn distributeId and
+// isResponsibleForInitializeCursor for that.
 
 namespace arangodb {
 namespace transaction {
@@ -59,8 +72,7 @@ class IdExecutorInfos : public ExecutorInfos {
 
   [[nodiscard]] std::string const& distributeId();
 
-  // TODO This is probably needed only for UnsortedGather now, so can be removed here.
-  //[[nodiscard]] bool isResponsibleForInitializeCursor() const;
+  [[nodiscard]] bool isResponsibleForInitializeCursor() const;
 
  private:
   std::string const _distributeId;
@@ -69,16 +81,16 @@ class IdExecutorInfos : public ExecutorInfos {
 };
 
 // forward declaration
-template <BlockPassthrough usePassThrough, class T>
+template <class Fetcher>
 class IdExecutor;
 
 // (empty) implementation of IdExecutor<void>
 template <>
-class IdExecutor<BlockPassthrough::Enable, void> {};
+class IdExecutor<void> {};
 
 // implementation of ExecutionBlockImpl<IdExecutor<void>>
 template <>
-class ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>> : public ExecutionBlock {
+class ExecutionBlockImpl<IdExecutor<void>> : public ExecutionBlock {
  public:
   ExecutionBlockImpl(ExecutionEngine* engine, ExecutionNode const* node,
                      RegisterId outputRegister, bool doCount);
@@ -108,13 +120,13 @@ class ExecutionBlockImpl<IdExecutor<BlockPassthrough::Enable, void>> : public Ex
   bool const _doCount;
 };
 
-template <BlockPassthrough usePassThrough, class UsedFetcher>
+template <class UsedFetcher>
 // cppcheck-suppress noConstructor
 class IdExecutor {
  public:
   struct Properties {
     static constexpr bool preservesOrder = true;
-    static constexpr BlockPassthrough allowsBlockPassthrough = usePassThrough;
+    static constexpr BlockPassthrough allowsBlockPassthrough = BlockPassthrough::Enable;
     static constexpr bool inputSizeRestrictsOutputSize = false;
   };
   // Only Supports SingleRowFetcher and ConstFetcher
@@ -133,12 +145,8 @@ class IdExecutor {
    */
   std::pair<ExecutionState, Stats> produceRows(OutputAqlItemRow& output);
 
-  template <BlockPassthrough allowPass = usePassThrough, typename = std::enable_if_t<allowPass == BlockPassthrough::Enable>>
   std::tuple<ExecutionState, Stats, SharedAqlItemBlockPtr> fetchBlockForPassthrough(size_t atMost);
 
-  template <BlockPassthrough allowPass = usePassThrough, typename = std::enable_if_t<allowPass == BlockPassthrough::Disable>>
-  std::tuple<ExecutionState, NoStats, size_t> skipRows(size_t atMost);
-
  private:
   Fetcher& _fetcher;
 };

diff --git a/tests/Aql/IdExecutorTest.cpp b/tests/Aql/IdExecutorTest.cpp
@@ -32,15 +32,12 @@
 #include "Aql/ResourceUsage.h"
 #include "Aql/Stats.h"
 
-#include <velocypack/Builder.h>
 #include <velocypack/velocypack-aliases.h>
 
 using namespace arangodb;
 using namespace arangodb::aql;
 
-namespace arangodb {
-namespace tests {
-namespace aql {
+namespace arangodb::tests::aql {
 
 class IdExecutorTest : public ::testing::Test {
  protected:
@@ -66,7 +63,7 @@ class IdExecutorTest : public ::testing::Test {
 
 TEST_F(IdExecutorTest, there_are_no_rows_upstream) {
   ConstFetcherHelper fetcher(itemBlockManager, nullptr);
-  IdExecutor<::arangodb::aql::BlockPassthrough::Enable, ConstFetcher> testee(fetcher, infos);
+  IdExecutor<ConstFetcher> testee(fetcher, infos);
   NoStats stats{};
 
   std::tie(state, stats) = testee.produceRows(row);
@@ -77,7 +74,7 @@ TEST_F(IdExecutorTest, there_are_no_rows_upstream) {
 TEST_F(IdExecutorTest, there_are_rows_in_the_upstream) {
   auto input = VPackParser::fromJson("[ [true], [false], [true] ]");
   ConstFetcherHelper fetcher(itemBlockManager, input->buffer());
-  IdExecutor<::arangodb::aql::BlockPassthrough::Enable, ConstFetcher> testee(fetcher, infos);
+  IdExecutor<ConstFetcher> testee(fetcher, infos);
   NoStats stats{};
 
   // This block consumes all rows at once.
@@ -98,6 +95,4 @@ TEST_F(IdExecutorTest, there_are_rows_in_the_upstream) {
   }
 }
 
-}  // namespace aql
-}  // namespace tests
 }  // namespace arangodb