Skip to content

feat(optimizer): plan node base #748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 19 commits into from
Mar 9, 2022
Merged

feat(optimizer): plan node base #748

merged 19 commits into from
Mar 9, 2022

Conversation

@st1page
Copy link
Contributor

@st1page st1page commented Mar 8, 2022 •
edited
Loading

What's changed and what's your intention?

extract the common field of the nodes
main changes at the plan_node/mod.rs

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@st1page st1page requested review from fuyufjh, liurenjie1024 and xxchan and removed request for xxchan March 8, 2022 10:02
fuyufjh
fuyufjh approved these changes Mar 8, 2022
View reviewed changes
Copy link
Collaborator

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM for the rest

rust/frontend/src/optimizer/plan_node/mod.rs Outdated
Copy link
Collaborator

@fuyufjh fuyufjh Mar 8, 2022

Choose a reason for hiding this comment

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

The base field in logical nodes are pub so this is not necessary?

Copy link
Contributor Author

@st1page st1page Mar 8, 2022

Choose a reason for hiding this comment

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

actually I do not want it to be pub..

Copy link
Contributor Author

@st1page st1page Mar 9, 2022

Choose a reason for hiding this comment

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

yes, you are right and I have fixed it.

@st1page st1page requested a review from BowenXiao1999 March 8, 2022 12:41
liurenjie1024
liurenjie1024 reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM

BowenXiao1999
BowenXiao1999 approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@BowenXiao1999 BowenXiao1999 left a comment

Choose a reason for hiding this comment

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

LGTM

rust/frontend/src/optimizer/plan_node/mod.rs Outdated
Copy link
Contributor

@BowenXiao1999 BowenXiao1999 Mar 9, 2022

Choose a reason for hiding this comment

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

WIll the assign include in this PR?

Copy link
Contributor Author

@st1page st1page Mar 9, 2022

Choose a reason for hiding this comment

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

in future

@st1page st1page force-pushed the sts/optimizer_plan_node_base branch from dc73731 to 507984e Compare March 9, 2022 03:20
@st1page st1page marked this pull request as ready for review March 9, 2022 03:27
@codecov
Copy link

codecov bot commented Mar 9, 2022 •
edited
Loading

Codecov Report

Merging #748 (708dd0b) into main (7d220e4) will decrease coverage by 0.00%.
The diff coverage is 57.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #748      +/-   ##
============================================
- Coverage     72.36%   72.36%   -0.01%     
  Complexity     2759     2759              
============================================
  Files           915      915              
  Lines         53042    53050       +8     
  Branches       1783     1783              
============================================
+ Hits          38384    38389       +5     
- Misses        13766    13769       +3     
  Partials        892      892
Flag Coverage Δ
java 61.17% <ø> (ø)
rust 77.08% <57.44%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...st/frontend/src/optimizer/plan_node/batch_limit.rs 0.00% <0.00%> (ø)
.../frontend/src/optimizer/plan_node/batch_project.rs 0.00% <0.00%> (ø)
...ust/frontend/src/optimizer/plan_node/batch_sort.rs 0.00% <0.00%> (ø)
...d/src/optimizer/plan_node/batch_sort_merge_join.rs 0.00% <0.00%> (ø)
.../frontend/src/optimizer/plan_node/logical_limit.rs 0.00% <0.00%> (ø)
...t/frontend/src/optimizer/plan_node/logical_topn.rs 0.00% <0.00%> (ø)
rust/frontend/src/optimizer/plan_node/mod.rs 29.54% <ø> (ø)
...rontend/src/optimizer/plan_node/stream_exchange.rs 0.00% <0.00%> (ø)
...ontend/src/optimizer/plan_node/stream_hash_join.rs 0.00% <0.00%> (ø)
...frontend/src/optimizer/plan_node/stream_project.rs 0.00% <0.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d220e4...708dd0b. Read the comment docs.

@st1page st1page changed the title WIP feat(optimizer): plan node base feat(optimizer): plan node base Mar 9, 2022
@github-actions github-actions bot added the type/feature Type: New feature. label Mar 9, 2022
@st1page st1page merged commit ebb8519 into main Mar 9, 2022
@st1page st1page deleted the sts/optimizer_plan_node_base branch March 9, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuyufjh fuyufjh fuyufjh approved these changes

@xxchan xxchan xxchan approved these changes

+2 more reviewers

@liurenjie1024 liurenjie1024 liurenjie1024 left review comments

@BowenXiao1999 BowenXiao1999 BowenXiao1999 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type/feature Type: New feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@st1page @liurenjie1024 @fuyufjh @BowenXiao1999 @xxchan