Prune properties #4523

nevermore3 · 2022-08-15T11:02:06Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

1、attributes are collected from the root node, and the attributes required by each node are the union of itself and the parent node
2、when encountering UNION and BicaresionProduct operators, the left and right branches collect attributes independently Except for the join operator
3、when the vertexProp of appendVertices is null, it can be returned directly without going through RPC
4、delete _src from edge properties， instead of VID

How do you solve it?

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

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

src/graph/context/Iterator.cpp

Shylock-Hg · 2022-08-17T01:04:37Z

src/graph/executor/query/AppendVerticesExecutor.cpp

  auto *av = asNode<AppendVertices>(node());
-  StorageClient *storageClient = qctx()->getStorageClient();
+  if (FLAGS_optimize_appendvertices && av != nullptr && av->props() == nullptr) {


This will lead error if dangle edges presents.

will be turned on when testing， default is false

If so, I think you could eliminate AppendVertices operator in optimizer.

We don't have test cases for dangling edges now. If this operator returns directly, the behavior is different from before.

I also agree with @Shylock-Hg that if property pruning is a deterministic optimization, isn't it better practice to handle it in the optimizer?

there will be a problem of dangle edges. Currently it is manually controlled, and it is usually turned off. It is only turned on when the LDBC test is performed

tests/tck/features/optimizer/PrunePropertiesRule.feature

src/graph/planner/ngql/PathPlanner.cpp

src/graph/visitor/PropertyTrackerVisitor.cpp

czpmango · 2022-08-18T06:17:13Z

nebula/src/graph/optimizer/rule/EliminateAppendVerticesRule.cpp

Lines 53 to 55 in 6317917

    
           if (!graph::AnonVarGenerator::isAnnoVar(appendVertices->colNames().back())) {  // Anonymous node 
        
             return false; 
        
           }

I found that there is still room for optimization in these lines of code. If you are optimizing similar statements MATCH (v:player)-[]-(n) RETUNR v.player.name, it may help.

Shylock-Hg · 2022-08-18T06:22:59Z

nebula/src/graph/optimizer/rule/EliminateAppendVerticesRule.cpp

Lines 53 to 55 in 6317917

if (!graph::AnonVarGenerator::isAnnoVar(appendVertices->colNames().back())) { // Anonymous node

return false;

}

I found that there is still room for optimization in these lines of code. If you are optimizing similar statements MATCH (v:player)-[]-(n) RETUNR v.player.name, it may help.

Yes, could prune properties before optimizer.

Shylock-Hg · 2022-08-18T08:23:53Z

src/graph/executor/query/AppendVerticesExecutor.cpp

@@ -69,6 +71,39 @@ folly::Future<Status> AppendVerticesExecutor::appendVertices() {
      });
 }

