Skip to content
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.

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

feat(optimizer): Implement ProjectJoin rule #4385

Merged
merged 5 commits into from
Aug 3, 2022
Merged

feat(optimizer): Implement ProjectJoin rule #4385

merged 5 commits into from
Aug 3, 2022

Conversation

jon-chuang
Copy link
Contributor

@jon-chuang jon-chuang commented Aug 3, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Merge projects into joins's output indices where project's expressions are all InputRefs

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #4385 (a10b86a) into main (072b6e0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4385   +/-   ##
=======================================
  Coverage   74.48%   74.48%           
=======================================
  Files         846      846           
  Lines      123826   123832    +6     
=======================================
+ Hits        92226    92239   +13     
+ Misses      31600    31593    -7     
Flag Coverage Δ
rust 74.48% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/optimizer/plan_node/logical_expand.rs 94.67% <ø> (-0.04%) ⬇️
...c/frontend/src/optimizer/plan_node/logical_join.rs 90.55% <ø> (+0.02%) ⬆️
src/frontend/src/optimizer/mod.rs 91.16% <100.00%> (+0.08%) ⬆️
src/frontend/src/optimizer/rule/project_join.rs 100.00% <100.00%> (+100.00%) ⬆️
src/meta/src/manager/id.rs 95.10% <0.00%> (+0.54%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@chenzl25
Copy link
Contributor

chenzl25 commented Aug 3, 2022

IIRC, physical optimization likes toBatch or toStream always create project for LogicalJoin with a non-trivial output_indices. Can we benefit from this rule?

@st1page
Copy link
Contributor

st1page commented Aug 3, 2022

IIRC, physical optimization likes toBatch or toStream always create project for LogicalJoin with a non-trivial output_indices. Can we benefit from this rule?

IIRC, we have support output_indices in executor, what the non-trivial means

@chenzl25
Copy link
Contributor

chenzl25 commented Aug 3, 2022

In this case trivial means identity.

@chenzl25
Copy link
Contributor

chenzl25 commented Aug 3, 2022

In this case trivial means identity.

Oh, I just only focus on the pull_filter part which always create a project. No question Now.

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

related issue #1922

Will we also need to similarly merge simple project into output indices of other nodes? 🤔

@jon-chuang
Copy link
Contributor Author

jon-chuang commented Aug 3, 2022

Will we also need to similarly merge simple project into output indices of other nodes?

Possibly. One of these is scan. I'm not sure if its necessary to do this as it may already be facilitated by prune_cols?

Anw, we can open an issue on this.

@mergify mergify bot merged commit ba66d65 into main Aug 3, 2022
@mergify mergify bot deleted the jon/project-join branch August 3, 2022 08:58
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants