From fd8f632789e817f634a70b4215b5166d72295f71 Mon Sep 17 00:00:00 2001 From: jievince <38901892+jievince@users.noreply.github.com> Date: Mon, 22 Aug 2022 17:14:51 +0800 Subject: [PATCH 1/4] Minor fixes and improvements --- src/graph/planner/ngql/GoPlanner.cpp | 5 +- src/graph/validator/GoValidator.cpp | 3 +- tests/tck/features/go/SimpleCase.feature | 63 +++++++++++++++++++++++- 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/graph/planner/ngql/GoPlanner.cpp b/src/graph/planner/ngql/GoPlanner.cpp index bcaa4964bc0..310ffa01776 100644 --- a/src/graph/planner/ngql/GoPlanner.cpp +++ b/src/graph/planner/ngql/GoPlanner.cpp @@ -439,12 +439,11 @@ SubPlan GoPlanner::oneStepPlan(SubPlan& startVidPlan) { SubPlan GoPlanner::nStepsPlan(SubPlan& startVidPlan) { auto qctx = goCtx_->qctx; - auto isSimple = goCtx_->isSimple; loopStepVar_ = qctx->vctx()->anonVarGen()->getVar(); auto* start = StartNode::make(qctx); PlanNode* scan = nullptr; - if (isSimple) { + if (!goCtx_->joinInput) { auto* gd = GetDstBySrc::make(qctx, start, goCtx_->space.id); gd->setSrc(goCtx_->from.src); gd->setEdgeTypes(buildEdgeTypes()); @@ -461,7 +460,7 @@ SubPlan GoPlanner::nStepsPlan(SubPlan& startVidPlan) { auto* sampleLimit = buildSampleLimit(scan); PlanNode* getDst = nullptr; - if (isSimple) { + if (!goCtx_->joinInput) { auto* dedup = Dedup::make(qctx, sampleLimit); dedup->setOutputVar(goCtx_->vidsVar); dedup->setColNames(goCtx_->colNames); diff --git a/src/graph/validator/GoValidator.cpp b/src/graph/validator/GoValidator.cpp index 3c0a4b8d153..2661af54b82 100644 --- a/src/graph/validator/GoValidator.cpp +++ b/src/graph/validator/GoValidator.cpp @@ -187,7 +187,8 @@ void GoValidator::extractPropExprs(const Expression* expr, Expression* GoValidator::rewriteVertexEdge2EdgeProp(const Expression* expr) { auto pool = qctx_->objPool(); - const auto& name = expr->toString(); + auto name = expr->toString(); + std::transform(name.begin(), name.end(), name.begin(), ::tolower); if (name == "id($^)" || name == "src(edge)") { return EdgeSrcIdExpression::make(pool, "*"); } diff --git a/tests/tck/features/go/SimpleCase.feature b/tests/tck/features/go/SimpleCase.feature index c90f411d8f4..ef6817b9ded 100644 --- a/tests/tck/features/go/SimpleCase.feature +++ b/tests/tck/features/go/SimpleCase.feature @@ -21,7 +21,7 @@ Feature: Simple case | 1 | GetDstBySrc | 0 | | | 0 | Start | | | - Scenario: go m steps yield distinct dst id + Scenario: go m steps When profiling query: """ GO 3 STEPS FROM "Tony Parker" OVER serve BIDIRECT YIELD DISTINCT id($$) AS dst | YIELD count(*) @@ -39,6 +39,67 @@ Feature: Simple case | 2 | GetDstBySrc | 1 | | | 1 | Start | | | | 0 | Start | | | + # The last step degenerates to `GetNeighbors` when yield or filter properties other than `_dst` + When profiling query: + """ + GO 3 STEPS FROM "Tony Parker" OVER serve BIDIRECT YIELD id($$) AS dst | YIELD count(*) + """ + Then the result should be, in any order, with relax comparison: + | count(*) | + | 65 | + And the execution plan should be: + | id | name | dependencies | operator info | + | 7 | Aggregate | 6 | | + | 6 | Project | 5 | | + | 5 | GetNeighbors | 4 | | + | 4 | Loop | 0 | {"loopBody": "3"} | + | 3 | Dedup | 2 | | + | 2 | GetDstBySrc | 1 | | + | 1 | Start | | | + | 0 | Start | | | + When profiling query: + """ + GO 3 STEPS FROM "Tony Parker" OVER serve BIDIRECT YIELD $$.player.age AS age | YIELD count(*) + """ + Then the result should be, in any order, with relax comparison: + | count(*) | + | 65 | + And the execution plan should be: + | id | name | dependencies | operator info | + | 11 | Aggregate | 10 | | + | 10 | Project | 9 | | + | 9 | LeftJoin | 8 | | + | 8 | Project | 7 | | + | 7 | GetVertices | 6 | | + | 6 | Project | 5 | | + | 5 | GetNeighbors | 4 | | + | 4 | Loop | 0 | {"loopBody": "3"} | + | 3 | Dedup | 2 | | + | 2 | GetDstBySrc | 1 | | + | 1 | Start | | | + | 0 | Start | | | + When profiling query: + """ + GO 3 STEPS FROM "Tony Parker" OVER * WHERE $$.player.age > 36 YIELD $$.player.age AS age | YIELD count(*) + """ + Then the result should be, in any order, with relax comparison: + | count(*) | + | 10 | + And the execution plan should be: + | id | name | dependencies | operator info | + | 12 | Aggregate | 11 | | + | 11 | Project | 10 | | + | 10 | Filter | 9 | | + | 9 | LeftJoin | 8 | | + | 8 | Project | 7 | | + | 7 | GetVertices | 6 | | + | 6 | Project | 5 | | + | 5 | GetNeighbors | 4 | | + | 4 | Loop | 0 | {"loopBody": "3"} | + | 3 | Dedup | 2 | | + | 2 | GetDstBySrc | 1 | | + | 1 | Start | | | + | 0 | Start | | | Scenario: go m to n steps yield distinct dst id When profiling query: From 3c06643901277787b0af90e75a6b60de675b0526 Mon Sep 17 00:00:00 2001 From: jievince <38901892+jievince@users.noreply.github.com> Date: Mon, 29 Aug 2022 11:41:50 +0800 Subject: [PATCH 2/4] fix ctest --- .../validator/test/QueryValidatorTest.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/graph/validator/test/QueryValidatorTest.cpp b/src/graph/validator/test/QueryValidatorTest.cpp index a7777c5f843..c58471c3eae 100644 --- a/src/graph/validator/test/QueryValidatorTest.cpp +++ b/src/graph/validator/test/QueryValidatorTest.cpp @@ -81,8 +81,7 @@ TEST_F(QueryValidatorTest, GoNSteps) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart}; EXPECT_TRUE(checkResult(query, expected)); } @@ -96,8 +95,7 @@ TEST_F(QueryValidatorTest, GoNSteps) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart, }; EXPECT_TRUE(checkResult(query, expected)); @@ -114,8 +112,7 @@ TEST_F(QueryValidatorTest, GoNSteps) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart, }; EXPECT_TRUE(checkResult(query, expected)); @@ -131,8 +128,7 @@ TEST_F(QueryValidatorTest, GoNSteps) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart}; EXPECT_TRUE(checkResult(query, expected)); } @@ -166,8 +162,7 @@ TEST_F(QueryValidatorTest, GoWithPipe) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart}; EXPECT_TRUE(checkResult(query, expected)); } @@ -459,8 +454,7 @@ TEST_F(QueryValidatorTest, GoReversely) { PK::kLoop, PK::kStart, PK::kDedup, - PK::kProject, - PK::kGetNeighbors, + PK::kGetDstBySrc, PK::kStart, }; EXPECT_TRUE(checkResult(query, expected)); From 98bdd279f4575e1304e770411da6d79ab4600b1b Mon Sep 17 00:00:00 2001 From: jievince <38901892+jievince@users.noreply.github.com> Date: Mon, 29 Aug 2022 15:01:35 +0800 Subject: [PATCH 3/4] fix rewriteVertexEdge2EdgeProp --- src/graph/validator/GoValidator.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/graph/validator/GoValidator.cpp b/src/graph/validator/GoValidator.cpp index 2661af54b82..34795c6f670 100644 --- a/src/graph/validator/GoValidator.cpp +++ b/src/graph/validator/GoValidator.cpp @@ -188,7 +188,6 @@ void GoValidator::extractPropExprs(const Expression* expr, Expression* GoValidator::rewriteVertexEdge2EdgeProp(const Expression* expr) { auto pool = qctx_->objPool(); auto name = expr->toString(); - std::transform(name.begin(), name.end(), name.begin(), ::tolower); if (name == "id($^)" || name == "src(edge)") { return EdgeSrcIdExpression::make(pool, "*"); } From 4291c68c1edd4283d7df7918781641817e15ac99 Mon Sep 17 00:00:00 2001 From: jievince <38901892+jievince@users.noreply.github.com> Date: Tue, 30 Aug 2022 11:17:07 +0800 Subject: [PATCH 4/4] fix fix --- src/graph/planner/ngql/GoPlanner.cpp | 6 +-- tests/tck/features/go/SimpleCase.feature | 22 ++++++++ .../features/match/SingleShorestPath.feature | 52 +++++++++---------- 3 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/graph/planner/ngql/GoPlanner.cpp b/src/graph/planner/ngql/GoPlanner.cpp index 310ffa01776..e2fbf344028 100644 --- a/src/graph/planner/ngql/GoPlanner.cpp +++ b/src/graph/planner/ngql/GoPlanner.cpp @@ -443,7 +443,7 @@ SubPlan GoPlanner::nStepsPlan(SubPlan& startVidPlan) { auto* start = StartNode::make(qctx); PlanNode* scan = nullptr; - if (!goCtx_->joinInput) { + if (!goCtx_->joinInput && goCtx_->limits.empty()) { auto* gd = GetDstBySrc::make(qctx, start, goCtx_->space.id); gd->setSrc(goCtx_->from.src); gd->setEdgeTypes(buildEdgeTypes()); @@ -460,7 +460,7 @@ SubPlan GoPlanner::nStepsPlan(SubPlan& startVidPlan) { auto* sampleLimit = buildSampleLimit(scan); PlanNode* getDst = nullptr; - if (!goCtx_->joinInput) { + if (!goCtx_->joinInput && goCtx_->limits.empty()) { auto* dedup = Dedup::make(qctx, sampleLimit); dedup->setOutputVar(goCtx_->vidsVar); dedup->setColNames(goCtx_->colNames); @@ -626,7 +626,7 @@ StatusOr GoPlanner::transform(AstContext* astCtx) { } bool GoPlanner::isSimpleCase() { - if (goCtx_->joinDst || goCtx_->filter || !goCtx_->distinct) { + if (goCtx_->joinDst || goCtx_->filter || !goCtx_->distinct || !goCtx_->limits.empty()) { return false; } auto& exprProps = goCtx_->exprProps; diff --git a/tests/tck/features/go/SimpleCase.feature b/tests/tck/features/go/SimpleCase.feature index ef6817b9ded..61131763c63 100644 --- a/tests/tck/features/go/SimpleCase.feature +++ b/tests/tck/features/go/SimpleCase.feature @@ -100,6 +100,28 @@ Feature: Simple case | 2 | GetDstBySrc | 1 | | | 1 | Start | | | | 0 | Start | | | + # Because GetDstBySrc doesn't support limit push down, so degenrate to GetNeighbors when the query contains limit/simple clause + When profiling query: + """ + GO 3 STEPS FROM "Tony Parker" OVER * YIELD DISTINCT id($$) LIMIT [100, 100, 100] | YIELD count(*) + """ + Then the result should be, in any order, with relax comparison: + | count(*) | + | 13 | + And the execution plan should be: + | id | name | dependencies | operator info | + | 11 | Aggregate | 10 | | + | 10 | Dedup | 9 | | + | 9 | Project | 12 | | + | 12 | Limit | 13 | | + | 13 | GetNeighbors | 6 | | + | 6 | Loop | 0 | {"loopBody": "5"} | + | 5 | Dedup | 4 | | + | 4 | Project | 14 | | + | 14 | Limit | 15 | | + | 15 | GetNeighbors | 1 | | + | 1 | Start | | | + | 0 | Start | | | Scenario: go m to n steps yield distinct dst id When profiling query: diff --git a/tests/tck/features/match/SingleShorestPath.feature b/tests/tck/features/match/SingleShorestPath.feature index 8a9adf506f2..7baca9fe21c 100644 --- a/tests/tck/features/match/SingleShorestPath.feature +++ b/tests/tck/features/match/SingleShorestPath.feature @@ -66,34 +66,34 @@ Feature: single shortestPath | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})> | When executing query: """ - MATCH p = shortestPath( (a:player{age:30})-[e*..5]->(b:team) ) RETURN p + MATCH p = shortestPath( (a:player{age:30})-[e*..5]->(b:team) ) RETURN b, length(p) """ Then the result should be, in any order, with relax comparison: - | p | - | <("DeAndre Jordan" :player{age: 30, name: "DeAndre Jordan"})-[:serve@0 {end_year: 2019, start_year: 2019}]->("Knicks" :team{name: "Knicks"})> | - | <("DeAndre Jordan" :player{age: 30, name: "DeAndre Jordan"})-[:serve@0 {end_year: 2019, start_year: 2018}]->("Mavericks" :team{name: "Mavericks"})> | - | <("DeAndre Jordan" :player{age: 30, name: "DeAndre Jordan"})-[:serve@0 {end_year: 2018, start_year: 2008}]->("Clippers" :team{name: "Clippers"})> | - | <("Kevin Durant" :player{age: 30, name: "Kevin Durant"})-[:serve@0 {end_year: 2016, start_year: 2007}]->("Thunders" :team{name: "Thunders"})> | - | <("Kevin Durant" :player{age: 30, name: "Kevin Durant"})-[:serve@0 {end_year: 2019, start_year: 2016}]->("Warriors" :team{name: "Warriors"})> | - | <("Russell Westbrook" :player{age: 30, name: "Russell Westbrook"})-[:like@0 {likeness: 90}]->("James Harden" :player{age: 29, name: "James Harden"})-[:serve@0 {end_year: 2019, start_year: 2012}]->("Rockets" :team{name: "Rockets"})> | - | <("Russell Westbrook" :player{age: 30, name: "Russell Westbrook"})-[:serve@0 {end_year: 2019, start_year: 2008}]->("Thunders" :team{name: "Thunders"})> | - | <("Russell Westbrook" :player{age: 30, name: "Russell Westbrook"})-[:like@0 {likeness: 90}]->("Paul George" :player{age: 28, name: "Paul George"})-[:serve@0 {end_year: 2017, start_year: 2010}]->("Pacers" :team{name: "Pacers"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:serve@0 {end_year: 2014, start_year: 2010}]->("Heat" :team{name: "Heat"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:like@0 {likeness: 100}]->("Ray Allen" :player{age: 43, name: "Ray Allen"})-[:like@0 {likeness: 9}]->("Rajon Rondo" :player{age: 33, name: "Rajon Rondo"})-[:serve@0 {end_year: 2016, start_year: 2015}]->("Kings" :team{name: "Kings"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:serve@0 {end_year: 2019, start_year: 2018}]->("Lakers" :team{name: "Lakers"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("Carmelo Anthony" :player{age: 34, name: "Carmelo Anthony"})-[:serve@0 {end_year: 2017, start_year: 2011}]->("Knicks" :team{name: "Knicks"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:serve@0 {end_year: 2011, start_year: 2005}]->("Hornets" :team{name: "Hornets"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:like@0 {likeness: 100}]->("Ray Allen" :player{age: 43, name: "Ray Allen"})-[:like@0 {likeness: 9}]->("Rajon Rondo" :player{age: 33, name: "Rajon Rondo"})-[:serve@0 {end_year: 2015, start_year: 2014}]->("Mavericks" :team{name: "Mavericks"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:serve@0 {end_year: 2018, start_year: 2009}]->("Clippers" :team{name: "Clippers"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("Dwyane Wade" :player{age: 37, name: "Dwyane Wade"})-[:serve@0 {end_year: 2017, start_year: 2016}]->("Bulls" :team{name: "Bulls"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:like@0 {likeness: 100}]->("Ray Allen" :player{age: 43, name: "Ray Allen"})-[:serve@0 {end_year: 2003, start_year: 1996}]->("Bucks" :team{name: "Bucks"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("Carmelo Anthony" :player{age: 34, name: "Carmelo Anthony"})-[:serve@0 {end_year: 2011, start_year: 2003}]->("Nuggets" :team{name: "Nuggets"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:like@0 {likeness: 100}]->("Ray Allen" :player{age: 43, name: "Ray Allen"})-[:serve@0 {end_year: 2012, start_year: 2007}]->("Celtics" :team{name: "Celtics"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:like@0 {likeness: 100}]->("Ray Allen" :player{age: 43, name: "Ray Allen"})-[:like@0 {likeness: 9}]->("Rajon Rondo" :player{age: 33, name: "Rajon Rondo"})-[:serve@0 {end_year: 2018, start_year: 2017}]->("Pelicans" :team{name: "Pelicans"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:serve@0 {end_year: 2021, start_year: 2017}]->("Rockets" :team{name: "Rockets"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("Carmelo Anthony" :player{age: 34, name: "Carmelo Anthony"})-[:serve@0 {end_year: 2018, start_year: 2017}]->("Thunders" :team{name: "Thunders"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:serve@0 {end_year: 2019, start_year: 2018}]->("Pistons" :team{name: "Pistons"})> | - | <("Blake Griffin" :player{age: 30, name: "Blake Griffin"})-[:like@0 {likeness: -1}]->("Chris Paul" :player{age: 33, name: "Chris Paul"})-[:like@0 {likeness: 90}]->("LeBron James" :player{age: 34, name: "LeBron James"})-[:serve@0 {end_year: 2010, start_year: 2003}]->("Cavaliers" :team{name: "Cavaliers"})> | + | b | length(p) | + | ("Celtics":team{name:"Celtics"}) | 4 | + | ("Kings":team{name:"Kings"}) | 5 | + | ("Cavaliers":team{name:"Cavaliers"}) | 3 | + | ("Thunders":team{name:"Thunders"}) | 3 | + | ("Nuggets":team{name:"Nuggets"}) | 3 | + | ("Rockets":team{name:"Rockets"}) | 2 | + | ("Mavericks":team{name:"Mavericks"}) | 5 | + | ("Bucks":team{name:"Bucks"}) | 4 | + | ("Lakers":team{name:"Lakers"}) | 3 | + | ("Pistons":team{name:"Pistons"}) | 1 | + | ("Bulls":team{name:"Bulls"}) | 3 | + | ("Clippers":team{name:"Clippers"}) | 1 | + | ("Pelicans":team{name:"Pelicans"}) | 5 | + | ("Knicks":team{name:"Knicks"}) | 3 | + | ("Heat":team{name:"Heat"}) | 3 | + | ("Hornets":team{name:"Hornets"}) | 2 | + | ("Mavericks":team{name:"Mavericks"}) | 1 | + | ("Clippers":team{name:"Clippers"}) | 1 | + | ("Knicks":team{name:"Knicks"}) | 1 | + | ("Thunders":team{name:"Thunders"}) | 1 | + | ("Rockets":team{name:"Rockets"}) | 2 | + | ("Pacers":team{name:"Pacers"}) | 2 | + | ("Thunders":team{name:"Thunders"}) | 1 | + | ("Warriors":team{name:"Warriors"}) | 1 | When executing query: """ MATCH p = shortestPath( (a:player{age:30})-[e*..5]->(b:team) )