Property pruner #3750

jievince · 2022-01-18T11:36:05Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:
Close #3096.

Description:

How do you solve it?

Collect the attributes used by each plan node from the top down and record them in the PropertyTracker. For a node (AppendVertices, Traverse), they only need to request the storaged attributes recorded in the PropertyTracker (that is, the attributes used by its upper nodes).

Afte property pruner, some plan nodes may need to be deleted.
eg. AppendVertices node could be deleted if all the vertexProps it requests are not used by the upper nodes.

PropertyTrackerVisitor is used to track the vertex/edge props used by the expressions of plan nodes.
Currently, we need to care about TagPropertyExpression and LabelTagProertyExpression for vertex properties, EdgePropertyExpression and AttributeExpression? for edge properties. If these exprs are not found, then InputPropertyExpresson and VariablePropertyExpression are collected.

TODO:

Add a method called markTheNodeAsDeleted, which implies the node is useless after the property pruner and should be deleted.
At present this PR only prunes vertex attributes, when the PR was almost reviewed, I can add the pruner of edge attribute.
Implement ColumnPruner, column pruner should be done before property pruner in the optimizer.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Shylock-Hg · 2022-01-19T02:15:20Z

Any design document about this?

jievince · 2022-01-20T05:35:45Z

Any design document about this?

Sorry, there is no design doc here. Bug after the PR is finished, I will make a brief description.

src/common/expression/Expression.cpp

src/graph/optimizer/Optimizer.cpp

czpmango · 2022-01-21T10:50:30Z

src/graph/optimizer/rule/CollapseProjectRule.cpp

@@ -11,6 +11,8 @@
 #include "graph/planner/plan/Query.h"
 #include "graph/util/ExpressionUtils.h"

+DEFINE_bool(enable_opt_collapse_project_rule, true, "");


Why disable this rule?

It's not disabled, it's just a switch. Actually, all opt rules need a switch.

RBO should not be designed to be a tradeoff. Why need a switch?

For test or debug.

RBO rule doesn't always make the plan better.

src/common/expression/Expression.cpp

czpmango · 2022-01-21T11:07:36Z

src/graph/util/ExpressionUtils.cpp

