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

Jump to bottom

Fix argument #4950

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Fix argument #4950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion src/graph/planner/match/MatchPathPlanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "graph/planner/match/WhereClausePlanner.h"
#include "graph/planner/plan/Algo.h"
#include "graph/planner/plan/Logic.h"
#include "graph/planner/plan/PlanNode.h"
#include "graph/planner/plan/Query.h"
#include "graph/util/ExpressionUtils.h"
#include "graph/util/SchemaUtil.h"
Expand Down Expand Up @@ -165,7 +166,9 @@ Status MatchPathPlanner::findStarts(
return Status::SemanticError("Can't solve the start vids from the sentence.");
}

if (matchClausePlan.tail->isSingleInput()) {
// Both StartNode and Argument are leaf plannodes
if (matchClausePlan.tail->isSingleInput() &&
matchClausePlan.tail->kind() != PlanNode::Kind::kArgument) {
auto start = StartNode::make(qctx);
matchClausePlan.tail->setDep(0, start);
matchClausePlan.tail = start;
Expand Down
5 changes: 5 additions & 0 deletions src/graph/planner/match/MatchPlanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma
}
}
if (!intersectedAliases.empty()) {
if (matchPlan.tail->kind() == PlanNode::Kind::kArgument) {
// The input of the argument operator is always the output of the plan on the other side of
// the join
matchPlan.tail->setInputVar(queryPlan.root->outputVar());
}
if (matchCtx->isOptional) {
// connect LeftJoin match filter
auto& whereCtx = matchCtx->where;
Expand Down
9 changes: 6 additions & 3 deletions src/graph/planner/match/ShortestPathPlanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,12 @@ StatusOr<SubPlan> ShortestPathPlanner::transform(
auto status = nodeFinder->transform(&nodeCtx);
NG_RETURN_IF_ERROR(status);
auto plan = status.value();
auto start = StartNode::make(qctx);
plan.tail->setDep(0, start);
plan.tail = start;
if (plan.tail->kind() != PlanNode::Kind::kStart &&
plan.tail->kind() != PlanNode::Kind::kArgument) {
auto start = StartNode::make(qctx);
plan.tail->setDep(0, start);
plan.tail = start;
}

auto initExpr = nodeCtx.initialExpr->clone();
auto columns = qctx->objPool()->makeAndAdd<YieldColumns>();
Expand Down
5 changes: 2 additions & 3 deletions src/graph/planner/plan/Logic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ std::unique_ptr<PlanNodeDescription> Select::explain() const {
return desc;
}

Argument::Argument(QueryContext* qctx, std::string alias, const PlanNode* dep)
: SingleInputNode(qctx, Kind::kArgument, dep) {
Argument::Argument(QueryContext* qctx, std::string alias) : PlanNode(qctx, Kind::kArgument) {
alias_ = alias;
// An argument is a kind of leaf node, it has no dependencies but read a variable.
inputVars_.emplace_back(nullptr);
Expand All @@ -87,7 +86,7 @@ void Argument::cloneMembers(const Argument& arg) {
}

std::unique_ptr<PlanNodeDescription> Argument::explain() const {
auto desc = SingleInputNode::explain();
auto desc = PlanNode::explain();
addDescription("inputVar", inputVar(), desc.get());
return desc;
}
Expand Down
8 changes: 4 additions & 4 deletions src/graph/planner/plan/Logic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ class PassThroughNode final : public SingleInputNode {
};

// This operator is used for getting a named alias from another executed operator.
class Argument final : public SingleInputNode {
class Argument final : public PlanNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Treat Argument as a leaf node and delete the useless StartNode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Emm. A little wired, node with one input should inherit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just ok.

public:
static Argument* make(QueryContext* qctx, std::string alias, const PlanNode* dep = nullptr) {
return qctx->objPool()->makeAndAdd<Argument>(qctx, alias, dep);
static Argument* make(QueryContext* qctx, std::string alias) {
return qctx->objPool()->makeAndAdd<Argument>(qctx, alias);
}

PlanNode* clone() const override;
Expand All @@ -163,7 +163,7 @@ class Argument final : public SingleInputNode {

private:
friend ObjectPool;
Argument(QueryContext* qctx, std::string alias, const PlanNode* dep = nullptr);
Argument(QueryContext* qctx, std::string alias);

void cloneMembers(const Argument&);

Expand Down
6 changes: 2 additions & 4 deletions tests/tck/features/bugfix/ArgumentPlanNodeDep.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ Feature: Fix Argument plan node dependency
And the execution plan should be:
| id | name | dependencies | operator info |
| 11 | Aggregate | 10 | |
| 10 | BiInnerJoin | 9,4 | |
| 9 | Project | 8 | |
| 10 | BiInnerJoin | 8,4 | |
| 8 | AppendVertices | 7 | |
| 7 | Dedup | 6 | |
| 6 | PassThrough | 5 | |
| 5 | Start | | |
| 4 | Project | 3 | |
| 3 | AppendVertices | 2 | |
| 2 | Traverse | 1 | |
| 1 | Argument | 0 | |
| 0 | Start | | |
| 1 | Argument | | |
12 changes: 12 additions & 0 deletions tests/tck/features/match/MultiLineMultiQueryParts.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ Feature: Multi Line Multi Query Parts
| "Tim Duncan" | "Boris Diaw" | "Spurs" |
| "Tim Duncan" | "Boris Diaw" | "Suns" |
| "Tim Duncan" | "Boris Diaw" | "Tim Duncan" |
When executing query:
"""
MATCH (v:player{name:"Tim Duncan"})-[:like*1..3]->(n)
WITH DISTINCT v, n
MATCH (v)-[:serve]->(nn) return id(v) AS vid, id(nn) AS nnid
"""
Then the result should be, in any order:
| vid | nnid |
| "Tim Duncan" | "Spurs" |
| "Tim Duncan" | "Spurs" |
| "Tim Duncan" | "Spurs" |
| "Tim Duncan" | "Spurs" |
When executing query:
"""
MATCH (m)-[]-(n),(n) WHERE id(m)=="Tim Duncan" and id(n)=="Tony Parker"
Expand Down
18 changes: 6 additions & 12 deletions tests/tck/features/optimizer/PrunePropertiesRule.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,7 @@ Feature: Prune Properties rule
| 29 | Project | 8 | |
| 8 | AppendVertices | 7 | { "props": "[{\"tagId\":10,\"props\":[\"name\"]}]" } |
| 7 | Traverse | 6 | { "vertexProps": "[{\"tagId\":9,\"props\":[\"name\"]}]", "edgeProps": "[{\"type\": -5, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"type\": 5, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": -3}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 3}, {\"type\": -4, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 4}]" } |
| 6 | Argument | 31 | |
| 31 | Start | | |
| 6 | Argument | | |
| 30 | Project | 12 | |
| 12 | AppendVertices | 11 | { "props": "[{\"props\":[\"name\"],\"tagId\":9}]" } |
| 11 | Traverse | 10 | { "vertexProps": "[{\"props\":[\"name\"],\"tagId\":10}]", "edgeProps": "[{\"type\": -5, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"type\": 5, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": -3}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 3}, {\"type\": -4, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 4}]" } |
Expand Down Expand Up @@ -264,8 +263,7 @@ Feature: Prune Properties rule
| 11 | Project | 10 | |
| 10 | AppendVertices | 9 | { "props": "[{\"tagId\":9,\"props\":[\"name\"]}, {\"tagId\":10,\"props\":[\"name\"]}]" } |
| 9 | Traverse | 8 | { "vertexProps": "[{\"tagId\":9,\"props\":[\"name\"]}]" } |
| 8 | Argument | 33 | |
| 33 | Start | | |
| 8 | Argument | | |
When profiling query:
"""
MATCH (m)-[]-(n) WHERE id(m)=="Tim Duncan"
Expand Down Expand Up @@ -300,13 +298,11 @@ Feature: Prune Properties rule
| 33 | Project | 10 | |
| 10 | AppendVertices | 9 | { "props": "[{\"tagId\":10,\"props\":[\"name\"]}]" } |
| 9 | Traverse | 8 | { "vertexProps": "[{\"tagId\":9,\"props\":[\"name\"]}]" } |
| 8 | Argument | 35 | |
| 35 | Start | | |
| 8 | Argument | | |
| 34 | Project | 13 | |
| 13 | AppendVertices | 12 | { "props": "[{\"tagId\":9,\"props\":[\"name\"]}]" } |
| 12 | Traverse | 11 | { "vertexProps": "[{\"tagId\":10,\"props\":[\"name\"]}]", "edgeProps": "[{\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": -5}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 5}, {\"type\": -3, \"props\": [\"_dst\", \"_rank\", \"_type\"]}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 3}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": -4}, {\"props\": [\"_dst\", \"_rank\", \"_type\"], \"type\": 4}]" } |
| 11 | Argument | 36 | |
| 36 | Start | | |
| 11 | Argument | | |
When profiling query:
"""
MATCH (v:player{name:"Tony Parker"})
Expand Down Expand Up @@ -483,8 +479,7 @@ Feature: Prune Properties rule
| 12 | Project | 18 | |
| 18 | AppendVertices | 10 | { "props": "[{\"props\":[\"_tag\"],\"tagId\":10}]" } |
| 10 | Traverse | 8 | {"vertexProps": "[{\"props\":[\"name\", \"age\", \"_tag\"],\"tagId\":9}, {\"props\":[\"name\", \"speciality\", \"_tag\"],\"tagId\":8}, {\"props\":[\"name\", \"_tag\"],\"tagId\":10}]", "edgeProps": "[{\"type\": 4, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 8 | Argument | 9 | |
| 9 | Start | | |
| 8 | Argument | | |

@distonly
Scenario: union match
Expand Down Expand Up @@ -558,8 +553,7 @@ Feature: Prune Properties rule
| 15 | Traverse | 14 | {"vertexProps": "[{\"props\":[\"age\"],\"tagId\":9}]", "edgeProps": "[{\"type\": 4, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 14 | Traverse | 13 | {"vertexProps": "", "edgeProps": "[{\"type\": -3, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 13 | Traverse | 11 | {"vertexProps": "", "edgeProps": "[{\"type\": -3, \"props\": [\"_type\", \"_rank\", \"_dst\"]}, {\"type\": 3, \"props\": [\"_type\", \"_rank\", \"_dst\"]}]" } |
| 11 | Argument | 12 | |
| 12 | Start | | |
| 11 | Argument | | |

@distonly
Scenario: test properties:
Expand Down