-
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
Remove build_struct|list_column
#14786
Remove build_struct|list_column
#14786
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.
Since these APIs are also used in cuspatial, would you mind making a PR to cuSpatial replacing these uses?
@isVoid sure opened a PR in cuspatial: rapidsai/cuspatial#1327 |
Based on #14786 (review) should this be considered a breaking change? |
I think this is considered a private API since it's in the |
Actually no, I don't think this should be a breaking change. cuSpatial shouldn't use these internal APIs in the first place. We discussed about removing all of these usage. Labeling this as a breaking change can make contributors who reads the changelog mistakenly believe the column APIs are public. |
I agree with @isVoid, we shouldn't mark these as breaking. We should just remember while reviewing these sort of changes to cudf's column layer that they have the potential to break cuspatial in particular. |
…/cudf into ref/rm/build_col_types
This API is planned for removal in cudf: rapidsai/cudf#14786 Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mark Harris (https://github.com/harrism) - Michael Wang (https://github.com/isVoid) URL: #1327
@vyasr any other project you can think of that might be using this API? I removed this API from cuspatial in rapidsai/cuspatial#1327 |
Apologies, I missed this query earlier. No, I can't think of any other project using these APIs. Morpheus is the only other package that uses cudf internals, but they tend to use a different set of them (more focused on extracting pointers from columns and then reconstructing them, basically things around pybind/cython interop rather than creating cudf columns from more arbitrary data). |
/merge |
Description
IMO these do not provide much value compared to constructing with
ListColumn
orStructColumn
cc #14778 (comment)Checklist