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.

Sign up for GitHub

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

Expose stream parameter in public strings split/partition APIs #14247

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Expose stream parameter in public strings split/partition APIs #14247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4da5749
Expose stream parameter in public strings split/partition APIs
davidwendt Oct 4, 2023
d7cfbcb
fix doxygen consistency
davidwendt Oct 4, 2023
e24715a
Merge branch 'branch-23.12' into stream-strings-partition
davidwendt Oct 4, 2023
525d31a
Merge branch 'branch-23.12' into stream-strings-partition
davidwendt Oct 5, 2023
95c76e1
Merge branch 'branch-23.12' into stream-strings-partition
davidwendt Oct 5, 2023
5a071e9
fix merge conflict
davidwendt Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions cpp/include/cudf/strings/split/partition.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2022, NVIDIA CORPORATION.
* Copyright (c) 2019-2023, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -51,15 +51,17 @@ namespace strings {
* r[2] is ["cd","g_h"]
* @endcode
*
* @param strings Strings instance for this operation.
* @param input Strings instance for this operation
* @param delimiter UTF-8 encoded string indicating where to split each string.
* Default of empty string indicates split on whitespace.
* @param mr Device memory resource used to allocate the returned table's device memory.
* @return New table of strings columns.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned table's device memory
* @return New table of strings columns
Comment on lines +54 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It is a bit awkward to have mixed style like this: some sentences end with period while some others don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest for the delimiter parameter description? I've used semi-colon before but that was criticized as well.

Copy link
Contributor

@ttnghia ttnghia Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a difficult question too. IMO the best and most consistent way is to end every sentence with a period so we don't have situations like this. But since we are not using period, probably we should avoid having separate sentences for each parameter description. For example:

 * @param delimiter UTF-8 encoded string indicating where to split each string,
 *        default of empty string indicates split on whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is these descriptions are not sentences and so do not warrant a period.
I'll go with a comma for now if that looks better though I'd prefer a semi-colon.
Unfortunately, we don't seem to have any consensus on this.

Copy link
Member

@harrism harrism Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll weigh in. :) Docs are writing that we would like to be readable, so we should apply sound style and grammar rules. If it's a sentence, end it with a period. I argue that parameter definitions should be sentences, even though we often leave out definite articles and pronouns and the verb ("to be") is often implied ("This CUDA stream is used for device memory operations and kernel launches."). Treating them all as sentences ensures consistent style when a parameter definition requires more than one sentence, as in this case.

I agree that semicolons are fine to use where appropriate. Dusting off my copy of Strunk & White, chapter 1:

(Rule 5) Do not join independent clauses with a comma. If two or more clauses grammatically complete and not joined by a conjunction are to form a single compound sentence, the proper mark of punctuation is a semicolon.
(Rule 6) Do not break sentences in two. In other words, don't use periods as commas.

Mr. White didn't give a rule for when to decide that two clauses should form a compound sentence, though he mentioned typical usage is "cause and consequence". Personally I think in the case of delimiter it makes sense as two sentences the way you have it.

