Skip to content

feat(streaming): introduce sort operator based on watermark #6085

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

Merged
merged 14 commits into from
Nov 3, 2022

Conversation

xx01cyx
Copy link
Contributor

@xx01cyx xx01cyx commented Oct 28, 2022

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

What's changed and what's your intention?

Summarize your change

Introduce sort operator, which consumes unordered input and outputs ordered data based on watermark.

How does this PR work? Need a brief introduction for the changed logic

View implementation design doc for basic logics. There's also detailed comments within the code in this PR.

Describe any limitations of the current code

  • The schema is currently the same as its input executor. We should design a schema tailored to sort operator. (In future PR)
  • We don't support range delete currently, so we have to use point delete instead now. (In future PR)

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)

@xx01cyx xx01cyx requested review from BugenZhao and st1page October 28, 2022 01:31
@github-actions github-actions bot added the type/feature Type: New feature. label Oct 28, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 2275 files.

Valid Invalid Ignored Fixed
1080 2 1193 0
Click to see the invalid file list
  • src/stream/src/executor/sort.rs
  • src/stream/src/from_proto/sort.rs

xx01cyx and others added 2 commits October 28, 2022 09:34
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@xx01cyx xx01cyx requested a review from TennyZhuang October 28, 2022 01:39
@TennyZhuang TennyZhuang requested a review from fuyufjh October 28, 2022 06:06
@TennyZhuang
Copy link
Contributor

Please fix clippy lints

@TennyZhuang TennyZhuang marked this pull request as draft October 28, 2022 06:14
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #6085 (9a51b51) into main (912db57) will decrease coverage by 0.15%.
The diff coverage is 48.15%.

@@            Coverage Diff             @@
##             main    #6085      +/-   ##
==========================================
- Coverage   74.36%   74.20%   -0.16%     
==========================================
  Files         935      939       +4     
  Lines      150096   150811     +715     
==========================================
+ Hits       111612   111915     +303     
- Misses      38484    38896     +412     
Flag Coverage Δ
rust 74.20% <48.15%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
src/frontend/src/optimizer/plan_node/plan_base.rs 96.96% <0.00%> (-3.04%) ⬇️
.../frontend/src/optimizer/plan_node/stream_derive.rs 0.00% <0.00%> (ø)
src/meta/src/lib.rs 0.96% <ø> (+0.03%) ⬆️
src/storage/src/hummock/file_cache/cache.rs 90.76% <0.00%> (-0.85%) ⬇️
src/storage/src/hummock/mod.rs 54.97% <0.00%> (-2.22%) ⬇️
src/stream/src/executor/mod.rs 48.32% <ø> (ø)
src/stream/src/from_proto/mod.rs 0.00% <ø> (ø)
src/stream/src/from_proto/sort.rs 0.00% <0.00%> (ø)
src/frontend/src/optimizer/plan_node/stream.rs 13.55% <13.95%> (-0.57%) ⬇️
src/storage/src/hummock/file_cache/store.rs 89.96% <14.28%> (-1.78%) ⬇️
... and 20 more

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

@xx01cyx xx01cyx marked this pull request as ready for review October 28, 2022 11:34
@xx01cyx xx01cyx requested a review from yuhao-su October 28, 2022 11:35
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Great job!!!

@BugenZhao
Copy link
Member

You may need also to update the generated proto for the dashboard.

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM. Great job 🚀

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.

Good job! Done with high quality

Comment on lines +268 to +271
for (owned_vnode, _) in newly_owned_vnodes
.iter()
.filter(|is_set| *is_set)
.enumerate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

May add a new function on Bitmap to iterate on all set bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added this function in the WatermarkFilter branch. I'll modify this to use the function in my PR.

@mergify mergify bot merged commit 8857a02 into main Nov 3, 2022
@mergify mergify bot deleted the cyx/sort-executor branch November 3, 2022 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature Type: New feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants