Implement Transitive Paths using binary search instead of a hash map

src/engine/TransitivePathBinSearch.cpp

@@ -24,53 +24,6 @@ TransitivePathBinSearch::TransitivePathBinSearch(
       subtree_, {startSide.subCol_, targetSide.subCol_});
 }
 
-// _____________________________________________________________________________
-Map TransitivePathBinSearch::transitiveHull(const BinSearchMap& edges,
-                                            const std::vector<Id>& startNodes,
-                                            std::optional<Id> target) const {
-  // For every node do a dfs on the graph
-  Map hull{allocator()};
-
-  std::vector<std::pair<Id, size_t>> stack;
-  ad_utility::HashSetWithMemoryLimit<Id> marks{
-      getExecutionContext()->getAllocator()};
-  for (auto startNode : startNodes) {
-    if (hull.contains(startNode)) {
-      // We have already computed the hull for this node
-      continue;
-    }
-
-    marks.clear();
-    stack.clear();
-    stack.push_back({startNode, 0});
-
-    if (minDist_ == 0 && (!target.has_value() || startNode == target.value())) {
-      insertIntoMap(hull, startNode, startNode);
-    }
-
-    while (stack.size() > 0) {
-      checkCancellation();
-      auto [node, steps] = stack.back();
-      stack.pop_back();
-
-      if (steps <= maxDist_ && marks.count(node) == 0) {
-        if (steps >= minDist_) {
-          marks.insert(node);
-          if (!target.has_value() || node == target.value()) {
-            insertIntoMap(hull, startNode, node);
-          }
-        }
-
-        auto successors = edges.successors(node);
-        for (auto successor : successors) {
-          stack.push_back({successor, steps + 1});
-        }
-      }
-    }
-  }
-  return hull;
-}
-
 // _____________________________________________________________________________
 BinSearchMap TransitivePathBinSearch::setupEdgesMap(
     const IdTable& dynSub, const TransitivePathSide& startSide,

src/engine/TransitivePathBinSearch.h

@@ -55,6 +55,12 @@ struct BinSearchMap {
   }
 };
 