*/
std::unique_ptr<table> partition(
strings_column_view const& strings,
strings_column_view const& input,
string_scalar const& delimiter = string_scalar(""),
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
Expand All @@ -83,15 +85,17 @@ std::unique_ptr<table> partition(
* r[2] is ["cd","h"]
* @endcode
*
* @param strings Strings instance for this operation.
* @param input Strings instance for this operation
* @param delimiter UTF-8 encoded string indicating where to split each string.
* Default of empty string indicates split on whitespace.
* @param mr Device memory resource used to allocate the returned table's device memory.
* @return New strings columns.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned table's device memory
* @return New strings columns
*/
std::unique_ptr<table> rpartition(
strings_column_view const& strings,
strings_column_view const& input,
string_scalar const& delimiter = string_scalar(""),
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/** @} */ // end of doxygen group
Expand Down
16 changes: 12 additions & 4 deletions cpp/include/cudf/strings/split/split_re.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,15 @@ struct regex_program;
* @param prog Regex program instance
* @param maxsplit Maximum number of splits to perform.
* Default of -1 indicates all possible splits on each string.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned result's device memory
* @return A table of columns of strings
*/
std::unique_ptr<table> split_re(
strings_column_view const& input,
regex_program const& prog,
size_type maxsplit = -1,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
Expand Down Expand Up @@ -125,17 +127,19 @@ std::unique_ptr<table> split_re(
*
* @throw cudf::logic_error if `pattern` is empty.
*
* @param input A column of string elements to be split.
* @param input A column of string elements to be split
* @param prog Regex program instance
* @param maxsplit Maximum number of splits to perform.
* Default of -1 indicates all possible splits on each string.
* @param mr Device memory resource used to allocate the returned result's device memory.
* @return A table of columns of strings.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned result's device memory
* @return A table of columns of strings
*/
std::unique_ptr<table> rsplit_re(
strings_column_view const& input,
regex_program const& prog,
size_type maxsplit = -1,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
Expand Down Expand Up @@ -185,13 +189,15 @@ std::unique_ptr<table> rsplit_re(
* @param prog Regex program instance
* @param maxsplit Maximum number of splits to perform.
* Default of -1 indicates all possible splits on each string.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned result's device memory
* @return Lists column of strings.
* @return Lists column of strings
*/
std::unique_ptr<column> split_record_re(
strings_column_view const& input,
regex_program const& prog,
size_type maxsplit = -1,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
Expand Down Expand Up @@ -243,13 +249,15 @@ std::unique_ptr<column> split_record_re(
* @param prog Regex program instance
* @param maxsplit Maximum number of splits to perform.
* Default of -1 indicates all possible splits on each string.
* @param stream CUDA stream used for device memory operations and kernel launches
* @param mr Device memory resource used to allocate the returned result's device memory
* @return Lists column of strings
*/
std::unique_ptr<column> rsplit_record_re(
strings_column_view const& input,
regex_program const& prog,
size_type maxsplit = -1,
rmm::cuda_stream_view stream = cudf::get_default_stream(),
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/** @} */ // end of doxygen group
Expand Down
10 changes: 6 additions & 4 deletions cpp/src/strings/split/partition.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,20 +239,22 @@ std::unique_ptr<table> rpartition(strings_column_view const& strings,

// external APIs

std::unique_ptr<table> partition(strings_column_view const& strings,
std::unique_ptr<table> partition(strings_column_view const& input,
string_scalar const& delimiter,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::partition(strings, delimiter, cudf::get_default_stream(), mr);
return detail::partition(input, delimiter, stream, mr);
}

std::unique_ptr<table> rpartition(strings_column_view const& strings,
std::unique_ptr<table> rpartition(strings_column_view const& input,
string_scalar const& delimiter,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::rpartition(strings, delimiter, cudf::get_default_stream(), mr);
return detail::rpartition(input, delimiter, stream, mr);
}

} // namespace strings
Expand Down
12 changes: 8 additions & 4 deletions cpp/src/strings/split/split_re.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,37 +340,41 @@ std::unique_ptr<column> rsplit_record_re(strings_column_view const& input,
std::unique_ptr<table> split_re(strings_column_view const& input,
regex_program const& prog,
size_type maxsplit,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::split_re(input, prog, maxsplit, cudf::get_default_stream(), mr);
return detail::split_re(input, prog, maxsplit, stream, mr);
}

std::unique_ptr<column> split_record_re(strings_column_view const& input,
regex_program const& prog,
size_type maxsplit,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::split_record_re(input, prog, maxsplit, cudf::get_default_stream(), mr);
return detail::split_record_re(input, prog, maxsplit, stream, mr);
}

std::unique_ptr<table> rsplit_re(strings_column_view const& input,
regex_program const& prog,
size_type maxsplit,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::rsplit_re(input, prog, maxsplit, cudf::get_default_stream(), mr);
return detail::rsplit_re(input, prog, maxsplit, stream, mr);
}

std::unique_ptr<column> rsplit_record_re(strings_column_view const& input,
regex_program const& prog,
size_type maxsplit,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
return detail::rsplit_record_re(input, prog, maxsplit, cudf::get_default_stream(), mr);
return detail::rsplit_record_re(input, prog, maxsplit, stream, mr);
}

} // namespace strings
Expand Down
2 changes: 1 addition & 1 deletion cpp/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ ConfigureTest(STREAM_SEARCH_TEST streams/search_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_DICTIONARY_TEST streams/dictionary_test.cpp STREAM_MODE testing)
ConfigureTest(
STREAM_STRINGS_TEST streams/strings/case_test.cpp streams/strings/find_test.cpp
streams/strings/strings_tests.cpp STREAM_MODE testing
streams/strings/split_test.cpp streams/strings/strings_tests.cpp STREAM_MODE testing
)
ConfigureTest(STREAM_SORTING_TEST streams/sorting_test.cpp STREAM_MODE testing)
ConfigureTest(STREAM_TEXT_TEST streams/text/ngrams_test.cpp STREAM_MODE testing)
Expand Down
49 changes: 49 additions & 0 deletions cpp/tests/streams/strings/split_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright (c) 2023, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <cudf_test/base_fixture.hpp>
#include <cudf_test/column_wrapper.hpp>
#include <cudf_test/default_stream.hpp>

#include <cudf/strings/regex/regex_program.hpp>
#include <cudf/strings/split/partition.hpp>
#include <cudf/strings/split/split.hpp>
#include <cudf/strings/split/split_re.hpp>

#include <string>

class StringsSplitTest : public cudf::test::BaseFixture {};

TEST_F(StringsSplitTest, SplitPartition)
{
auto input = cudf::test::strings_column_wrapper({"Héllo thesé", "tést strings", ""});
auto view = cudf::strings_column_view(input);

auto const delimiter = cudf::string_scalar("é", true, cudf::test::get_default_stream());
cudf::strings::split(view, delimiter, -1, cudf::test::get_default_stream());
cudf::strings::rsplit(view, delimiter, -1, cudf::test::get_default_stream());
cudf::strings::split_record(view, delimiter, -1, cudf::test::get_default_stream());
cudf::strings::rsplit_record(view, delimiter, -1, cudf::test::get_default_stream());
cudf::strings::partition(view, delimiter, cudf::test::get_default_stream());
cudf::strings::rpartition(view, delimiter, cudf::test::get_default_stream());

auto const pattern = std::string("\\s");
auto const prog = cudf::strings::regex_program::create(pattern);
cudf::strings::split_re(view, *prog, -1, cudf::test::get_default_stream());
cudf::strings::split_record_re(view, *prog, -1, cudf::test::get_default_stream());
cudf::strings::rsplit_re(view, *prog, -1, cudf::test::get_default_stream());
cudf::strings::rsplit_record_re(view, *prog, -1, cudf::test::get_default_stream());
}