From 8a7f48e250748eae47f9b4cf079baf95d28a8e76 Mon Sep 17 00:00:00 2001 From: "kyle.cao" Date: Tue, 13 Jul 2021 15:52:06 +0800 Subject: [PATCH] Fix filter degradation (#1240) Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com> Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com> --- src/planner/match/WhereClausePlanner.cpp | 2 +- src/planner/match/WhereClausePlanner.h | 7 +- src/planner/match/WithClausePlanner.cpp | 4 +- src/planner/plan/Query.cpp | 1 + tests/tck/features/match/With.feature | 81 ++++++++++++++---------- 5 files changed, 57 insertions(+), 38 deletions(-) diff --git a/src/planner/match/WhereClausePlanner.cpp b/src/planner/match/WhereClausePlanner.cpp index 71f98bf90..cb09ec1f2 100644 --- a/src/planner/match/WhereClausePlanner.cpp +++ b/src/planner/match/WhereClausePlanner.cpp @@ -21,7 +21,7 @@ StatusOr WhereClausePlanner::transform(CypherClauseContextBase* ctx) { if (wctx->filter) { SubPlan wherePlan; auto* newFilter = MatchSolver::doRewrite(wctx->qctx, *wctx->aliasesUsed, wctx->filter); - wherePlan.root = Filter::make(wctx->qctx, nullptr, newFilter, true); + wherePlan.root = Filter::make(wctx->qctx, nullptr, newFilter, needStableFilter_); wherePlan.tail = wherePlan.root; return wherePlan; diff --git a/src/planner/match/WhereClausePlanner.h b/src/planner/match/WhereClausePlanner.h index e4d5913ca..82a14fafb 100644 --- a/src/planner/match/WhereClausePlanner.h +++ b/src/planner/match/WhereClausePlanner.h @@ -16,9 +16,14 @@ namespace graph { */ class WhereClausePlanner final : public CypherClausePlanner { public: - WhereClausePlanner() = default; + explicit WhereClausePlanner(bool needStableFilter = false) + : needStableFilter_(needStableFilter) {} StatusOr transform(CypherClauseContextBase* clauseCtx) override; + +private: + // `needStableFilter_=true` only if there is orderBy in withClause(to avoid unstableErase) + bool needStableFilter_{false}; }; } // namespace graph } // namespace nebula diff --git a/src/planner/match/WithClausePlanner.cpp b/src/planner/match/WithClausePlanner.cpp index 0d796519d..83ce623e4 100644 --- a/src/planner/match/WithClausePlanner.cpp +++ b/src/planner/match/WithClausePlanner.cpp @@ -53,7 +53,9 @@ Status WithClausePlanner::buildWith(WithClauseContext* wctx, SubPlan& subPlan) { } if (wctx->where != nullptr) { - auto wherePlan = std::make_unique()->transform(wctx->where.get()); + bool needStableFilter = wctx->order != nullptr; + auto wherePlan = + std::make_unique(needStableFilter)->transform(wctx->where.get()); NG_RETURN_IF_ERROR(wherePlan); auto plan = std::move(wherePlan).value(); SegmentsConnector::addInput(plan.tail, subPlan.root, true); diff --git a/src/planner/plan/Query.cpp b/src/planner/plan/Query.cpp index 4004eafb7..00e59bdf9 100644 --- a/src/planner/plan/Query.cpp +++ b/src/planner/plan/Query.cpp @@ -203,6 +203,7 @@ Filter::Filter(QueryContext* qctx, PlanNode* input, Expression* condition, bool std::unique_ptr Filter::explain() const { auto desc = SingleInputNode::explain(); addDescription("condition", condition_ ? condition_->toString() : "", desc.get()); + addDescription("isStable", needStableFilter_ ? "true" : "false", desc.get()); return desc; } diff --git a/tests/tck/features/match/With.feature b/tests/tck/features/match/With.feature index 1d90ee180..f2b33aac9 100644 --- a/tests/tck/features/match/With.feature +++ b/tests/tck/features/match/With.feature @@ -72,7 +72,7 @@ Feature: With clause RETURN count(names) """ Then the result should be, in any order, with relax comparison: - | COUNT(names) | + | count(names) | | 191 | When executing query: """ @@ -82,8 +82,52 @@ Feature: With clause RETURN collect(names) """ Then the result should be, in any order, with relax comparison: - | COLLECT(names) | + | collect(names) | | ["Tony Parker", "Tiago Splitter", "Spurs", "Shaquile O'Neal", "Marco Belinelli"] | + When profiling query: + """ + MATCH (v:player) + WITH v.age AS age, v AS v, v.name AS name + ORDER BY age DESCENDING, name ASCENDING + LIMIT 20 + WHERE age > 30 + RETURN v, age + """ + Then the result should be, in order, with relax comparison: + | v | age | + | ("Shaquile O'Neal" :player{age: 47, name: "Shaquile O'Neal"}) | 47 | + | ("Grant Hill" :player{age: 46, name: "Grant Hill"}) | 46 | + | ("Jason Kidd" :player{age: 45, name: "Jason Kidd"}) | 45 | + | ("Steve Nash" :player{age: 45, name: "Steve Nash"}) | 45 | + | ("Ray Allen" :player{age: 43, name: "Ray Allen"}) | 43 | + | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | 42 | + | ("Vince Carter" :player{age: 42, name: "Vince Carter"}) | 42 | + | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | 41 | + | ("Dirk Nowitzki" :player{age: 40, name: "Dirk Nowitzki"}) | 40 | + | ("Kobe Bryant" :player{age: 40, name: "Kobe Bryant"}) | 40 | + | ("Tracy McGrady" :player{age: 39, name: "Tracy McGrady"}) | 39 | + | ("David West" :player{age: 38, name: "David West"}) | 38 | + | ("Paul Gasol" :player{age: 38, name: "Paul Gasol"}) | 38 | + | ("Yao Ming" :player{age: 38, name: "Yao Ming"}) | 38 | + | ("Dwyane Wade" :player{age: 37, name: "Dwyane Wade"}) | 37 | + | ("Amar'e Stoudemire" :player{age: 36, name: "Amar'e Stoudemire"}) | 36 | + | ("Boris Diaw" :player{age: 36, name: "Boris Diaw"}) | 36 | + | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | 36 | + | ("Carmelo Anthony" :player{age: 34, name: "Carmelo Anthony"}) | 34 | + | ("LeBron James" :player{age: 34, name: "LeBron James"}) | 34 | + And the execution plan should be: + | id | name | dependencies | operator info | + | 13 | Project | 12 | | + | 12 | Filter | 17 | {"isStable": "true"} | + | 17 | TopN | 9 | | + | 9 | Project | 8 | | + | 8 | Filter | 7 | {"isStable": "false"} | + | 7 | Project | 6 | | + | 6 | Project | 5 | | + | 5 | Filter | 16 | {"isStable": "false"} | + | 16 | GetVertices | 1 | | + | 1 | IndexScan | 0 | | + | 0 | Start | | | @skip Scenario: with match return @@ -122,39 +166,6 @@ Feature: With clause | "hello" | 2 | 2 | | "hello" | 3 | 3 | - Scenario: match with return - When executing query: - """ - MATCH (v:player) - WITH v.age AS age, v AS v, v.name AS name - ORDER BY age DESCENDING, name ASCENDING - LIMIT 20 - WHERE age > 30 - RETURN v, age - """ - Then the result should be, in order, with relax comparison: - | v | age | - | ("Shaquile O'Neal" :player{age: 47, name: "Shaquile O'Neal"}) | 47 | - | ("Grant Hill" :player{age: 46, name: "Grant Hill"}) | 46 | - | ("Jason Kidd" :player{age: 45, name: "Jason Kidd"}) | 45 | - | ("Steve Nash" :player{age: 45, name: "Steve Nash"}) | 45 | - | ("Ray Allen" :player{age: 43, name: "Ray Allen"}) | 43 | - | ("Tim Duncan" :bachelor{name: "Tim Duncan", speciality: "psychology"} :player{age: 42, name: "Tim Duncan"}) | 42 | - | ("Vince Carter" :player{age: 42, name: "Vince Carter"}) | 42 | - | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) | 41 | - | ("Dirk Nowitzki" :player{age: 40, name: "Dirk Nowitzki"}) | 40 | - | ("Kobe Bryant" :player{age: 40, name: "Kobe Bryant"}) | 40 | - | ("Tracy McGrady" :player{age: 39, name: "Tracy McGrady"}) | 39 | - | ("David West" :player{age: 38, name: "David West"}) | 38 | - | ("Paul Gasol" :player{age: 38, name: "Paul Gasol"}) | 38 | - | ("Yao Ming" :player{age: 38, name: "Yao Ming"}) | 38 | - | ("Dwyane Wade" :player{age: 37, name: "Dwyane Wade"}) | 37 | - | ("Amar'e Stoudemire" :player{age: 36, name: "Amar'e Stoudemire"}) | 36 | - | ("Boris Diaw" :player{age: 36, name: "Boris Diaw"}) | 36 | - | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | 36 | - | ("Carmelo Anthony" :player{age: 34, name: "Carmelo Anthony"}) | 34 | - | ("LeBron James" :player{age: 34, name: "LeBron James"}) | 34 | - Scenario: with exists When executing query: """