+/**
+ * @class TransitivePathBinSearch
+ * @brief This class implements the transitive path operation. The
+ * implementation represents the graph as adjacency lists and uses binary search
+ * to find successors of given nodes.
+ */
 class TransitivePathBinSearch : public TransitivePathImpl<BinSearchMap> {
  public:
   TransitivePathBinSearch(QueryExecutionContext* qec,
@@ -64,23 +70,6 @@ class TransitivePathBinSearch : public TransitivePathImpl<BinSearchMap> {
                           size_t maxDist);
 
  private:
-  /**
-   * @brief Compute the transitive hull starting at the given nodes,
-   * using the given Map.
-   *
-   * @param edges Adjacency lists, mapping Ids (nodes) to their connected
-   * Ids.
-   * @param nodes A list of Ids. These Ids are used as starting points for the
-   * transitive hull. Thus, this parameter guides the performance of this
-   * algorithm.
-   * @param target Optional target Id. If supplied, only paths which end
-   * in this Id are added to the hull.
-   * @return Map Maps each Id to its connected Ids in the transitive hull
-   */
-  Map transitiveHull(const BinSearchMap& edges,
-                     const std::vector<Id>& startNodes,
-                     std::optional<Id> target) const override;
-
   // initialize the map from the subresult
   BinSearchMap setupEdgesMap(
       const IdTable& dynSub, const TransitivePathSide& startSide,

src/engine/TransitivePathHashMap.cpp

@@ -16,89 +16,12 @@ TransitivePathHashMap::TransitivePathHashMap(
     QueryExecutionContext* qec, std::shared_ptr<QueryExecutionTree> child,
     TransitivePathSide leftSide, TransitivePathSide rightSide, size_t minDist,
     size_t maxDist)
-    : TransitivePathImpl<Map>(qec, std::move(child), std::move(leftSide),
-                              std::move(rightSide), minDist, maxDist) {}
+    : TransitivePathImpl<HashMapWrapper>(
+          qec, std::move(child), std::move(leftSide), std::move(rightSide),
+          minDist, maxDist) {}
 
 // _____________________________________________________________________________
-Map TransitivePathHashMap::transitiveHull(const Map& edges,
-                                          const std::vector<Id>& startNodes,
-                                          std::optional<Id> target) const {
-  using MapIt = Map::const_iterator;
-  // For every node do a dfs on the graph
-  Map hull{allocator()};
-
-  // Stores nodes we already have a path to. This avoids cycles.
-  ad_utility::HashSetWithMemoryLimit<Id> marks{
-      getExecutionContext()->getAllocator()};
-
-  // The stack used to store the dfs' progress
-  std::vector<Set::const_iterator> positions;
-
-  // Used to store all edges leading away from a node for every level.
-  // Reduces access to the hashmap, and is safe as the map will not
-  // be modified after this point.
-  std::vector<const Set*> edgeCache;
-
-  for (Id currentStartNode : startNodes) {
-    if (hull.contains(currentStartNode)) {
-      // We have already computed the hull for this node
-      continue;
-    }
-
-    // Reset for this iteration
-    marks.clear();
-
-    MapIt rootEdges = edges.find(currentStartNode);
-    if (rootEdges != edges.end()) {
-      positions.push_back(rootEdges->second.begin());
-      edgeCache.push_back(&rootEdges->second);
-    }
-    if (minDist_ == 0 &&
-        (!target.has_value() || currentStartNode == target.value())) {
-      insertIntoMap(hull, currentStartNode, currentStartNode);
-    }
-
-    // While we have not found the entire transitive hull and have not reached
-    // the max step limit
-    while (!positions.empty()) {
-      checkCancellation();
-      size_t stackIndex = positions.size() - 1;
-      // Process the next child of the node at the top of the stack
-      Set::const_iterator& pos = positions[stackIndex];
-      const Set* nodeEdges = edgeCache.back();
-
-      if (pos == nodeEdges->end()) {
-        // We finished processing this node
-        positions.pop_back();
-        edgeCache.pop_back();
-        continue;
-      }
-
-      Id child = *pos;
-      ++pos;
-      size_t childDepth = positions.size();
-      if (childDepth <= maxDist_ && marks.count(child) == 0) {
-        // process the child
-        if (childDepth >= minDist_) {
-          marks.insert(child);
-          if (!target.has_value() || child == target.value()) {
-            insertIntoMap(hull, currentStartNode, child);
-          }
-        }
-        // Add the child to the stack
-        MapIt it = edges.find(child);
-        if (it != edges.end()) {
-          positions.push_back(it->second.begin());
-          edgeCache.push_back(&it->second);
-        }
-      }
-    }
-  }
-  return hull;
-}
-
-// _____________________________________________________________________________
-Map TransitivePathHashMap::setupEdgesMap(
+HashMapWrapper TransitivePathHashMap::setupEdgesMap(
     const IdTable& dynSub, const TransitivePathSide& startSide,
     const TransitivePathSide& targetSide) const {
   return CALL_FIXED_SIZE((std::array{dynSub.numColumns()}),
@@ -108,7 +31,7 @@ Map TransitivePathHashMap::setupEdgesMap(
 
 // _____________________________________________________________________________
 template <size_t SUB_WIDTH>
-Map TransitivePathHashMap::setupEdgesMap(
+HashMapWrapper TransitivePathHashMap::setupEdgesMap(
     const IdTable& dynSub, const TransitivePathSide& startSide,
     const TransitivePathSide& targetSide) const {
   const IdTableView<SUB_WIDTH> sub = dynSub.asStaticView<SUB_WIDTH>();
@@ -120,5 +43,6 @@ Map TransitivePathHashMap::setupEdgesMap(
     checkCancellation();
     insertIntoMap(edges, startCol[i], targetCol[i]);
   }
-  return edges;
+  auto wrapper = HashMapWrapper(edges);
+  return wrapper;
 }

src/engine/TransitivePathHashMap.h

@@ -12,7 +12,28 @@
 #include "engine/TransitivePathImpl.h"
 #include "engine/idTable/IdTable.h"
 
-class TransitivePathHashMap : public TransitivePathImpl<Map> {
+struct HashMapWrapper {
+  Map map_;
+
+  auto successors(const Id node) const {
+    auto iterator = map_.find(node);
+    if (iterator == map_.end()) {
+      return std::vector<Id>();
+    }
+    std::vector<Id> result(iterator->second.begin(), iterator->second.end());
+    return result;
+  }
+};
+
+/**
+ * @class TransitivePathHashMap
+ * @brief This class implements the transitive path operation. The
+ * implementation uses a hash map to represent the graph and find successors
+ * of given nodes.
+ *
+ *
+ */
+class TransitivePathHashMap : public TransitivePathImpl<HashMapWrapper> {
  public:
   TransitivePathHashMap(QueryExecutionContext* qec,
                         std::shared_ptr<QueryExecutionTree> child,
@@ -21,22 +42,6 @@ class TransitivePathHashMap : public TransitivePathImpl<Map> {
                         size_t maxDist);
 
  private:
-  /**
-   * @brief Compute the transitive hull starting at the given nodes,
-   * using the given Map.
-   *
-   * @param edges Adjacency lists, mapping Ids (nodes) to their connected
-   * Ids.
-   * @param nodes A list of Ids. These Ids are used as starting points for the
-   * transitive hull. Thus, this parameter guides the performance of this
-   * algorithm.
-   * @param target Optional target Id. If supplied, only paths which end
-   * in this Id are added to the hull.
-   * @return Map Maps each Id to its connected Ids in the transitive hull
-   */
-  Map transitiveHull(const Map& edges, const std::vector<Id>& startNodes,
-                     std::optional<Id> target) const override;
-
   /**
    * @brief Prepare a Map and a nodes vector for the transitive hull
    * computation.
@@ -73,10 +78,12 @@ class TransitivePathHashMap : public TransitivePathImpl<Map> {
       const TransitivePathSide& targetSide) const;
 
   // initialize the map from the subresult
-  Map setupEdgesMap(const IdTable& dynSub, const TransitivePathSide& startSide,
-                    const TransitivePathSide& targetSide) const override;
+  HashMapWrapper setupEdgesMap(
+      const IdTable& dynSub, const TransitivePathSide& startSide,
+      const TransitivePathSide& targetSide) const override;
 
   template <size_t SUB_WIDTH>
-  Map setupEdgesMap(const IdTable& dynSub, const TransitivePathSide& startSide,
-                    const TransitivePathSide& targetSide) const;
+  HashMapWrapper setupEdgesMap(const IdTable& dynSub,
+                               const TransitivePathSide& startSide,
+                               const TransitivePathSide& targetSide) const;
 };

src/engine/TransitivePathImpl.h

@@ -145,10 +145,10 @@
   ResultTable computeResult() override {
     if (minDist_ == 0 && !isBoundOrId() && lhs_.isVariable() &&
         rhs_.isVariable()) {
       AD_THROW(
           "This query might have to evalute the empty path, which is currently "
           "not supported");
     }
     auto [startSide, targetSide] = decideDirection();
     shared_ptr<const ResultTable> subRes = subtree_->getResult();
 
@@ -183,6 +183,65 @@
             subRes->getSharedLocalVocab()};
   };
 
+  /**
+   * @brief Compute the transitive hull starting at the given nodes,
+   * using the given Map.
+   *
+   * @param edges Adjacency lists, mapping Ids (nodes) to their connected
+   * Ids.
+   * @param nodes A list of Ids. These Ids are used as starting points for the
+   * transitive hull. Thus, this parameter guides the performance of this
+   * algorithm.
+   * @param target Optional target Id. If supplied, only paths which end
+   * in this Id are added to the hull.
+   * @return Map Maps each Id to its connected Ids in the transitive hull
+   */
+  Map transitiveHull(const T& edges, const std::vector<Id>& startNodes,
+                     std::optional<Id> target) const {
+    // For every node do a dfs on the graph
+    Map hull{allocator()};
+
+    std::vector<std::pair<Id, size_t>> stack;
+    ad_utility::HashSetWithMemoryLimit<Id> marks{
+        getExecutionContext()->getAllocator()};
+    for (auto startNode : startNodes) {
+      if (hull.contains(startNode)) {
+        // We have already computed the hull for this node
+        continue;
+      }
+
+      marks.clear();
+      stack.clear();
+      stack.push_back({startNode, 0});
+
+      if (minDist_ == 0 &&
+          (!target.has_value() || startNode == target.value())) {
+        insertIntoMap(hull, startNode, startNode);
+      }
+
+      while (stack.size() > 0) {
+        checkCancellation();
+        auto [node, steps] = stack.back();
+        stack.pop_back();
+
+        if (steps <= maxDist_ && marks.count(node) == 0) {
+          if (steps >= minDist_) {
+            marks.insert(node);
+            if (!target.has_value() || node == target.value()) {
+              insertIntoMap(hull, startNode, node);
+            }
+          }
+
+          const auto& successors = edges.successors(node);
+          for (auto successor : successors) {
+            stack.push_back({successor, steps + 1});
+          }
+        }
+      }
+    }
+    return hull;
+  }
+
   /**
    * @brief Prepare a Map and a nodes vector for the transitive hull
    * computation.
@@ -210,9 +269,9 @@
       // TODO<C++23> Use ranges::to.
       nodes.insert(nodes.end(), startNodes.begin(), startNodes.end());
       if (minDist_ == 0) {
         std::span<const Id> targetNodes = sub.getColumn(targetSide.subCol_);
         nodes.insert(nodes.end(), targetNodes.begin(), targetNodes.end());
       }
     }
 
     return {std::move(edges), std::move(nodes)};
@@ -248,22 +307,6 @@
     return {std::move(edges), std::move(nodes)};
   };
 
-  /**
-   * @brief Compute the transitive hull starting at the given nodes,
-   * using the given Map.
-   *
-   * @param edges Adjacency lists, mapping Ids (nodes) to their connected
-   * Ids.
-   * @param nodes A list of Ids. These Ids are used as starting points for the
-   * transitive hull. Thus, this parameter guides the performance of this
-   * algorithm.
-   * @param target Optional target Id. If supplied, only paths which end
-   * in this Id are added to the hull.
-   * @return Map Maps each Id to its connected Ids in the transitive hull
-   */
-  virtual Map transitiveHull(const T& edges, const std::vector<Id>& startNodes,
-                             std::optional<Id> target) const = 0;
-
   virtual T setupEdgesMap(const IdTable& dynSub,
                           const TransitivePathSide& startSide,
                           const TransitivePathSide& targetSide) const = 0;

src/global/RuntimeParameters.h

@@ -45,7 +45,7 @@ inline auto& RuntimeParameters() {
             DurationParameter<std::chrono::seconds, "default-query-timeout">{
                 30s}),
         SizeT<"lazy-index-scan-max-size-materialization">{1'000'000},
-        Bool<"use-binsearch-transitive-path">{false},
+        Bool<"use-binsearch-transitive-path">{true},
         Bool<"group-by-hash-map-enabled">{false}};
   }();
   return params;

test/QueryPlannerTest.cpp

@@ -727,8 +727,8 @@ TEST(QueryPlanner, TransitivePathRightId) {
 
   auto getId = ad_utility::testing::makeGetId(qec->getIndex());
 
-  TransitivePathSide left{std::nullopt, 0, Variable("?x"), 0};
-  TransitivePathSide right{std::nullopt, 1, getId("<o>"), 1};
+  TransitivePathSide left{std::nullopt, 1, Variable("?x"), 0};
+  TransitivePathSide right{std::nullopt, 0, getId("<o>"), 1};
   h::expect(
       "SELECT ?y WHERE {"
       "?x <p>+ <o> }",
@@ -765,8 +765,8 @@ TEST(QueryPlanner, TransitivePathBindRight) {
       h::TransitivePath(
           left, right, 0, std::numeric_limits<size_t>::max(),
           scan("?y", "<p>", "<o>"),
-          scan("?_qlever_internal_variable_query_planner_0", "<p>",
-               "?_qlever_internal_variable_query_planner_1")));
+          h::Sort(scan("?_qlever_internal_variable_query_planner_0", "<p>",
+                       "?_qlever_internal_variable_query_planner_1"))));
 }
 
 // __________________________________________________________________________