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(batch): introduce delete executor in compute node #883

Merged
merged 9 commits into from
Mar 14, 2022

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Mar 14, 2022

What's changed and what's your intention?

This PR introduces delete executor in batch crate.

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)

A step of #16.

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #883 (7d1c536) into main (d6d3d81) will increase coverage by 0.02%.
The diff coverage is 79.11%.

@@             Coverage Diff              @@
##               main     #883      +/-   ##
============================================
+ Coverage     72.30%   72.32%   +0.02%     
  Complexity     2766     2766              
============================================
  Files           934      935       +1     
  Lines         54867    54992     +125     
  Branches       1787     1787              
============================================
+ Hits          39670    39772     +102     
- Misses        14307    14330      +23     
  Partials        890      890              
Flag Coverage Δ
java 61.22% <ø> (ø)
rust 76.78% <79.11%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
rust/batch/src/executor/mod.rs 22.91% <0.00%> (-0.49%) ⬇️
rust/common/src/catalog/mod.rs 66.66% <ø> (ø)
rust/stream/src/executor/test_utils.rs 75.00% <ø> (+1.38%) ⬆️
rust/common/src/catalog/schema.rs 80.95% <50.00%> (-19.05%) ⬇️
rust/batch/src/executor/insert.rs 83.65% <66.66%> (-1.11%) ⬇️
rust/batch/src/executor/delete.rs 79.06% <79.06%> (ø)
rust/compute/tests/table_v2_materialize.rs 94.52% <92.10%> (+2.62%) ⬆️
rust/source/src/table_v2.rs 94.82% <100.00%> (ø)
rust/stream/src/executor/local_simple_agg.rs 85.07% <100.00%> (ø)
rust/stream/src/executor/mview/state.rs 96.29% <100.00%> (ø)
... and 3 more

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

Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao marked this pull request as ready for review March 14, 2022 06:08
}

async fn next(&mut self) -> Result<Option<DataChunk>> {
if self.executed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not specific to this PR, just try to confirm whether it is the standard in our system now that next on an already finished executor should be a no-op instead of err?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1. Also curious about this.

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

LGTM

@BugenZhao BugenZhao merged commit 7be7027 into main Mar 14, 2022
@BugenZhao BugenZhao deleted the bz/delete-part-1 branch March 14, 2022 07:31
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.

2 participants