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

Clean up the test code of substring kernel. #1853

Merged
merged 6 commits into from Jun 17, 2022

Conversation

HaoYang670
Copy link
Contributor

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #1801.

Rationale for this change

Avoid repeated code.
More readable.
Less code is better.

What changes are included in this PR?

Use macros.
Better format.

Are there any user-facing changes?

No.

Signed-off-by: remzi <13716567376yh@gmail.com>
@HaoYang670 HaoYang670 marked this pull request as draft June 13, 2022 01:41
@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 13, 2022
@HaoYang670
Copy link
Contributor Author

Blocked by #1852

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2022

Codecov Report

Merging #1853 (d788f99) into master (6144f19) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
- Coverage   83.51%   83.43%   -0.08%     
==========================================
  Files         201      203       +2     
  Lines       56948    56954       +6     
==========================================
- Hits        47559    47519      -40     
- Misses       9389     9435      +46     
Impacted Files Coverage Δ
arrow/src/compute/kernels/substring.rs 99.33% <100.00%> (-0.20%) ⬇️
arrow/src/buffer/mutable.rs 89.13% <0.00%> (-1.41%) ⬇️
arrow/src/compute/kernels/filter.rs 88.05% <0.00%> (-0.28%) ⬇️
arrow/src/array/array_primitive.rs 94.97% <0.00%> (-0.09%) ⬇️
arrow/src/array/array_binary.rs 94.18% <0.00%> (-0.06%) ⬇️
arrow/src/ffi.rs 86.97% <0.00%> (ø)
arrow/src/pyarrow.rs 0.00% <0.00%> (ø)
arrow/src/array/ffi.rs 100.00% <0.00%> (ø)
arrow/src/array/data.rs 84.16% <0.00%> (ø)
arrow/src/buffer/ops.rs 96.77% <0.00%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6144f19...d788f99. Read the comment docs.

@HaoYang670 HaoYang670 marked this pull request as ready for review June 13, 2022 12:20
@tustvold
Copy link
Contributor

Sorry for the delay, I'll try to get to this later today if nobody else does, I'm currently on holiday and reviewing large PRs like this on a phone is challenging 😅

@HaoYang670
Copy link
Contributor Author

Never mind. Have a good holiday! 🏝

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @HaoYang670 -- this looks like a nice cleanup to me

};
}

macro_rules! do_test {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some docstrings explaining what was going on here (specifically what the expectations on `$cases) are would have helped me understand this code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!


macro_rules! do_test {
($cases:expr, $array_ty:ty, $substring_fn:ident) => {
$cases.into_iter().try_for_each::<_, Result<()>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why bother with try_for_each and not just use unwrap() or expect()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Updated.

Signed-off-by: remzi <13716567376yh@gmail.com>
Signed-off-by: remzi <13716567376yh@gmail.com>
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks great -- thanks again @HaoYang670

@@ -445,6 +445,16 @@ mod tests {
use super::*;
use crate::datatypes::*;

/// A helper macro to generate test cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@alamb alamb merged commit 45846f0 into apache:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up the testing code for substring kernel
4 participants