Address PR comments

src/engine/Bind.h

@@ -31,9 +31,9 @@ class Bind : public Operation {
   std::vector<QueryExecutionTree*> getChildren() override;
   size_t getCostEstimate() override;
   bool supportsLimit() const override;
-  std::unique_ptr<Operation> cloneImpl() const override;
 
  private:
+  std::unique_ptr<Operation> cloneImpl() const override;
   uint64_t getSizeEstimateBeforeLimit() override;
 
  public:

src/engine/CartesianProductJoin.h

@@ -61,6 +61,8 @@ class CartesianProductJoin : public Operation {
 
   VariableToColumnMap computeVariableToColumnMap() const override;
 
+  std::unique_ptr<Operation> cloneImpl() const override;
+
  public:
   float getMultiplicity([[maybe_unused]] size_t col) override;
 
@@ -69,8 +71,6 @@ class CartesianProductJoin : public Operation {
   // The Cartesian product join can efficiently evaluate a limited result.
   [[nodiscard]] bool supportsLimit() const override { return true; }
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
  protected:
   // Don't promise any sorting of the result.
   // TODO<joka921> Depending on the implementation we could propagate sorted

src/engine/CountAvailablePredicates.h

@@ -64,6 +64,8 @@ class CountAvailablePredicates : public Operation {
  private:
   uint64_t getSizeEstimateBeforeLimit() override;
 
+  std::unique_ptr<Operation> cloneImpl() const override;
+
  public:
   size_t getCostEstimate() override;
 
@@ -72,8 +74,6 @@ class CountAvailablePredicates : public Operation {
   const Variable& predicateVariable() const { return predicateVariable_; }
   const Variable& countVariable() const { return countVariable_; }
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
  private:
   /**
    * @brief Computes all relations that have one of input[inputCol]'s entities

src/engine/Describe.h

@@ -46,9 +46,8 @@ class Describe : public Operation {
   float getMultiplicity(size_t col) override;
   bool knownEmptyResult() override;
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
  private:
+  std::unique_ptr<Operation> cloneImpl() const override;
   [[nodiscard]] vector<ColumnIndex> resultSortedOn() const override;
   ProtoResult computeResult(bool requestLaziness) override;
   VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/Distinct.h

@@ -48,12 +48,11 @@ class Distinct : public Operation {
     return {subtree_.get()};
   }
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
  protected:
   [[nodiscard]] string getCacheKeyImpl() const override;
 
  private:
+  std::unique_ptr<Operation> cloneImpl() const override;
   ProtoResult computeResult(bool requestLaziness) override;
 
   VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/ExistsJoin.h

@@ -75,9 +75,9 @@ class ExistsJoin : public Operation {
     return {left_.get(), right_.get()};
   }
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;
 
   VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/Filter.h

@@ -53,9 +53,9 @@ class Filter : public Operation {
     return _subtree->getMultiplicity(col);
   }
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   VariableToColumnMap computeVariableToColumnMap() const override {
     return _subtree->getVariableColumns();
   }

src/engine/GroupBy.h

@@ -576,6 +576,7 @@ class GroupBy : public Operation {
   // GROUP BY. This is used by some of the optimizations above.
   bool isVariableBoundInSubtree(const Variable& variable) const;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
   // TODO<joka921> implement optimization when *additional* Variables are

src/engine/HasPredicateScan.h

@@ -108,9 +108,9 @@ class HasPredicateScan : public Operation {
   ProtoResult computeSubqueryS(IdTable* result,
                                const CompactVectorOfStrings<Id>& patterns);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;
 
   [[nodiscard]] VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/IndexScan.h

@@ -161,8 +161,6 @@ class IndexScan final : public Operation {
   // An index scan can directly and efficiently support LIMIT and OFFSET
   [[nodiscard]] bool supportsLimit() const override { return true; }
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
   Permutation::Enum permutation() const { return permutation_; }
 
   // Return the stored triple in the order that corresponds to the
@@ -178,6 +176,8 @@ class IndexScan final : public Operation {
       const CompressedRelationReader::LazyScanMetadata& metadata);
 
  private:
+  std::unique_ptr<Operation> cloneImpl() const override;
+
   ProtoResult computeResult(bool requestLaziness) override;
 
   vector<QueryExecutionTree*> getChildren() override { return {}; }

src/engine/Join.h

@@ -142,12 +142,12 @@ class Join : public Operation {
   static void hashJoin(const IdTable& dynA, ColumnIndex jc1,
                        const IdTable& dynB, ColumnIndex jc2, IdTable* dynRes);
 
-  std::unique_ptr<Operation> cloneImpl() const override;
-
  protected:
   virtual string getCacheKeyImpl() const override;
 
  private:
+  std::unique_ptr<Operation> cloneImpl() const override;
+
   ProtoResult computeResult(bool requestLaziness) override;
 
   VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/Minus.h

@@ -25,7 +25,7 @@ class Minus : public Operation {
 
   // Uninitialized Object for testing the computeMinus method
   struct OnlyForTestingTag {};
-  explicit Minus(OnlyForTestingTag){};
+  explicit Minus(OnlyForTestingTag) {};
 
  protected:
   string getCacheKeyImpl() const override;
@@ -62,9 +62,9 @@ class Minus : public Operation {
                     const vector<std::array<ColumnIndex, 2>>& matchedColumns,
                     IdTable* result) const;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   /**
    * @brief Compares the two rows under the assumption that the first
    * entries of the rows are equal.

src/engine/MultiColumnJoin.h

@@ -62,9 +62,9 @@ class MultiColumnJoin : public Operation {
       const std::vector<std::array<ColumnIndex, 2>>& joinColumns,
       IdTable* resultMightBeUnsorted);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;
 
   VariableToColumnMap computeVariableToColumnMap() const override;

src/engine/Operation.cpp

@@ -649,5 +649,7 @@ std::unique_ptr<Operation> Operation::clone() const {
     return true;
   };
   AD_CORRECTNESS_CHECK(areChildrenDifferent());
+  AD_CORRECTNESS_CHECK(variableToColumnMap_ == result->variableToColumnMap_);
+  AD_EXPENSIVE_CHECK(getCacheKey() == result->getCacheKey());
   return result;
 }

src/engine/Operation.h

@@ -316,9 +316,11 @@ class Operation {
 
   const auto& getLimit() const { return _limit; }
 
+ private:
   // Actual implementation of `clone()` without extra checks.
   virtual std::unique_ptr<Operation> cloneImpl() const = 0;
 
+ public:
   // Create a deep copy of this operation.
   std::unique_ptr<Operation> clone() const;
 

src/engine/OptionalJoin.h

@@ -66,9 +66,9 @@ class OptionalJoin : public Operation {
       IdTable* dynResult,
       Implementation implementation = Implementation::GeneralCase);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   void computeSizeEstimateAndMultiplicities();
 
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;

src/engine/OrderBy.h

@@ -77,9 +77,9 @@ class OrderBy : public Operation {
     return {subtree_.get()};
   }
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;
 
   VariableToColumnMap computeVariableToColumnMap() const override {

src/engine/PathSearch.cpp

@@ -476,14 +476,13 @@ void PathSearch::pathsToResultTable(IdTable& tableDyn, PathsLimited& paths,
 std::unique_ptr<Operation> PathSearch::cloneImpl() const {
   auto copy = std::make_unique<PathSearch>(*this);
   copy->subtree_ = subtree_->clone();
-  if (sourceTree_.has_value()) {
-    copy->sourceTree_ = sourceTree_.value()->clone();
-  }
-  if (targetTree_.has_value()) {
-    copy->targetTree_ = targetTree_.value()->clone();
-  }
-  if (sourceAndTargetTree_.has_value()) {
-    copy->sourceAndTargetTree_ = sourceAndTargetTree_.value()->clone();
-  }
+  auto cloneIfNonEmpty = [](auto& tree) {
+    if (tree.has_value()) {
+      tree = tree.value()->clone();
+    }
+  };
+  cloneIfNonEmpty(copy->sourceTree_);
+  cloneIfNonEmpty(copy->targetTree_);
+  cloneIfNonEmpty(copy->sourceAndTargetTree_);
   return copy;
 }

src/engine/PathSearch.h

@@ -252,9 +252,9 @@ class PathSearch : public Operation {
   Result computeResult([[maybe_unused]] bool requestLaziness) override;
   VariableToColumnMap computeVariableToColumnMap() const override;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   std::pair<std::span<const Id>, std::span<const Id>> handleSearchSides() const;
 
   /**

src/engine/Service.h

@@ -102,9 +102,9 @@ class Service : public Operation {
                                       std::shared_ptr<Operation> right,
                                       bool rightOnly, bool requestLaziness);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   // The string returned by this function is used as cache key.
   std::string getCacheKeyImpl() const override;
 

src/engine/Sort.h

@@ -66,9 +66,9 @@ class Sort : public Operation {
     return {subtree_.get()};
   }
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   virtual ProtoResult computeResult(
       [[maybe_unused]] bool requestLaziness) override;
 

src/engine/SpatialJoin.h

@@ -174,9 +174,9 @@ class SpatialJoin : public Operation {
     return childRight_;
   }
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   // helper function to generate a variable to column map from `childRight_`
   // that only contains the columns selected by `config_.payloadVariables_`
   // and (automatically added) the `config_.right_` variable.

src/engine/TextIndexScanForEntity.h

@@ -92,9 +92,9 @@ class TextIndexScanForEntity : public Operation {
 
   VariableToColumnMap computeVariableToColumnMap() const override;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   const VocabIndex& getVocabIndexOfFixedEntity() const {
     AD_CONTRACT_CHECK(hasFixedEntity());
     return std::get<FixedEntity>(varOrFixed_.entity_).second;

src/engine/TextIndexScanForWord.h

@@ -44,9 +44,9 @@ class TextIndexScanForWord : public Operation {
 
   VariableToColumnMap computeVariableToColumnMap() const override;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   // Returns a Result containing an IdTable with the columns being
   // the text variable and the completed word (if it was prefixed)
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;

src/engine/TextLimit.h

@@ -61,9 +61,9 @@ class TextLimit : public Operation {
 
   VariableToColumnMap computeVariableToColumnMap() const override;
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   ProtoResult computeResult([[maybe_unused]] bool requestLaziness) override;
 
   vector<QueryExecutionTree*> getChildren() override { return {child_.get()}; }

src/engine/TransitivePathBase.h

@@ -55,6 +55,14 @@ struct TransitivePathSide {
     // TODO<C++23> use ql::ranges::starts_with
     return (!sortedOn.empty() && sortedOn[0] == col);
   }
+
+  TransitivePathSide clone() const {
+    TransitivePathSide copy = *this;
+    if (copy.treeAndCol_.has_value()) {
+      copy.treeAndCol_.value().first = copy.treeAndCol_.value().first->clone();
+    }
+    return copy;
+  }
 };
 
 // We deliberately use the `std::` variants of a hash set and hash map because

src/engine/TransitivePathBinSearch.cpp

@@ -36,13 +36,7 @@ BinSearchMap TransitivePathBinSearch::setupEdgesMap(
 std::unique_ptr<Operation> TransitivePathBinSearch::cloneImpl() const {
   auto copy = std::make_unique<TransitivePathBinSearch>(*this);
   copy->subtree_ = subtree_->clone();
-  if (copy->lhs_.treeAndCol_.has_value()) {
-    copy->lhs_.treeAndCol_.value().first =
-        lhs_.treeAndCol_.value().first->clone();
-  }
-  if (copy->rhs_.treeAndCol_.has_value()) {
-    copy->rhs_.treeAndCol_.value().first =
-        rhs_.treeAndCol_.value().first->clone();
-  }
+  copy->lhs_ = lhs_.clone();
+  copy->rhs_ = rhs_.clone();
   return copy;
 }

src/engine/TransitivePathBinSearch.h

@@ -69,9 +69,9 @@ class TransitivePathBinSearch : public TransitivePathImpl<BinSearchMap> {
                           TransitivePathSide rightSide, size_t minDist,
                           size_t maxDist);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   // initialize the map from the subresult
   BinSearchMap setupEdgesMap(
       const IdTable& dynSub, const TransitivePathSide& startSide,

src/engine/TransitivePathHashMap.cpp

@@ -49,13 +49,7 @@ HashMapWrapper TransitivePathHashMap::setupEdgesMap(
 std::unique_ptr<Operation> TransitivePathHashMap::cloneImpl() const {
   auto copy = std::make_unique<TransitivePathHashMap>(*this);
   copy->subtree_ = subtree_->clone();
-  if (copy->lhs_.treeAndCol_.has_value()) {
-    copy->lhs_.treeAndCol_.value().first =
-        lhs_.treeAndCol_.value().first->clone();
-  }
-  if (copy->rhs_.treeAndCol_.has_value()) {
-    copy->rhs_.treeAndCol_.value().first =
-        rhs_.treeAndCol_.value().first->clone();
-  }
+  copy->lhs_ = lhs_.clone();
+  copy->rhs_ = rhs_.clone();
   return copy;
 }

src/engine/TransitivePathHashMap.h

@@ -23,7 +23,7 @@ struct HashMapWrapper {
   Set emptySet_;
 
   HashMapWrapper(Map map, ad_utility::AllocatorWithLimit<Id> allocator)
-      : map_(std::move(map)), emptySet_(allocator){};
+      : map_(std::move(map)), emptySet_(allocator) {};
 
   /**
    * @brief Return the successors for the given Id. The successors are all ids,
@@ -58,9 +58,9 @@ class TransitivePathHashMap : public TransitivePathImpl<HashMapWrapper> {
                         TransitivePathSide rightSide, size_t minDist,
                         size_t maxDist);
 
+ private:
   std::unique_ptr<Operation> cloneImpl() const override;
 
- private:
   /**
    * @brief Prepare a Map and a nodes vector for the transitive hull
    * computation.