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(stream): add append_only in proto stream node #2020

Merged
merged 5 commits into from
Apr 21, 2022

Conversation

st1page
Copy link
Contributor

@st1page st1page commented Apr 21, 2022

What's changed and what's your intention?

add append_only in proto stream node

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)

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2020 (384efbc) into main (579f845) will increase coverage by 0.00%.
The diff coverage is 55.00%.

@@           Coverage Diff           @@
##             main    #2020   +/-   ##
=======================================
  Coverage   70.65%   70.65%           
=======================================
  Files         629      629           
  Lines       80967    80987   +20     
=======================================
+ Hits        57208    57223   +15     
- Misses      23759    23764    +5     
Flag Coverage Δ
rust 70.65% <55.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/meta/src/stream/fragmenter.rs 84.84% <0.00%> (-0.29%) ⬇️
src/meta/src/stream/rewrite/delta_join.rs 0.00% <0.00%> (ø)
src/meta/src/stream/graph/stream_graph.rs 59.95% <33.33%> (-0.18%) ⬇️
src/frontend/src/optimizer/plan_node/mod.rs 98.01% <100.00%> (+0.01%) ⬆️
...ntend/src/optimizer/plan_node/stream_table_scan.rs 94.91% <100.00%> (+0.13%) ⬆️
src/meta/src/stream/test_fragmenter.rs 99.63% <100.00%> (+<0.01%) ⬆️
src/frontend/src/optimizer/plan_node/plan_base.rs 95.83% <0.00%> (+4.16%) ⬆️

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

@st1page st1page requested a review from skyzh April 21, 2022 08:02
Copy link
Contributor

@TennyZhuang TennyZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@@ -581,6 +582,7 @@ where
fields: chain_node.upstream_fields.clone(),
operator_id: merge_node.operator_id,
identity: "MergeExecutor".to_string(),
append_only: stream_node.append_only,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we determine append_only of upstream node solely from the StreamTableScan's append_only property?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that append_only of StreamTableScan is always false... So I think we can follow the append_only property given by frontend for now. If there's issue after we implement the full append-only inference, let's fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I think the information should be in the catalog of the mv...

@@ -164,6 +165,7 @@ impl StreamTableScan {
} else {
"".into()
},
append_only: false, // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

should be self.append_only?

@@ -127,6 +127,7 @@ impl StreamTableScan {
pk_indices: pk_indices.clone(),
input: vec![],
fields: vec![], // TODO: fill this later
append_only: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

For L117 (MergeNode), also should be self.append_only?

@st1page st1page enabled auto-merge (squash) April 21, 2022 09:57
@st1page st1page merged commit 0c831db into main Apr 21, 2022
@st1page st1page deleted the sts/proto_stream_node_append_only branch April 21, 2022 10:13
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