+Status ExpressionUtils::extractPropsFromExprs(const Expression *expr,
+                                              PropertyTracker &propsUsed,
+                                              const graph::QueryContext *qctx,
+                                              GraphSpaceID spaceID,
+                                              const std::string &entityAlias) {


Define a more generic interface for utility functions.

I don't get it. Could you have a explain?

czpmango · 2022-01-21T11:10:14Z

src/common/expression/Expression.h

@@ -223,6 +225,17 @@ class Expression {

 std::ostream& operator<<(std::ostream& os, Expression::Kind kind);

+struct PropertyTracker {
+  std::unordered_map<std::string, std::unordered_map<TagID, std::unordered_set<std::string>>>


Why use TagID rather than TagName?

Because the plan node such as AppendVertices or Traverse 's data membervertexprops_ store the tagId.

I think you can materialize TagID before you pass it to Traverse or AppendVertices instead of doing it in a visitor, which is debug friendly as well.

I think both are ok, other visitors like DeducePropsVisitor has used tagId:

void DeducePropsVisitor::visit(LabelTagPropertyExpression *expr) { auto status = qctx_->schemaMng()->toTagID(space_, expr->sym());

nevermore3 · 2022-01-26T08:43:54Z

src/graph/visitor/PropertyTrackerVisitor.cpp

+  propsUsed_.colsSet.emplace(colName);
+}
+
+// void PropertyTrackerVisitor::visit(AttributeExpression *expr) {


why disable AttributeExpression

Please see the TODO of the desc of this PR:

At present, this PR only prunes vertex attributes, when the PR was almost reviewed, I can add the pruner of edge attribute.

CPWstatic

The others in this pr are all looks good to me. Please move on it. Look forward to the effect of this pr.

CPWstatic · 2022-01-28T02:12:48Z

src/graph/optimizer/Optimizer.cpp

-  auto rootGroup = std::move(status).value();
+  auto spaceID = qctx->rctx()->session()->space().id;
+
+  // auto status = preprocess(root, qctx, spaceID);


We should figure out the relation between prepare and preprocess.

I think preprocess is operated on a raw plan, while prepare which invoked convertToGroup is to convert the raw plan to a group, which is used in the exploration phase.

The 3 phase of optimizer of TIDB:

// Phase 1: Preprocessing // The target of this phase is to preprocess the plan tree by some heuristic // rules which should always be beneficial, for example Column Pruning. //------------------------------------------------------------------------------ // Phase 2: Exploration //------------------------------------------------------------------------------ // // The target of this phase is to explore all the logically equivalent // expressions by exploring all the equivalent group expressions of each group. //------------------------------------------------------------------------------ // Phase 3: Implementation //------------------------------------------------------------------------------ // // The target of this phase is to search the best physical plan for a Group // which satisfies a certain required physical property.

CPWstatic · 2022-01-28T02:15:18Z

src/common/expression/Expression.h

@@ -223,6 +224,17 @@ class Expression {

 std::ostream& operator<<(std::ostream& os, Expression::Kind kind);

+struct PropertyTracker {


This class should not be part of this file i think

I will find a better place to put it into.

CPWstatic · 2022-01-28T02:16:53Z

src/graph/planner/plan/PlanNode.cpp

@@ -379,6 +379,22 @@ void PlanNode::updateSymbols() {
  }
 }

+Status PlanNode::pruneProperties(PropertyTracker& propsUsed,


I think we could do this by using the visitor pattern.

Yes, you are right. Putting the method pruneProperties to the Plan node is a little intrusive. I will use the visitor pattern.

Shylock-Hg · 2022-01-28T02:25:48Z

tests/tck/features/yield/NoSpaceChosen.feature

@@ -1,3 +1,4 @@
+@wang


What's this?

I will delete it.

Shylock-Hg · 2022-01-28T02:25:58Z

tests/tck/features/match/MultiQueryParts.feature

    # Below scenario is not suppoted for the execution plan has a scan.
-    When executing query:
+    When profiling query:


Why change this?

I was going to add a execution plan check here

Shylock-Hg · 2022-01-28T02:27:17Z

tests/tck/features/match/MultiQueryParts.feature

@@ -26,7 +27,23 @@ Feature: Multi Query Parts
      | "Tim Duncan" | "Boris Diaw"  | "Spurs"      |
      | "Tim Duncan" | "Boris Diaw"  | "Suns"       |
      | "Tim Duncan" | "Boris Diaw"  | "Tim Duncan" |
-    When executing query:
+    # And the execution plan should be:


Why comment these?

There is something wrong in the check mechanism of operator info of tck currently.

jievince · 2022-01-28T05:06:46Z

The others in this pr are all looks good to me. Please move on it. Look forward to the effect of this pr.

Ok, fine!

…r visitor

…o propsused

… props...

…yTrackerVisitor

Shylock-Hg · 2022-02-11T06:16:48Z

src/graph/optimizer/Optimizer.cpp

@@ -20,6 +21,8 @@ using nebula::graph::QueryContext;
 using nebula::graph::Select;
 using nebula::graph::SingleDependencyNode;

+DEFINE_bool(enable_optimizer_property_pruner_rule, true, "");


I think you should reuse enable_optimizer

How could enable_optimizer be able to control whether to enable a single rule?

Now we don't provide a way to control single optimization. And in my mind, enable_optimizer must control all optimizations include this of course.

* property pruner * postprocess works. * add Travese::pruneProperties * make vertexprops of travese be null when node alias is not used * fix extractPropsFromExps for tagPropExpr and edgePropexpr, vFilter and eFilter * add PropertyTracker::update for project node * add DeduceMatchPropsVisitor and FLAGS_enable_opt_collapse_project_rule * add Filter::pruneProperties and only 3 tck cases not passed * Do not do propsUsed.colsSet.erase(it) * revert labeltagpropexpr and pass all tck * add hasAlias, add deduce types for id, src, dst func * rename visitor * ingore func id, src, dst, type, typeid, rank, hash in property tracker visitor * add Aggregate::pruneProperties and store the alias of id/src/dst... to propsused * format tck * remove some unusedless headers * add tck for plan * rename has1, has2, has3 * add flag_enable_optimizer_property_pruner_rule and support prune edge props... * add kUnknownEdgeType * add PrunePropertiesVisitor * remove PlanNode::pruneProperties, and move PropertyTracker to PropertyTrackerVisitor * add PrunePropertiesRule.feature * add markDeleted interface * remove unused code * fix header macro * update tck Co-authored-by: kyle.cao <[email protected]>

jievince added the ready-for-testing PR: ready for the CI test label Jan 18, 2022

jievince force-pushed the property-pruner branch 2 times, most recently from 3f2257b to bddd2a9 Compare January 21, 2022 08:34

czpmango reviewed Jan 21, 2022

View reviewed changes

src/common/expression/Expression.cpp Outdated Show resolved Hide resolved

czpmango reviewed Jan 21, 2022

View reviewed changes

src/graph/optimizer/Optimizer.cpp Show resolved Hide resolved

czpmango reviewed Jan 21, 2022

View reviewed changes

src/common/expression/Expression.cpp Outdated Show resolved Hide resolved

czpmango reviewed Jan 21, 2022

View reviewed changes

jievince force-pushed the property-pruner branch 2 times, most recently from 39703ff to c116258 Compare January 24, 2022 06:19

jievince marked this pull request as ready for review January 24, 2022 06:53

jievince requested review from czpmango, CPWstatic and jackwener January 24, 2022 06:56

jievince changed the title ~~[WIP]Property pruner~~ Property pruner Jan 24, 2022

jievince force-pushed the property-pruner branch from 2acd783 to f898eeb Compare January 24, 2022 07:29

jievince requested a review from Shylock-Hg January 25, 2022 06:12

nevermore3 reviewed Jan 26, 2022

View reviewed changes

jievince requested a review from nevermore3 January 27, 2022 02:02

CPWstatic reviewed Jan 28, 2022

View reviewed changes

Shylock-Hg reviewed Jan 28, 2022

View reviewed changes

jievince force-pushed the property-pruner branch 3 times, most recently from 3d2c42c to b573dcb Compare February 10, 2022 09:57

jievince added 16 commits February 11, 2022 11:41

add hasAlias, add deduce types for id, src, dst func

792d0f1

rename visitor

7d4f9dc

ingore func id, src, dst, type, typeid, rank, hash in property tracke…

93ec6c5

…r visitor

add Aggregate::pruneProperties and store the alias of id/src/dst... t…

bcae49b

…o propsused

format tck

97071d6

remove some unusedless headers

c41e056

add tck for plan

b761996

rename has1, has2, has3

ab9f684

add flag_enable_optimizer_property_pruner_rule and support prune edge…

cde0eb3

… props...

add kUnknownEdgeType

cfc0275

add PrunePropertiesVisitor

72b0c22

remove PlanNode::pruneProperties, and move PropertyTracker to Propert…

f60ca32

…yTrackerVisitor

add PrunePropertiesRule.feature

4f80bc0

add markDeleted interface

b8a96af

remove unused code

cb33f65

fix header macro

679ff67

jievince dismissed czpmango’s stale review via 679ff67 February 11, 2022 03:42

jievince force-pushed the property-pruner branch from dcb0ab1 to 679ff67 Compare February 11, 2022 03:42

jievince requested a review from yixinglu February 11, 2022 05:03

jievince force-pushed the property-pruner branch from 3da2537 to dbc07ba Compare February 11, 2022 05:10

update tck

1e936ea

jievince force-pushed the property-pruner branch from dbc07ba to 1e936ea Compare February 11, 2022 05:33

czpmango approved these changes Feb 11, 2022

View reviewed changes

yixinglu approved these changes Feb 11, 2022

View reviewed changes

Shylock-Hg reviewed Feb 11, 2022

View reviewed changes

Merge branch 'master' into property-pruner

a04dc00

yixinglu merged commit 25a0621 into vesoft-inc:master Feb 11, 2022

jamieliu1023 mentioned this pull request Feb 12, 2022

Weekly Report 2022-02-11 vesoft-inc/nebula-community#89

Closed

czpmango mentioned this pull request Feb 23, 2022

[Optimizer] enhance ColumnPruning #3931

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Property pruner #3750

Property pruner #3750

jievince commented Jan 18, 2022 •

edited

Loading

Shylock-Hg commented Jan 19, 2022

jievince commented Jan 20, 2022

czpmango Jan 21, 2022

jievince Jan 23, 2022

czpmango Jan 24, 2022

jievince Jan 24, 2022

czpmango Jan 21, 2022

jievince Jan 23, 2022

czpmango Jan 21, 2022

jievince Jan 23, 2022

czpmango Jan 24, 2022

jievince Jan 24, 2022

nevermore3 Jan 26, 2022

jievince Jan 26, 2022 •

edited

Loading

CPWstatic left a comment

CPWstatic Jan 28, 2022

jievince Jan 28, 2022

CPWstatic Jan 28, 2022

jievince Jan 28, 2022

CPWstatic Jan 28, 2022

jievince Jan 28, 2022

Shylock-Hg Jan 28, 2022

jievince Jan 28, 2022

Shylock-Hg Jan 28, 2022

jievince Jan 28, 2022

Shylock-Hg Jan 28, 2022

jievince Jan 28, 2022

jievince commented Jan 28, 2022 •

edited

Loading

Shylock-Hg Feb 11, 2022

jievince Feb 11, 2022

Shylock-Hg Feb 11, 2022 •

edited

Loading

		@@ -223,6 +224,17 @@ class Expression {

		std::ostream& operator<<(std::ostream& os, Expression::Kind kind);

		struct PropertyTracker {

Property pruner #3750

Property pruner #3750

Conversation

jievince commented Jan 18, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Shylock-Hg commented Jan 19, 2022

jievince commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jievince Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

CPWstatic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jievince commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shylock-Hg Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

jievince commented Jan 18, 2022 •

edited

Loading

jievince Jan 26, 2022 •

edited

Loading

jievince commented Jan 28, 2022 •

edited

Loading

Shylock-Hg Feb 11, 2022 •

edited

Loading