diff --git a/src/graph/optimizer/Optimizer.cpp b/src/graph/optimizer/Optimizer.cpp index 66c2ed21292..669c258982f 100644 --- a/src/graph/optimizer/Optimizer.cpp +++ b/src/graph/optimizer/Optimizer.cpp @@ -126,6 +126,32 @@ void Optimizer::addBodyToGroupNode(OptContext *ctx, gnode->addBody(body); } +namespace { + +bool findArgumentRefPlanNodeInPath(const std::vector &path, PlanNode *argument) { + DCHECK_EQ(argument->kind(), PlanNode::Kind::kArgument); + for (int i = path.size() - 1; i >= 0; i--) { + const auto *pn = path[i]; + if (pn->isBiInput()) { + DCHECK_LT(i, path.size() - 1); + const auto *bpn = static_cast(pn); + if (bpn->right() == path[i + 1]) { + // Argument is in the right side dependency of binary plan node, check the left child + // output columns + if (argument->isColumnsIncludedIn(bpn->left())) { + argument->setInputVar(bpn->left()->outputVar()); + return true; + } + } else { + // Argument is in the left side dependency of binary plan node, continue to find + // next parent plan node + DCHECK_EQ(bpn->left(), path[i + 1]); + } + } + } + return false; +} + Status rewriteArgumentInputVarInternal(PlanNode *root, uint16_t stackDepth, bool &hasArgument, @@ -143,34 +169,7 @@ Status rewriteArgumentInputVarInternal(PlanNode *root, case 0: { if (root->kind() == PlanNode::Kind::kArgument) { hasArgument = true; - for (int i = path.size() - 1; i >= 0; i--) { - const auto *pn = path[i]; - if (pn->isBiInput()) { - DCHECK_LT(i, path.size() - 1); - const auto *bpn = static_cast(pn); - if (bpn->right() == path[i + 1]) { - // Argument is in the right side dependency of binary plan node, check the left child - // output columns - const auto &colNames = bpn->left()->colNames(); - bool found = true; - for (const auto &col : root->colNames()) { - if (std::find(colNames.begin(), colNames.end(), col) == colNames.end()) { - found = false; - break; - } - } - if (found) { - root->setInputVar(bpn->left()->outputVar()); - break; - } - } else { - // Argument is in the left side dependency of binary plan node, continue to find - // next parent plan node - DCHECK_EQ(bpn->left(), path[i + 1]); - } - } - } - if (root->inputVar().empty()) { + if (!findArgumentRefPlanNodeInPath(path, root) || root->inputVar().empty()) { return Status::Error("Could not find the right input variable for argument plan node"); } } @@ -207,6 +206,8 @@ Status rewriteArgumentInputVarInternal(PlanNode *root, return Status::OK(); } +} // namespace + // static Status Optimizer::rewriteArgumentInputVar(PlanNode *root) { bool hasArgument = false; diff --git a/src/graph/planner/plan/PlanNode.cpp b/src/graph/planner/plan/PlanNode.cpp index 46fe8e7593f..c8824b1d85d 100644 --- a/src/graph/planner/plan/PlanNode.cpp +++ b/src/graph/planner/plan/PlanNode.cpp @@ -354,18 +354,15 @@ void PlanNode::setInputVar(const std::string& varname, size_t idx) { } } -Status PlanNode::isColumnsIncluded(const std::string& varname) const { - auto* refVar = qctx()->symTable()->getVar(varname); - // The referenced variable should have all columns used in current node +bool PlanNode::isColumnsIncludedIn(const PlanNode* other) const { + const auto& otherColNames = other->colNames(); for (auto& colName : colNames()) { - auto iter = std::find(refVar->colNames.begin(), refVar->colNames.end(), colName); - if (iter == refVar->colNames.end()) { - return Status::Error("the column referenced by Argument is not included in variable `%s': %s", - varname.c_str(), - colName.c_str()); + auto iter = std::find(otherColNames.begin(), otherColNames.end(), colName); + if (iter == otherColNames.end()) { + return false; } } - return Status::OK(); + return true; } std::unique_ptr PlanNode::explain() const { diff --git a/src/graph/planner/plan/PlanNode.h b/src/graph/planner/plan/PlanNode.h index 3315b896eef..cf840e33ae0 100644 --- a/src/graph/planner/plan/PlanNode.h +++ b/src/graph/planner/plan/PlanNode.h @@ -304,7 +304,7 @@ class PlanNode { return static_cast(this); } - Status isColumnsIncluded(const std::string& varname) const; + bool isColumnsIncludedIn(const PlanNode* other) const; protected: PlanNode(QueryContext* qctx, Kind kind); diff --git a/tests/tck/features/match/AllShortestPaths.feature b/tests/tck/features/match/AllShortestPaths.feature index 12bc9f50426..d2109881aee 100644 --- a/tests/tck/features/match/AllShortestPaths.feature +++ b/tests/tck/features/match/AllShortestPaths.feature @@ -476,16 +476,64 @@ Feature: allShortestPaths """ Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down. -# Scenario: allShortestPaths for argument issue -# When executing query: -# """ -# MATCH (a:player), (b:player) -# MATCH p= allShortestPaths((a:player {name:"Tim Duncan"})-[*..15]-(b:player {age:33})) -# WITH nodes(p) AS pathNodes -# UNWIND pathNodes AS node -# RETURN DISTINCT node -# """ -# Then the result should be, in any order, with relax comparison: -# | node | -# | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | -# | ("Dejounte Murray" :player{age: 29, name: "Dejounte Murray"}) | + Scenario: allShortestPaths for argument issue + When profiling query: + """ + MATCH (a:player{name:'Tim Duncan'}), (b:player{name:'Tony Parker'}) + WITH a AS b, b AS a + MATCH allShortestPaths((a)-[:like*1..3]-(b)) + RETURN a, b + """ + Then the result should be, in any order, with relax comparison: + | a | b | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Tim Duncan" :player{name: "Tim Duncan"}) | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Tim Duncan" :player{name: "Tim Duncan"}) | + And the execution plan should be: + | id | name | dependencies | operator info | + | 19 | Project | 18 | | + | 18 | BiInnerJoin | 10,17 | | + | 10 | Project | 9 | | + | 9 | BiCartesianProduct | 24,25 | | + | 24 | AppendVertices | 20 | | + | 20 | IndexScan | 2 | | + | 2 | Start | | | + | 25 | AppendVertices | 21 | | + | 21 | IndexScan | 6 | | + | 6 | Start | | | + | 17 | Project | 16 | | + | 16 | ShortestPath | 15 | | + | 15 | BiCartesianProduct | 12,14 | | + | 12 | Project | 11 | | + | 11 | Argument | | | + | 14 | Project | 13 | | + | 13 | Argument | | | + When profiling query: + """ + MATCH (a:player{name:'Tim Duncan'}), (b:player{name:'Tony Parker'}) + WITH a AS b, b AS a + OPTIONAL MATCH allShortestPaths((a)-[:like*1..3]-(b)) + RETURN a, b + """ + Then the result should be, in any order, with relax comparison: + | a | b | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Tim Duncan" :player{name: "Tim Duncan"}) | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | ("Tim Duncan" :player{name: "Tim Duncan"}) | + And the execution plan should be: + | id | name | dependencies | operator info | + | 19 | Project | 18 | | + | 18 | BiLeftJoin | 10,17 | | + | 10 | Project | 9 | | + | 9 | BiCartesianProduct | 24,25 | | + | 24 | AppendVertices | 20 | | + | 20 | IndexScan | 2 | | + | 2 | Start | | | + | 25 | AppendVertices | 21 | | + | 21 | IndexScan | 6 | | + | 6 | Start | | | + | 17 | Project | 16 | | + | 16 | ShortestPath | 15 | | + | 15 | BiCartesianProduct | 12,14 | | + | 12 | Project | 11 | | + | 11 | Argument | | | + | 14 | Project | 13 | | + | 13 | Argument | | |