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(frontend): add StreamProjectMaterialize PlanNode #8843

Closed
wants to merge 2 commits into from

Conversation

jon-chuang
Copy link
Contributor

@jon-chuang jon-chuang commented Mar 29, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Add StreamProjectMaterialize PlanNode

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

  • My PR DOES NOT contain user-facing changes.

References

risingwavelabs/rfcs#24

@jon-chuang jon-chuang requested review from st1page and xxchan and removed request for st1page March 29, 2023 02:34
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #8843 (6fda714) into main (81b306b) will decrease coverage by 0.07%.
The diff coverage is 1.36%.

@@            Coverage Diff             @@
##             main    #8843      +/-   ##
==========================================
- Coverage   70.82%   70.75%   -0.07%     
==========================================
  Files        1172     1173       +1     
  Lines      194486   194618     +132     
==========================================
- Hits       137736   137700      -36     
- Misses      56750    56918     +168     
Flag Coverage Δ
rust 70.75% <1.36%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
src/frontend/src/optimizer/plan_node/mod.rs 92.99% <ø> (ø)
...optimizer/plan_node/stream_project_materialized.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/mod.rs 0.00% <ø> (ø)
...rontend/src/optimizer/plan_node/logical_project.rs 97.76% <100.00%> (-0.11%) ⬇️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -270,23 +270,8 @@ impl ToStream for LogicalProject {
let new_input = self
.input()
.to_stream_with_dist_required(&input_required, ctx)?;
let new_logical = self.clone_with_input(new_input.clone());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for review: we already do project merge rule

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.

Hmmm, although there aren't any objections, I think we should mark risingwavelabs/rfcs#24 as accepted first to make sure consensus is reached.

@jon-chuang
Copy link
Contributor Author

jon-chuang commented Mar 30, 2023

as accepted first to make sure consensus is reached.

Agreed on waiting, I decided to get the ball rolling as I think the consensus is more or less in this direction - hoping to get it sooner as I'd like this feature for my demo :)

@st1page
Copy link
Contributor

st1page commented Mar 30, 2023

as accepted first to make sure consensus is reached.

Agreed on waiting, I decided to get the ball rolling as I think the consensus is more or less in this direction - hoping to get it sooner as I'd like this feature for my demo :)

I agree that this PR can achieve that with minimal invasiveness and easy to revert it... So I think I will be glad to approve it if it is really urgent. But at least, I think we need to add warnings to users "it is an experimental feature" when we write a UDF on an updatable stream.

@jon-chuang
Copy link
Contributor Author

But at least, I think we need to add warnings to users "it is an experimental feature" when we write a UDF on an updatable stream.

Agreed.

@github-actions
Copy link
Contributor

This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review.

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.

3 participants