fix match step range (#5216)

* use smart pointer change raw pointer * fix error * fix test error * address comment Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
vesoft-inc · Jan 28, 2023 · 9c9fb93 · 9c9fb93
1 parent 70e7864
commit 9c9fb93
Show file tree

Hide file tree

Showing 19 changed files with 164 additions and 85 deletions.
diff --git a/src/common/expression/MatchPathPatternExpression.h b/src/common/expression/MatchPathPatternExpression.h
@@ -53,6 +53,10 @@ class MatchPathPatternExpression final : public Expression {
     return *matchPath_;
   }
 
+  MatchPath* matchPathPtr() const {
+    return matchPath_.get();
+  }
+
  private:
   friend ObjectPool;
   explicit MatchPathPatternExpression(ObjectPool* pool, std::unique_ptr<MatchPath>&& matchPath)

diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h
@@ -46,7 +46,7 @@ struct NodeInfo {
 
 struct EdgeInfo {
   bool anonymous{false};
-  MatchStepRange* range{nullptr};
+  std::unique_ptr<MatchStepRange> range{nullptr};
   std::vector<EdgeType> edgeTypes;
   MatchEdge::Direction direction{MatchEdge::Direction::OUT_EDGE};
   std::vector<std::string> types;

diff --git a/src/graph/executor/algo/ShortestPathBase.h b/src/graph/executor/algo/ShortestPathBase.h
@@ -23,7 +23,7 @@ class ShortestPathBase {
                    std::unordered_map<std::string, std::string>* stats)
       : pathNode_(node), qctx_(qctx), stats_(stats) {
     singleShortest_ = node->singleShortest();
-    maxStep_ = node->stepRange()->max();
+    maxStep_ = node->stepRange().max();
   }
 
   virtual ~ShortestPathBase() {}

diff --git a/src/graph/executor/query/TraverseExecutor.cpp b/src/graph/executor/query/TraverseExecutor.cpp
@@ -112,7 +112,7 @@ folly::Future<Status> TraverseExecutor::getNeighbors() {
 
 Expression* TraverseExecutor::selectFilter() {
   Expression* filter = nullptr;
-  if (!(currentStep_ == 1 && traverse_->zeroStep())) {
+  if (!(currentStep_ == 1 && range_.min() == 0)) {
     filter = const_cast<Expression*>(traverse_->filter());
   }
   if (currentStep_ == 1) {
@@ -166,18 +166,14 @@ folly::Future<Status> TraverseExecutor::handleResponse(RpcResponse&& resps) {
         initVertices_.emplace_back(vertex);
       }
     }
-    if (range_ && range_->min() == 0) {
+    if (range_.min() == 0) {
       result_.rows = buildZeroStepPath();
     }
   }
   expand(iter.get());
   if (!isFinalStep()) {
     if (vids_.empty()) {
-      if (range_ != nullptr) {
-        return buildResult();
-      } else {
-        return folly::makeFuture<Status>(Status::OK());
-      }
+      return buildResult();
     } else {
       return getNeighbors();
     }
@@ -267,12 +263,8 @@ std::vector<Row> TraverseExecutor::buildZeroStepPath() {
 }
 
 folly::Future<Status> TraverseExecutor::buildResult() {
-  size_t minStep = 1;
-  size_t maxStep = 1;
-  if (range_ != nullptr) {
-    minStep = range_->min();
-    maxStep = range_->max();
-  }
+  size_t minStep = range_.min();
+  size_t maxStep = range_.max();
 
   result_.colNames = traverse_->colNames();
   if (maxStep == 0) {

diff --git a/src/graph/executor/query/TraverseExecutor.h b/src/graph/executor/query/TraverseExecutor.h
@@ -67,8 +67,7 @@ class TraverseExecutor final : public StorageAccessExecutor {
   folly::Future<Status> buildPathMultiJobs(size_t minStep, size_t maxStep);
 
   bool isFinalStep() const {
-    return (range_ == nullptr && currentStep_ == 1) ||
-           (range_ != nullptr && (currentStep_ == range_->max() || range_->max() == 0));
+    return currentStep_ == range_.max() || range_.max() == 0;
   }
 
   bool filterSameEdge(const Row& lhs,
@@ -133,7 +132,7 @@ class TraverseExecutor final : public StorageAccessExecutor {
   std::unordered_map<Value, std::vector<Value>, VertexHash, VertexEqual> adjList_;
   std::unordered_map<Value, std::vector<Row>, VertexHash, VertexEqual> dst2PathsMap_;
   const Traverse* traverse_{nullptr};
-  MatchStepRange* range_{nullptr};
+  MatchStepRange range_;
   size_t currentStep_{0};
 };
 

diff --git a/src/graph/optimizer/rule/GetEdgesTransformAppendVerticesLimitRule.cpp b/src/graph/optimizer/rule/GetEdgesTransformAppendVerticesLimitRule.cpp
@@ -58,7 +58,8 @@ bool GetEdgesTransformAppendVerticesLimitRule::match(OptContext *ctx,
   if (colNames[colSize - 2][0] != '_') {  // src
     return false;
   }
-  if (traverse->stepRange() != nullptr) {
+  const auto &stepRange = traverse->stepRange();
+  if (stepRange.min() != 1 || stepRange.max() != 1) {
     return false;
   }
   // Can't apply vertex filter in GetEdges

diff --git a/src/graph/optimizer/rule/GetEdgesTransformRule.cpp b/src/graph/optimizer/rule/GetEdgesTransformRule.cpp
@@ -54,7 +54,8 @@ bool GetEdgesTransformRule::match(OptContext *ctx, const MatchedResult &matched)
   if (colNames[colSize - 2][0] != '_') {  // src
     return false;
   }
-  if (traverse->stepRange() != nullptr) {
+  const auto &stepRange = traverse->stepRange();
+  if (stepRange.min() != 1 || stepRange.max() != 1) {
     return false;
   }
   // Can't apply vertex filter in GetEdges

diff --git a/src/graph/planner/match/MatchPathPlanner.cpp b/src/graph/planner/match/MatchPathPlanner.cpp
@@ -202,6 +202,10 @@ Status MatchPathPlanner::leftExpandFromNode(size_t startIndex, SubPlan& subplan)
     addNodeAlias(node);
     bool expandInto = isExpandInto(dst.alias);
     auto& edge = edgeInfos[i - 1];
+    MatchStepRange stepRange(1, 1);
+    if (edge.range != nullptr) {
+      stepRange = *edge.range;
+    }
     auto traverse = Traverse::make(qctx, subplan.root, spaceId);
     traverse->setSrc(nextTraverseStart);
     auto vertexProps = SchemaUtil::getAllVertexProp(qctx, spaceId, true);
@@ -212,7 +216,7 @@ Status MatchPathPlanner::leftExpandFromNode(size_t startIndex, SubPlan& subplan)
     traverse->setTagFilter(genVertexFilter(node));
     traverse->setEdgeFilter(genEdgeFilter(edge));
     traverse->setEdgeDirection(edge.direction);
-    traverse->setStepRange(edge.range);
+    traverse->setStepRange(stepRange);
     traverse->setDedup();
     // If start from end of the path pattern, the first traverse would not
     // track the previous path, otherwise, it should.
@@ -269,6 +273,10 @@ Status MatchPathPlanner::rightExpandFromNode(size_t startIndex, SubPlan& subplan
     bool expandInto = isExpandInto(dst.alias);
 
     auto& edge = edgeInfos[i];
+    MatchStepRange stepRange(1, 1);
+    if (edge.range != nullptr) {
+      stepRange = *edge.range;
+    }
     auto traverse = Traverse::make(qctx, subplan.root, spaceId);
     traverse->setSrc(nextTraverseStart);
     auto vertexProps = SchemaUtil::getAllVertexProp(qctx, spaceId, true);
@@ -279,7 +287,7 @@ Status MatchPathPlanner::rightExpandFromNode(size_t startIndex, SubPlan& subplan
     traverse->setTagFilter(genVertexFilter(node));
     traverse->setEdgeFilter(genEdgeFilter(edge));
     traverse->setEdgeDirection(edge.direction);
-    traverse->setStepRange(edge.range);
+    traverse->setStepRange(stepRange);
     traverse->setDedup();
     traverse->setTrackPrevPath(i != startIndex);
     traverse->setColNames(

diff --git a/src/graph/planner/match/ShortestPathPlanner.cpp b/src/graph/planner/match/ShortestPathPlanner.cpp
@@ -97,14 +97,19 @@ StatusOr<SubPlan> ShortestPathPlanner::transform(WhereClauseContext* bindWhereCl
 
   auto cp = CrossJoin::make(qctx, leftPlan.root, rightPlan.root);
 
+  MatchStepRange stepRange(1, 1);
+  if (edge.range != nullptr) {
+    stepRange = *edge.range;
+  }
+
   auto shortestPath = ShortestPath::make(qctx, cp, spaceId, singleShortest);
   auto vertexProp = SchemaUtil::getAllVertexProp(qctx, spaceId, true);
   NG_RETURN_IF_ERROR(vertexProp);
   shortestPath->setVertexProps(std::move(vertexProp).value());
   shortestPath->setEdgeProps(SchemaUtil::getEdgeProps(edge, false, qctx, spaceId));
   shortestPath->setReverseEdgeProps(SchemaUtil::getEdgeProps(edge, true, qctx, spaceId));
   shortestPath->setEdgeDirection(edge.direction);
-  shortestPath->setStepRange(edge.range);
+  shortestPath->setStepRange(stepRange);
   shortestPath->setColNames(std::move(colNames));
 
   subplan.root = shortestPath;

diff --git a/src/graph/planner/plan/Algo.cpp b/src/graph/planner/plan/Algo.cpp
@@ -39,7 +39,7 @@ std::unique_ptr<PlanNodeDescription> ProduceAllPaths::explain() const {
 std::unique_ptr<PlanNodeDescription> ShortestPath::explain() const {
   auto desc = SingleInputNode::explain();
   addDescription("singleShortest", folly::toJson(util::toJson(singleShortest_)), desc.get());
-  addDescription("steps", range_ != nullptr ? range_->toString() : "", desc.get());
+  addDescription("steps", range_.toString(), desc.get());
   addDescription("edgeDirection", apache::thrift::util::enumNameSafe(edgeDirection_), desc.get());
   addDescription(
       "vertexProps", vertexProps_ ? folly::toJson(util::toJson(*vertexProps_)) : "", desc.get());

diff --git a/src/graph/planner/plan/Algo.h b/src/graph/planner/plan/Algo.h
@@ -174,7 +174,7 @@ class ShortestPath final : public SingleInputNode {
 
   std::unique_ptr<PlanNodeDescription> explain() const override;
 
-  MatchStepRange* stepRange() const {
+  MatchStepRange stepRange() const {
     return range_;
   }
 
@@ -202,7 +202,7 @@ class ShortestPath final : public SingleInputNode {
     return singleShortest_;
   }
 
-  void setStepRange(MatchStepRange* range) {
+  void setStepRange(const MatchStepRange& range) {
     range_ = range;
   }
 
@@ -234,7 +234,7 @@ class ShortestPath final : public SingleInputNode {
  private:
   GraphSpaceID space_;
   bool singleShortest_{false};
-  MatchStepRange* range_{nullptr};
+  MatchStepRange range_;
   std::unique_ptr<std::vector<EdgeProp>> edgeProps_;
   std::unique_ptr<std::vector<EdgeProp>> reverseEdgeProps_;
   std::unique_ptr<std::vector<VertexProp>> vertexProps_;

diff --git a/src/graph/planner/plan/Query.cpp b/src/graph/planner/plan/Query.cpp
@@ -790,7 +790,7 @@ void Traverse::cloneMembers(const Traverse& g) {
 
 std::unique_ptr<PlanNodeDescription> Traverse::explain() const {
   auto desc = GetNeighbors::explain();
-  addDescription("steps", range_ != nullptr ? range_->toString() : "", desc.get());
+  addDescription("steps", range_.toString(), desc.get());
   addDescription("vertex filter", vFilter_ != nullptr ? vFilter_->toString() : "", desc.get());
   addDescription("edge filter", eFilter_ != nullptr ? eFilter_->toString() : "", desc.get());
   addDescription("if_track_previous_path", folly::toJson(util::toJson(trackPrevPath_)), desc.get());

diff --git a/src/graph/planner/plan/Query.h b/src/graph/planner/plan/Query.h
@@ -1581,17 +1581,17 @@ class Traverse final : public GetNeighbors {
 
   Traverse* clone() const override;
 
-  MatchStepRange* stepRange() const {
+  MatchStepRange stepRange() const {
     return range_;
   }
 
   bool isOneStep() const {
-    return !range_;
+    return range_.min() == 1 && range_.max() == 1;
   }
 
   // Contains zero step
   bool zeroStep() const {
-    return range_ != nullptr && range_->min() == 0;
+    return range_.min() == 0;
   }
 
   Expression* vFilter() const {
@@ -1617,7 +1617,7 @@ class Traverse final : public GetNeighbors {
     return this->colNames().back();
   }
 
-  void setStepRange(MatchStepRange* range) {
+  void setStepRange(const MatchStepRange& range) {
     range_ = range;
   }
 
@@ -1659,7 +1659,7 @@ class Traverse final : public GetNeighbors {
  private:
   void cloneMembers(const Traverse& g);
 
-  MatchStepRange* range_{nullptr};
+  MatchStepRange range_;
   Expression* vFilter_{nullptr};
   Expression* eFilter_{nullptr};
   bool trackPrevPath_{true};