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

refactor(stream): human-friendly StreamChunk literal for test #2089

Merged
merged 6 commits into from
Apr 24, 2022

Conversation

wangrunji0408
Copy link
Contributor

@wangrunji0408 wangrunji0408 commented Apr 24, 2022

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Now it's painful to read unit tests of executors because the code to construct and verify StreamChunk is verbose.

This PR introduces a literal format for StreamChunk. An example is as follows:

   I I I   F      // a header with types: 'I' for i64, 'F' for f64
U- 2 5 .   .      // '.' for NULL
U+ 2 5 2 6.0 D    // 'D' for deleted in visibility
+  . . 4 8.0      // "//" for comments
-  . . 3 4.0

Developers can use StreamChunk::from_str to build a chunk from literal.
We also impl PartialEq for StreamChunk so that output chunks can be verified by assert_eq!.

Using these utilities, I rewrote a lot of code in the unit tests to make them more readable.

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)

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2022

CLA assistant check
All committers have signed the CLA.

@TennyZhuang
Copy link
Collaborator

I'd prefer to introduce a new trait StreamChunkTestExt, to avoid misuse.

Also, it's good to create a mod like test_prebuilt.

Copy link
Collaborator

@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.

Rest LGTM

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@wangrunji0408 wangrunji0408 enabled auto-merge (squash) April 24, 2022 10:53
@codecov
Copy link

codecov bot commented Apr 24, 2022

Codecov Report

Merging #2089 (e386989) into main (6019ef1) will decrease coverage by 0.39%.
The diff coverage is 99.57%.

@@            Coverage Diff             @@
##             main    #2089      +/-   ##
==========================================
- Coverage   71.03%   70.64%   -0.40%     
==========================================
  Files         635      635              
  Lines       81661    80464    -1197     
==========================================
- Hits        58010    56843    -1167     
+ Misses      23651    23621      -30     
Flag Coverage Δ
rust 70.64% <99.57%> (-0.40%) ⬇️

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

Impacted Files Coverage Δ
src/stream/src/executor_v2/chain.rs 76.08% <83.33%> (-2.22%) ⬇️
src/common/src/array/stream_chunk.rs 74.90% <94.93%> (-14.52%) ⬇️
src/common/src/array/column.rs 100.00% <100.00%> (ø)
src/common/src/array/data_chunk.rs 94.97% <100.00%> (ø)
src/stream/src/executor/dispatch.rs 77.89% <100.00%> (+0.77%) ⬆️
src/stream/src/executor/hash_join.rs 81.60% <100.00%> (-7.25%) ⬇️
src/stream/src/executor/source.rs 73.78% <100.00%> (-0.93%) ⬇️
src/stream/src/executor_v2/debug/schema_check.rs 94.33% <100.00%> (-0.40%) ⬇️
src/stream/src/executor_v2/debug/update_check.rs 93.75% <100.00%> (-0.28%) ⬇️
src/stream/src/executor_v2/filter.rs 89.82% <100.00%> (+0.49%) ⬆️
... and 30 more

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

@wangrunji0408 wangrunji0408 merged commit 0c94f47 into main Apr 24, 2022
@wangrunji0408 wangrunji0408 deleted the wrj/simplify-executor-test branch April 24, 2022 11:04
@@ -62,7 +62,7 @@ impl DataChunkBuilder {
}

/// `DataChunk` is a collection of arrays with visibility mask.
#[derive(Clone, Default)]
#[derive(Clone, Default, PartialEq)]
Copy link
Member

@BugenZhao BugenZhao Apr 24, 2022

Choose a reason for hiding this comment

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

Should we consider comparing two data chunks regardless of the visibility? That is, even though the visibilities differ, they are the same since the data after compaction is the same.

Seems this is used for test only.

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.

None yet

4 participants