-
Notifications
You must be signed in to change notification settings - Fork 915
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
Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354
Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354
Changes from 1 commit
694f8d4
06d8691
11141ef
4e43fae
92b3263
034d767
a18c7ef
c2c3221
576e1c6
850ffe6
acef7d0
07ae25d
c36c2cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,7 +161,11 @@ struct column_to_strings_fn { | |
column_view const& column) const | ||
karthikeyann marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
return cudf::strings::detail::from_booleans( | ||
column, options_.get_true_value(), options_.get_false_value(), stream_, mr_); | ||
column, | ||
cudf::string_scalar(options_.get_true_value(), true, stream_), | ||
cudf::string_scalar(options_.get_false_value(), true, stream_), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind making a change similar to the one in #14444? |
||
stream_, | ||
mr_); | ||
} | ||
|
||
// strings: | ||
|
@@ -367,10 +371,10 @@ void write_chunked(data_sink* out_sink, | |
|
||
CUDF_EXPECTS(str_column_view.size() > 0, "Unexpected empty strings column."); | ||
|
||
cudf::string_scalar newline{options.get_line_terminator()}; | ||
cudf::string_scalar newline{options.get_line_terminator(), true, stream}; | ||
auto p_str_col_w_nl = cudf::strings::detail::join_strings(str_column_view, | ||
newline, | ||
string_scalar("", false), | ||
string_scalar("", false, stream), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think, it's not a matter of being consistent. That temporary scalar is not required after this usage. So, it's not a named parameter. its scope ends after function call. |
||
stream, | ||
rmm::mr::get_current_device_resource()); | ||
strings_column_view strings_column{p_str_col_w_nl->view()}; | ||
|
@@ -472,14 +476,15 @@ void write_csv(data_sink* out_sink, | |
// | ||
std::string delimiter_str{options.get_inter_column_delimiter()}; | ||
auto str_concat_col = [&] { | ||
cudf::string_scalar narep{options.get_na_rep(), true, stream}; | ||
if (str_table_view.num_columns() > 1) | ||
return cudf::strings::detail::concatenate(str_table_view, | ||
delimiter_str, | ||
options.get_na_rep(), | ||
strings::separator_on_nulls::YES, | ||
stream, | ||
rmm::mr::get_current_device_resource()); | ||
cudf::string_scalar narep{options.get_na_rep()}; | ||
return cudf::strings::detail::concatenate( | ||
str_table_view, | ||
cudf::string_scalar(delimiter_str, true, stream), | ||
narep, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One parameter is inline defined while the other is not. Please make them consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That temporary scalar is not required after this usage. So, it's not a named parameter. its scopes end after function call. |
||
strings::separator_on_nulls::YES, | ||
stream, | ||
rmm::mr::get_current_device_resource()); | ||
return cudf::strings::detail::replace_nulls( | ||
str_table_view.column(0), narep, stream, rmm::mr::get_current_device_resource()); | ||
}(); | ||
|
vuule marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is introducing a use of the default stream, which we want to avoid. There are public APIs accepting a user stream that can call this, which won't use the desired stream. We need to refactor all callers of this code path, and pass their stream through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the stream relevant in this case? Is any allocation actually happening when creating an empty column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed an issue a while ago for it: #11463 and there is also another issue for it: #13737.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So before that issue is closed, we don't have any input stream to put into line 115 above and have to use the default stream anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupby needs a bigger cleanup; I can exclude it as part of this PR. It should be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed groupby from this PR.