Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

[OptRule] Collapse continuous Project plan node #1282

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

czpmango
Copy link
Contributor

Merge consecutive project plan nodes to avoid memory replication, and this may benefit column pruning in some cases.

example: match (v:player) with v, v.age/10 as age, v.name as name, abs(v.age) as abs return age-1 as res

old plan:
...->Project(v,age,name,abs)->Project(res)

plan applying this rule:
...->Project(res)
(res=v.age/10-1)

@czpmango czpmango added ready-for-testing PR: ready for the CI test wip Solution: work in progress labels Jul 22, 2021
@czpmango czpmango requested a review from a team July 23, 2021 02:41
@czpmango czpmango removed the wip Solution: work in progress label Jul 23, 2021
@yixinglu yixinglu requested a review from jievince July 23, 2021 06:10
Comment on lines +35 to +36
| 7 | Project | 6 | |
| 6 | Project | 5 | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could these projects not be collapsed ?

Copy link
Contributor Author

@czpmango czpmango Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid multiple calculations of the expression($-._path referenced twice in the following project node), this situation was disabled.

jievince
jievince previously approved these changes Jul 23, 2021
Copy link
Contributor

@CPWstatic CPWstatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work.

#
# This source code is licensed under Apache 2.0 License,
# attached with Common Clause Condition 1.0, found in the LICENSES directory.
Feature: Push Filter down LeftJoin rule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the feature name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thx~

namespace nebula {
namespace opt {

class CollapseProjectRule final : public OptRule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emmm, a pattern is always not enough to understand the details, so we better add some comment for the opt rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already added.

@Shylock-Hg
Copy link
Contributor

How about the case that the result of collapsed Project may be used by other operator?

@jievince
Copy link
Contributor

How about the case that the result of collapsed Project may be used by other operator?

Is this not checked in checkDataFlowDeps?

@Shylock-Hg
Copy link
Contributor

How about the case that the result of collapsed Project may be used by other operator?

Is this not checked in checkDataFlowDeps?

That seems only check whether data flow same with control flow

@yixinglu yixinglu merged commit c6f702d into vesoft-inc:master Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants