Refactor the process of symbols in optimizer. #4146

Shylock-Hg · 2022-04-13T07:18:31Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

Close #3826

Description:

Add two constraints to optimizer:

One OptGroup will share the same output variable
There is no overlap output variable between OptGroup

And add one implicit transform:

The transformed result of optimizer rule will check dataflow by dependencies relationship.

And some improvement:

PlanNode clone will generate new output variable and use it
PlanNode only needs one output variable so change it from vector to scalar.

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 .....

yixinglu

Good Job! Could this PR solve the optimization issue about match after the rules bug fixed?
if so, we could consider to cherry pick it into v3.1.0.

src/graph/optimizer/OptGroup.cpp

yixinglu · 2022-04-13T07:55:17Z

src/graph/optimizer/OptRule.cpp

+      const OptGroup *dep = deps[i];
+      if (node->inputVar(i) != dep->outputVar()) {
+        return false;
+      }


Maybe we could do more things than this input variable validation, such as check whether these new generated OptGroupNodes reference other nodes outside since we guarantee that all plan nodes of the new subplan couldn't access other plan nodes except the intput/output nodes.

I this has been done in current implementation. All input variables of plan nodes in current transform result had been checed.

Shylock-Hg · 2022-04-13T08:53:35Z

Good Job! Could this PR solve the optimization issue about match after the rules bug fixed? if so, we could consider to cherry pick it into v3.1.0.

Could resolve the case mentioned in #3826

nodes.

…into fix/optimizer-symbols

yixinglu

LGTM

yixinglu · 2022-04-13T10:19:35Z

src/graph/optimizer/OptRule.cpp

