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

Refactor transform_lists_of_structs in row_operators.cu #13288

Merged

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented May 4, 2023

This split the functions transform_lists_of_structs in row_operators.cu into separate functions or simplified their implementation. From one function that can process both cases of having either one or two input columns, now we have two functions, each one processes one case.

Closes #13287.

@ttnghia ttnghia added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 4, 2023
@ttnghia ttnghia requested a review from a team as a code owner May 4, 2023 02:50
@ttnghia ttnghia self-assigned this May 4, 2023
@ttnghia ttnghia changed the title Complete refactoring [FEA] Refactor transform_lists_of_structs in row_operators.cu May 4, 2023
@ttnghia ttnghia changed the title [FEA] Refactor transform_lists_of_structs in row_operators.cu Refactor transform_lists_of_structs in row_operators.cu May 4, 2023
@ttnghia ttnghia requested review from vyasr, bdice and divyegala and removed request for hyperbolic2346 and nvdbaranec May 4, 2023 02:51
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A few requests -- overall this is great and the changes look much cleaner.

cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Show resolved Hide resolved
cpp/src/table/row_operators.cu Show resolved Hide resolved
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! The code looks in great shape now.

@ttnghia ttnghia requested a review from bdice May 9, 2023 15:52
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Big improvement here, thanks! Mostly small changes requested.

cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Outdated Show resolved Hide resolved
cpp/src/table/row_operators.cu Show resolved Hide resolved
@ttnghia ttnghia requested a review from vyasr May 11, 2023 22:52
@ttnghia
Copy link
Contributor Author

ttnghia commented May 15, 2023

/merge

@rapids-bot rapids-bot bot merged commit 85500af into rapidsai:branch-23.06 May 15, 2023
@ttnghia ttnghia deleted the refactor_transform_lists_of_structs branch May 15, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Refactor transform_lists_of_structs in row_operators.cu
6 participants