Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix match step range #5216

Merged
merged 5 commits into from
Jan 11, 2023
Merged

fix match step range #5216

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79894dc
use smart pointer change raw pointer
nevermore3 Jan 6, 2023
383c379
fix error
nevermore3 Jan 6, 2023
f8eb099
fix test error
nevermore3 Jan 10, 2023
a7f73d8
address comment
nevermore3 Jan 11, 2023
a181bcf
Merge branch 'master' into fix_match_step_range
Sophie-Xie Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/common/expression/MatchPathPatternExpression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/graph/context/ast/CypherAstContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ struct NodeInfo {

struct EdgeInfo {
bool anonymous{false};
MatchStepRange* range{nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I known, this AST is released when query executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it in the memory pool?

Copy link
Contributor

@Shylock-Hg Shylock-Hg Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All AST will be hold by QueryInstance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AST information will be transferred to Sentence in validate phase. the sentence is saved in the querycontext during the execution phase,. The current problem is the wild pointer caused by the release of the temporary variable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better change

nebula/src/graph/validator/MatchValidator.cpp

Line 1180 in fdca644

std::vector<MatchPath> pathPreds;
to std::vector<MatchPath*>, we assume lifetime of AST is available before complete query execution.

std::unique_ptr<MatchStepRange> range{nullptr};
std::vector<EdgeType> edgeTypes;
MatchEdge::Direction direction{MatchEdge::Direction::OUT_EDGE};
std::vector<std::string> types;
Expand Down
2 changes: 1 addition & 1 deletion src/graph/executor/algo/ShortestPathBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
Expand Down
18 changes: 5 additions & 13 deletions src/graph/executor/query/TraverseExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 2 additions & 3 deletions src/graph/executor/query/TraverseExecutor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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};
};

Expand Down
3 changes: 2 additions & 1 deletion src/graph/optimizer/rule/GetEdgesTransformAppendVerticesLimitRule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/graph/optimizer/rule/GetEdgesTransformRule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 10 additions & 2 deletions src/graph/planner/match/MatchPathPlanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down
7 changes: 6 additions & 1 deletion src/graph/planner/match/ShortestPathPlanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/graph/planner/plan/Algo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
6 changes: 3 additions & 3 deletions src/graph/planner/plan/Algo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class ShortestPath final : public SingleInputNode {

std::unique_ptr<PlanNodeDescription> explain() const override;

MatchStepRange* stepRange() const {
MatchStepRange stepRange() const {
return range_;
}

Expand Down Expand Up @@ -202,7 +202,7 @@ class ShortestPath final : public SingleInputNode {
return singleShortest_;
}

void setStepRange(MatchStepRange* range) {
void setStepRange(const MatchStepRange& range) {
range_ = range;
}

Expand Down Expand Up @@ -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_;
Expand Down
2 changes: 1 addition & 1 deletion src/graph/planner/plan/Query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
10 changes: 5 additions & 5 deletions src/graph/planner/plan/Query.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -1617,7 +1617,7 @@ class Traverse final : public GetNeighbors {
return this->colNames().back();
}

void setStepRange(MatchStepRange* range) {
void setStepRange(const MatchStepRange& range) {
range_ = range;
}

Expand Down Expand Up @@ -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};
Expand Down
Loading