+        return false;
+      }
+      // Only use by father plan node
+      if (node->inputVars()[i]->readBy.size() != 1) {


it's better to check whether the input variables are from outside plan. But it's ok to process like this now.

* Refactor the process of symbols in optimizer. * Fix variable shadowing. * Collect boundary from MatchResult directly. * Check variable in TransformResult don't referenced by outside plan nodes. Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Add SetUsageMessage For daemons (#4134) * fix the wrong regex pattern of scientific notation (#4136) * fix updatePart (#4139) * fix updatePart * fix format check Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * show DATA_BALANCE in job list (#4138) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * fix disk fault recovery (#4131) * fix disk fault recovery * add ut Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Refactor findpath (#4095) * optimize bfs * optimizer allpath * optimizer multi-shortestpath * optimizer multi shortest path * fix validate unit test * add some comment * fix error * fix bfs error * add comment * delete conjunct * add findpath unit test * delete useless file * delete log * remove check * multi thread executor * single shortest multi thread * add some testcases * add gflags * fix bfs error * address comment * Just report errorof property pruner in debug mode (#4142) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * remove Atomic Edge (#4127) * try to remove Atomic Edge * remove some space * remove zone Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> * fix show tag/edge index status (#4148) * fix multiple match (#4143) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Refactor the process of symbols in optimizer. (#4146) * Refactor the process of symbols in optimizer. * Fix variable shadowing. * Collect boundary from MatchResult directly. * Check variable in TransformResult don't referenced by outside plan nodes. Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> Co-authored-by: Alex Xing <90179377+SuperYoko@users.noreply.github.com> Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com> Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> Co-authored-by: liwenhui-soul <38217397+liwenhui-soul@users.noreply.github.com> Co-authored-by: panda-sheep <59197347+panda-sheep@users.noreply.github.com> Co-authored-by: jimingquan <mingquan.ji@vesoft.com> Co-authored-by: lionel.liu@vesoft.com <52276794+liuyu85cn@users.noreply.github.com> Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>

OptGroup was refactored in vesoft-inc#4146

delete: av->traverse->scanV add: project-> limit -> av->traverse->sv add: project-> av ->traverse->sv exclude optRule for buildPath in project.colums() Comments added lint: clang-format [WIP] Fixing regression when return e, e.foo.bar The limit info is lost in this case Fix after vesoft-inc#4146 OptGroup was refactored in vesoft-inc#4146 Removed GetEdgesTransformRule

* Stop scanV -> scanE optRule when buildPath delete: av->traverse->scanV add: project-> limit -> av->traverse->sv add: project-> av ->traverse->sv exclude optRule for buildPath in project.colums() Comments added lint: clang-format [WIP] Fixing regression when return e, e.foo.bar The limit info is lost in this case Fix after #4146 OptGroup was refactored in #4146 Removed GetEdgesTransformRule * Rebase on #4277 Remove useless AppendVertices As this rule only aplied for match ()-[e]->() return e limit 1 when 1. there is no index on e and 2. limit cluase exists, thus we could assert that AppendVertices will be removed by EliminateAppendVerticesRule // Tranformation: // Before: // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | Limit | // +---------+---------+ // | // +---------+---------+ // | Traverse | // +---------+---------+ // | // +---------+---------+ // | ScanVertices | // +---------+---------+ // // After: // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | Limit | // +---------+---------+ // | // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | ScanEdges | // +---------+---------+ * Addressing Shylock-Hg's comment * Make linter happy * Fix after EliminateAppendVerticesRule merged and linting * Reusing functions in GetEdgesTransformRule * Addressing comment from Shylock-Hg & rebase Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com>

* Stop scanV -> scanE optRule when buildPath delete: av->traverse->scanV add: project-> limit -> av->traverse->sv add: project-> av ->traverse->sv exclude optRule for buildPath in project.colums() Comments added lint: clang-format [WIP] Fixing regression when return e, e.foo.bar The limit info is lost in this case Fix after vesoft-inc#4146 OptGroup was refactored in vesoft-inc#4146 Removed GetEdgesTransformRule * Rebase on vesoft-inc#4277 Remove useless AppendVertices As this rule only aplied for match ()-[e]->() return e limit 1 when 1. there is no index on e and 2. limit cluase exists, thus we could assert that AppendVertices will be removed by EliminateAppendVerticesRule // Tranformation: // Before: // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | Limit | // +---------+---------+ // | // +---------+---------+ // | Traverse | // +---------+---------+ // | // +---------+---------+ // | ScanVertices | // +---------+---------+ // // After: // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | Limit | // +---------+---------+ // | // +---------+---------+ // | Project | // +---------+---------+ // | // +---------+---------+ // | ScanEdges | // +---------+---------+ * Addressing Shylock-Hg's comment * Make linter happy * Fix after EliminateAppendVerticesRule merged and linting * Reusing functions in GetEdgesTransformRule * Addressing comment from Shylock-Hg & rebase Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com> Co-authored-by: Wey Gu <weyl.gu@gmail.com> Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com>

Refactor the process of symbols in optimizer.

e321285

Shylock-Hg requested review from yixinglu and czpmango April 13, 2022 07:18

Shylock-Hg added type/bug Type: something is unexpected ready-for-testing PR: ready for the CI test labels Apr 13, 2022

Shylock-Hg mentioned this pull request Apr 13, 2022

Fix optimizer symbols. #4106

Closed

11 tasks

yixinglu reviewed Apr 13, 2022

View reviewed changes

Shylock-Hg added 3 commits April 13, 2022 16:02

Fix variable shadowing.

f66362a

Collect boundary from MatchResult directly.

dc8e8a2

Merge branch 'master' into fix/optimizer-symbols

ae995c0

Shylock-Hg requested a review from yixinglu April 13, 2022 08:52

yixinglu added the cherry-pick-v3.1 PR: need cherry-pick to this version label Apr 13, 2022

Shylock-Hg and others added 3 commits April 13, 2022 17:43

Check variable in TransformResult don't referenced by outside plan

27a7afa

nodes.

Merge branch 'fix/optimizer-symbols' of github.com:Shylock-Hg/nebula …

eca12c6

…into fix/optimizer-symbols

Merge branch 'master' into fix/optimizer-symbols

15fd27f

yixinglu approved these changes Apr 13, 2022

View reviewed changes

czpmango approved these changes Apr 13, 2022

View reviewed changes

Merge branch 'master' into fix/optimizer-symbols

5066d67

Sophie-Xie merged commit ad8d661 into vesoft-inc:master Apr 13, 2022

Shylock-Hg deleted the fix/optimizer-symbols branch April 13, 2022 11:41

jamieliu1023 mentioned this pull request Apr 16, 2022

Weekly Report 2022-04-15 vesoft-inc/nebula-community#104

Closed

wey-gu added a commit to wey-gu/nebula that referenced this pull request May 27, 2022

Fix after vesoft-inc#4146

b64ffd7

OptGroup was refactored in vesoft-inc#4146

wey-gu added a commit to wey-gu/nebula that referenced this pull request May 27, 2022

Fix after vesoft-inc#4146

0726413

OptGroup was refactored in vesoft-inc#4146

Shylock-Hg mentioned this pull request Jun 17, 2022

Fix get edges transform rule. #4328

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the process of symbols in optimizer. #4146

Refactor the process of symbols in optimizer. #4146

Shylock-Hg commented Apr 13, 2022 •

edited

Loading

yixinglu left a comment

yixinglu Apr 13, 2022

Shylock-Hg Apr 13, 2022

Shylock-Hg Apr 13, 2022

Shylock-Hg commented Apr 13, 2022

yixinglu left a comment

yixinglu Apr 13, 2022

Refactor the process of symbols in optimizer. #4146

Refactor the process of symbols in optimizer. #4146

Conversation

Shylock-Hg commented Apr 13, 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:

yixinglu left a comment

Choose a reason for hiding this comment

yixinglu Apr 13, 2022

Choose a reason for hiding this comment

Shylock-Hg Apr 13, 2022

Choose a reason for hiding this comment

Shylock-Hg Apr 13, 2022

Choose a reason for hiding this comment

Shylock-Hg commented Apr 13, 2022

yixinglu left a comment

Choose a reason for hiding this comment

yixinglu Apr 13, 2022

Choose a reason for hiding this comment

Shylock-Hg commented Apr 13, 2022 •

edited

Loading