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(streaming): integrate append-only executors to stream graph #3228

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

StrikeW
Copy link
Contributor

@StrikeW StrikeW commented Jun 15, 2022

What's changed and what's your intention?

Integrate append-only version executors (TopN, HashAgg, HashJoin, SimpleAgg) to the stream graph.

Checklist

  • I have written necessary docs and 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)

close #2407
close #1550

@StrikeW StrikeW requested review from fuyufjh and st1page June 15, 2022 05:07
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #3228 (8f30018) into main (74d8f6e) will decrease coverage by 0.00%.
The diff coverage is 74.50%.

@@            Coverage Diff             @@
##             main    #3228      +/-   ##
==========================================
- Coverage   73.49%   73.48%   -0.01%     
==========================================
  Files         745      745              
  Lines      101959   101947      -12     
==========================================
- Hits        74930    74916      -14     
- Misses      27029    27031       +2     
Flag Coverage Δ
rust 73.48% <74.50%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...tend/src/optimizer/plan_node/stream_materialize.rs 91.05% <ø> (-0.14%) ⬇️
src/stream/src/from_proto/global_simple_agg.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/hash_agg.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/hash_join.rs 0.00% <0.00%> (ø)
src/stream/src/from_proto/local_simple_agg.rs 0.00% <0.00%> (ø)
...rc/frontend/src/optimizer/plan_node/stream_topn.rs 49.15% <53.33%> (-3.68%) ⬇️
...rontend/src/optimizer/plan_node/stream_exchange.rs 94.59% <100.00%> (-0.28%) ⬇️
...rontend/src/optimizer/plan_node/stream_hash_agg.rs 95.16% <100.00%> (+1.41%) ⬆️
...ontend/src/optimizer/plan_node/stream_hash_join.rs 96.22% <100.00%> (ø)
...frontend/src/optimizer/plan_node/stream_project.rs 97.61% <100.00%> (-0.21%) ⬇️
... and 7 more

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

Comment on lines +52 to +56
let mut builder = if self.input().append_only() {
f.debug_struct("StreamAppendOnlyTopN")
} else {
f.debug_struct("StreamTopN")
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks duplicated with "append_only = true/false" in L62~64

(Similar for other operators)

Copy link
Contributor Author

@StrikeW StrikeW Jun 15, 2022

Choose a reason for hiding this comment

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

The append_only attribute in PlanBase indicates whether the output of operator is append-only, it doesn't mean the operator is append-only optimized. So if we want to check whether the append-only version operator is used, we should differentiate it by name (likes the StreamDeltaJoin and StreamHashJoin). cc @st1page

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. In this way, I would recommend to remove "append_only = true/false" in explain results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. I got it wrong before.

@StrikeW StrikeW merged commit 33c717c into main Jun 15, 2022
@StrikeW StrikeW deleted the siyuan/append-only branch June 15, 2022 07:13
@skyzh
Copy link
Contributor

skyzh commented Jun 15, 2022

Still no e2e tests? 🤣 Maybe add next time.

@StrikeW
Copy link
Contributor Author

StrikeW commented Jun 15, 2022

Still no e2e tests? 🤣 Maybe add next time.

Actually, I wrote some e2e tests locally (create append only tables, insert some data, create mv on that table). But I found it is not quite useful. Since non append-only executors can also output the same result. However, now I think it can check that the implementation of append-only executors is correct, I will add a basic test later.

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.

Append-only HashJoin Executor Append-only HashAgg Executor
3 participants