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
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions arrow/src/compute/kernels/substring.rs
Expand Up @@ -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.

❤️

/// # Arguments
/// * `input` - A vector which array can be built from.
/// * `start` - The start index of the substring.
/// * `len` - The length of the substring.
/// * `result` - The expected result of substring, which is a vector that array can be built from.
/// # Return
/// A vector of `(input, start, len, result)`.
///
/// Users can provide any number of `(start, len, result)` to generate test cases for one `input`.
macro_rules! gen_test_cases {
($input:expr, $(($start:expr, $len:expr, $result:expr)), *) => {
[
Expand All @@ -455,6 +465,12 @@ mod tests {
};
}

/// A helper macro to test the substring functions.
/// # Arguments
/// * `cases` - The test cases which is a vector of `(input, start, len, result)`.
/// Please look at [`gen_test_cases`] to find how to generate it.
/// * `array_ty` - The array type.
/// * `substring_fn` - Either [`substring`] or [`substring_by_char`].
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!

($cases:expr, $array_ty:ty, $substring_fn:ident) => {
$cases
Expand Down