-
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
Deprecate cudf::make_strings_column accepting typed offsets #14461
Deprecate cudf::make_strings_column accepting typed offsets #14461
Conversation
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.
Looks good, just some follow-up questions
template <> // CUDF_TYPE_MAPPING(char,INT8) causes duplicate id_to_type_impl definition | ||
constexpr inline type_id type_to_id<char>() | ||
{ | ||
return type_id::INT8; |
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 there any need to have actual CHAR data type in cudf?
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.
No. Because we would just remove it with PR #14202
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 was thinking outside of the context of strings columns. Just a char column as such.
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 cannot think of a reason to have one outside of strings.
I fear it would create issues in how the column data is interpretted/encoded.
@GregoryKimball asked me to cc @etseidl on this PR in case you are using these APIs in any current pending work. |
Thanks for the heads up...I believe I looked at this PR earlier and didn't think there'd be any problems...I'll do a closer look in a bit. Edit: I only seem to use
so I'm good. |
/merge |
A call to a deprecated `cudf::make_strings_column` factory function was introduced in #14664. This call had been previously fixed in #14461 but was lost when the source file was moved in #14664. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #14695
Description
Deprecates the following factory functions:
The first one is not needed, rarely used, and inefficient. The only callers found can use other, more efficient functions which do not require copying the device data. The 2nd one is only used in 8/150 places and can be easily recoded in place to use type-erased factory function instead.
Removing these APIs helps reduce the number of functions that require type-specific offsets to make it easier to support offsets of larger types in the future.
Checklist