Added Zip operation and Sample operation #26

Xintong-Zhan · 2023-12-05T00:53:02Z

Files modified:

examples/filter_even.cpp: Added usage examples of Sample operation.
examples/simple.cpp: Added a usage eample of Zip operation.
include/filter_rdd.h: Included ramdom header for the support of Sample operation.

Files added:

include/sample_rdd.h: Sample operation. This operation is based on FilterView by passing random boolean function as a param to FilterView.
include/zipped_rdd.h: Zip operation. No pipeline is implemented for the Zip operation since both params are Rdd, and using pipeline with one Rdd to be function param is a little bit weird.
tests/sample_rdd_test.cpp: Google test for Sample.
tests/zipped_rdd_test.cpp: Goole test for Zip.

…examples

codecov · 2023-12-05T00:56:09Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b15424d) 97.29% compared to head (2cf7f09) 97.36%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
include/zipped_rdd.h	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   97.29%   97.36%   +0.06%     
==========================================
  Files          22       26       +4     
  Lines         628      682      +54     
==========================================
+ Hits          611      664      +53     
- Misses         17       18       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Alex-XJK

I think the doxygen comment should be added. I have already provide some skeleton code for you, please double check it and add additional description if needed.

include/filter_rdd.h

include/sample_rdd.h

include/zipped_rdd.h

Alex-XJK · 2023-12-05T01:35:59Z

include/zipped_rdd.h

+  using V1 = utils::RddElementType<R1>;
+  using V2 = utils::RddElementType<R2>;
+
+  constexpr ZippedRdd(const R1& prev1, const R2& prev2) : Base{prev1, false} {


Shall we unify the name convention? Currently, it looks like some Rdd's are named in the present tense (Sample, Merge), and some are named in the past tense (Zipped, Transformed).

I think it is a relatively minor issue. But if any of you are available, it does not hurt to rename them to past tense.

Alex-XJK · 2023-12-05T01:44:58Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b15424d) 97.29% compared to head (908a0bf) 97.22%.
Report is 2 commits behind head on main.

Files Patch % Lines
include/zipped_rdd.h 92.30% 1 Missing ⚠️
tests/sample_rdd_test.cpp 95.00% 1 Missing ⚠️
Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

It is strange that one of the lines in the TEST script didn't get covered, because they are the ones we used to test other headers.
If there are no other coding issues hidden behind, I think we can simply ignore this coverage issue because, obviously, getting it functionally correct is much more important than getting it 100% coverage, at least for now.
If other reviewers approved all other parts, but this Codecov is still blocking the merge, let me know, and I can force merge it 😊

chenlighten

Thanks for finishing sampe_rdd and zipped_rdd. Commented some minor issues. You can take a look of them if you have time. If you are busy, feel free to take care of them in the future.

include/sample_rdd.h

chenlighten · 2023-12-05T02:43:36Z

include/zipped_rdd.h

+  using V1 = utils::RddElementType<R1>;
+  using V2 = utils::RddElementType<R2>;
+
+  constexpr ZippedRdd(const R1& prev1, const R2& prev2) : Base{prev1, false} {


I think it is a relatively minor issue. But if any of you are available, it does not hurt to rename them to past tense.

include/zipped_rdd.h

Xintong-Zhan · 2023-12-05T04:12:44Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b15424d) 97.29% compared to head (908a0bf) 97.22%.
Report is 2 commits behind head on main.

Files Patch % Lines
include/zipped_rdd.h 92.30% 1 Missing ⚠️
tests/sample_rdd_test.cpp 95.00% 1 Missing ⚠️
Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

For the Google Test script, the line that didn't get covered is for the SampleRdd operation with fraction = 0, which means the returned Rdd should contain nothing, thus the program won't get in the loop body for Rdd and I guess that's the reason for not being covered. And because of the random feature, the only deterministic case I could test in Google test script is fraction = 1 and fraction = 0. For other random sample cases, I present them in the ./examples directory.

tests/sample_rdd_test.cpp

Co-authored-by: Alex_XU <[email protected]>

Alex-XJK

Thank you for your effort!

Xintong-Zhan added 7 commits December 4, 2023 13:12

add zip

6b9920d

Fixed the bug in zipped_rdd and modified corresponding example file

e597523

Test the ramdom sample function as the input for filter_rdd

ff41c99

Add the initial implementation of sample_rdd

13543ce

Added the operations of Sample and Zip; added the corresponding test …

62f97f2

…examples

Added Google Test for Zip and Sample

91caf0a

Fixed zip rdd test bugs & Added Splits number check

908a0bf

Xintong-Zhan requested review from chenlighten and Alex-XJK December 5, 2023 00:53

Alex-XJK requested changes Dec 5, 2023

View reviewed changes

Alex-XJK assigned Xintong-Zhan Dec 5, 2023

chenlighten reviewed Dec 5, 2023

View reviewed changes

Alex-XJK reviewed Dec 5, 2023

View reviewed changes

tests/sample_rdd_test.cpp Outdated Show resolved Hide resolved

Xintong-Zhan and others added 7 commits December 5, 2023 10:43

Update include/sample_rdd.h

6a34844

Co-authored-by: Alex_XU <[email protected]>

Update include/sample_rdd.h

0e4103e

Co-authored-by: Alex_XU <[email protected]>

Update include/zipped_rdd.h

f4a8d5f

Co-authored-by: Alex_XU <[email protected]>

Update include/zipped_rdd.h

b89b0e9

Co-authored-by: Alex_XU <[email protected]>

Update tests/sample_rdd_test.cpp

5b720ec

Co-authored-by: Alex_XU <[email protected]>

Update codes to solve comment issues

b92fa6b

Deleted the unkown symbolic link

2cf7f09

Alex-XJK approved these changes Dec 5, 2023

View reviewed changes

Xintong-Zhan merged commit a1e82f2 into main Dec 5, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Zip operation and Sample operation #26

Added Zip operation and Sample operation #26

Xintong-Zhan commented Dec 5, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading

Alex-XJK left a comment

Alex-XJK Dec 5, 2023

chenlighten Dec 5, 2023

Alex-XJK commented Dec 5, 2023

Codecov Report

chenlighten left a comment •

edited

Loading

chenlighten Dec 5, 2023

Xintong-Zhan commented Dec 5, 2023

Codecov Report

Alex-XJK left a comment

Added Zip operation and Sample operation #26

Added Zip operation and Sample operation #26

Conversation

Xintong-Zhan commented Dec 5, 2023

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

Alex-XJK left a comment

Choose a reason for hiding this comment

Alex-XJK Dec 5, 2023

Choose a reason for hiding this comment

chenlighten Dec 5, 2023

Choose a reason for hiding this comment

Alex-XJK commented Dec 5, 2023

Codecov Report

chenlighten left a comment • edited Loading

Choose a reason for hiding this comment

chenlighten Dec 5, 2023

Choose a reason for hiding this comment

Xintong-Zhan commented Dec 5, 2023

Codecov Report

Alex-XJK left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2023 •

edited

Loading

chenlighten left a comment •

edited

Loading