+Status AppendVerticesExecutor::handleNullProp(const AppendVertices *av) {


You could eliminate this operator in optimizer based on prune properties before optimizer rules.

jievince

Good job

dutor

Well done.

codecov-commenter · 2022-08-26T11:09:01Z

Codecov Report

Merging #4523 (a52fcbf) into master (c884477) will increase coverage by 0.05%.
The diff coverage is 89.12%.

@@            Coverage Diff             @@
##           master    #4523      +/-   ##
==========================================
+ Coverage   84.66%   84.71%   +0.05%     
==========================================
  Files        1357     1357              
  Lines      135081   135283     +202     
==========================================
+ Hits       114360   114603     +243     
+ Misses      20721    20680      -41

Impacted Files	Coverage Δ
src/graph/executor/algo/BFSShortestPathExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/algo/BatchShortestPath.h	`100.00% <ø> (ø)`
...rc/graph/executor/algo/MultiShortestPathExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/algo/ProduceAllPathsExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/algo/ShortestPathBase.h	`50.00% <ø> (ø)`
src/graph/executor/algo/ShortestPathExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/algo/SingleShortestPath.h	`100.00% <ø> (ø)`
src/graph/executor/algo/SubgraphExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/query/AppendVerticesExecutor.h	`100.00% <ø> (ø)`
src/graph/planner/match/MatchSolver.cpp	`75.84% <ø> (+17.40%)`	⬆️
... and 80 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

* fix lookup (#4552) fix Co-authored-by: jimingquan <mingquan.ji@vesoft.com> Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * fix split brain in raft (#4479) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * fix invalid filter in GetProp make storage crashed (#4568) Co-authored-by: haowen <19355821+wenhaocs@users.noreply.github.com> * fix scan vertex/edge do not handle ttl (#4578) * fix scan vertex/edge do not handle ttl * use ErrorCode to unify community version and end version * Fix #1212. Return FoldConstantExprVisitor, if status_ already failed due to found syantax errors. (#4607) Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com> * Avoid fatal when expression illegal. (#4618) * Fix concurrent exception related to multi-match statement (#4605) * fix filter executor * Fix concurrency exception of multi-match statements fix iterator fix small delete small delete skip iterator type handle for concurrency small delete fix scan edges small delete small delete fix small delete small change small change fix ut small fix Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Prune properties(#4523) * fix conflict * extract attribute from properties function (#4604) * extract attribute from properties function * fix error * fix subscript error * add test case * process scanEdges * fix test error * add unwind & check vidType when executing not validate (#4456) * Update AppendVerticesExecutor.cpp fix conflict * Update AppendVerticesExecutor.cpp * Replace obsolete RocksDB API (#4395) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Update PrunePropertiesRule.feature * remove useless dc (#4533) * Update PrunePropertiesRule.feature * fix test error Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: jimingquan <mingquan.ji@vesoft.com> Co-authored-by: liwenhui-soul <38217397+liwenhui-soul@users.noreply.github.com> Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> Co-authored-by: haowen <19355821+wenhaocs@users.noreply.github.com> Co-authored-by: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com> Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com> Co-authored-by: Qiaolin Yu <90088090+Qiaolin-Yu@users.noreply.github.com>

czpmango · 2022-11-16T03:07:50Z

src/graph/visitor/PropertyTrackerVisitor.cpp

-  static const std::unordered_set<std::string> kEdgeIgnoreFuncs = {
-      "src", "dst", "type", "typeid", "rank"};
+  static const std::unordered_set<std::string> ignoreFuncs = {
+      "src", "dst", "type", "typeid", "id", "rank", "length"};


That is a bad implementation and bad fix.
We should not ignore the inner expression behavior. It's really ambiguous.

nevermore3 force-pushed the prune_properties branch 4 times, most recently from a754c35 to f77bff6 Compare August 16, 2022 11:19

nevermore3 requested review from Shylock-Hg, jievince and Aiee August 16, 2022 11:19

nevermore3 added type/enhancement Type: make the code neat or more efficient ready-for-testing PR: ready for the CI test priority/hi-pri Priority: high ready for review labels Aug 16, 2022

nevermore3 added this to the v3.3.0 milestone Aug 16, 2022

nevermore3 requested a review from dutor August 16, 2022 11:26

nevermore3 added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Aug 16, 2022

nevermore3 force-pushed the prune_properties branch from 25c394a to e5b3388 Compare August 16, 2022 16:02

Shylock-Hg reviewed Aug 17, 2022

View reviewed changes

src/graph/context/Iterator.cpp Show resolved Hide resolved

Shylock-Hg reviewed Aug 17, 2022

View reviewed changes

tests/tck/features/optimizer/PrunePropertiesRule.feature Show resolved Hide resolved

Shylock-Hg reviewed Aug 17, 2022

View reviewed changes

src/graph/planner/ngql/PathPlanner.cpp Show resolved Hide resolved

czpmango reviewed Aug 17, 2022

View reviewed changes

src/graph/visitor/PropertyTrackerVisitor.cpp Outdated Show resolved Hide resolved

nevermore3 force-pushed the prune_properties branch from e5b3388 to 0738158 Compare August 17, 2022 16:19

nevermore3 added 6 commits August 18, 2022 14:53

delete useless code

3552319

support getProp only return vid & delete _src from edge

e0c8310

fix prune properties

652dd7e

enhance edge"

d1b581e

fix pathBuildExpr

b7566e3

fix union error

4d5c9d5

nevermore3 added 8 commits August 18, 2022 14:53

fix unwind

ba2c8fb

fix bicartesionproduct

4355f71

add optimize appendvertice flag

de49271

fix test

896b032

fix format

8b65e39

add test

b754593

address comment

89b850f

fix aggreate

16ff6e1

nevermore3 force-pushed the prune_properties branch from 6e9db6d to 7a98297 Compare August 18, 2022 06:57

fix conflict

f7e0495

nevermore3 force-pushed the prune_properties branch from a9f63c3 to f7e0495 Compare August 18, 2022 07:25

Merge branch 'master' into prune_properties

b6cac66

Shylock-Hg reviewed Aug 18, 2022

View reviewed changes

Merge branch 'master' into prune_properties

58c291b

jievince approved these changes Aug 26, 2022

View reviewed changes

Shylock-Hg approved these changes Aug 26, 2022

View reviewed changes

Merge branch 'master' into prune_properties

a52fcbf

dutor approved these changes Aug 26, 2022

View reviewed changes

Sophie-Xie merged commit decd853 into vesoft-inc:master Aug 26, 2022

nevermore3 deleted the prune_properties branch August 26, 2022 14:34

wey-gu mentioned this pull request Aug 27, 2022

Weekly Report 2022-08-26 vesoft-inc/nebula-community#129

Closed

nevermore3 restored the prune_properties branch September 8, 2022 03:48

Sophie-Xie pushed a commit that referenced this pull request Sep 8, 2022

Prune properties(#4523)

3c2b86a

Sophie-Xie added the affects/v3.2 PR/issue: this bug affects v3.2.x version. label Sep 8, 2022

Sophie-Xie added cherry-pick-v3.2 PR: need cherry-pick to this version and removed affects/v3.2 PR/issue: this bug affects v3.2.x version. labels Sep 15, 2022

nevermore3 deleted the prune_properties branch October 9, 2022 11:27

czpmango reviewed Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune properties #4523

Prune properties #4523

nevermore3 commented Aug 15, 2022 •

edited

Loading

Shylock-Hg Aug 17, 2022

nevermore3 Aug 17, 2022 •

edited

Loading

Shylock-Hg Aug 17, 2022

czpmango Aug 17, 2022

czpmango Aug 17, 2022 •

edited

Loading

nevermore3 Aug 17, 2022

czpmango commented Aug 18, 2022

Shylock-Hg commented Aug 18, 2022

Shylock-Hg Aug 18, 2022

jievince left a comment

dutor left a comment

codecov-commenter commented Aug 26, 2022

czpmango Nov 16, 2022

Prune properties #4523

Prune properties #4523

Conversation

nevermore3 commented Aug 15, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

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

Checklist:

Release notes:

Shylock-Hg Aug 17, 2022

Choose a reason for hiding this comment

nevermore3 Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Shylock-Hg Aug 17, 2022

Choose a reason for hiding this comment

czpmango Aug 17, 2022

Choose a reason for hiding this comment

czpmango Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

nevermore3 Aug 17, 2022

Choose a reason for hiding this comment

czpmango commented Aug 18, 2022

Shylock-Hg commented Aug 18, 2022

Shylock-Hg Aug 18, 2022

Choose a reason for hiding this comment

jievince left a comment

Choose a reason for hiding this comment

dutor left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 26, 2022

Codecov Report

czpmango Nov 16, 2022

Choose a reason for hiding this comment

nevermore3 commented Aug 15, 2022 •

edited

Loading

nevermore3 Aug 17, 2022 •

edited

Loading

czpmango Aug 17, 2022 •

edited

Loading