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

Implement interleave_columns for structs columns #9012

Merged

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Aug 11, 2021

This PR adds support for structs column in the interleave_column API. In addition, it also does a simple refactor of the existing overload functions with a new style of SFINAE implementation.
Closes #8927.

@ttnghia ttnghia added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 11, 2021
@ttnghia ttnghia self-assigned this Aug 11, 2021
@ttnghia ttnghia requested a review from a team as a code owner August 11, 2021 03:27
@ttnghia ttnghia removed the improvement Improvement / enhancement to an existing function label Aug 11, 2021
@ttnghia
Copy link
Contributor Author

ttnghia commented Aug 11, 2021

@andygrove Please test this with your test cases in Spark. Thanks.

@rapidsai rapidsai deleted a comment from codecov bot Aug 11, 2021
@rapidsai rapidsai deleted a comment from codecov bot Aug 11, 2021
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

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

Had a question, does this support struct with nesting level more than 1? like Struct{Struct{Struct}}, adding a test case to cover that scenario would be helpful.

Rest looks good.

@ttnghia
Copy link
Contributor Author

ttnghia commented Aug 17, 2021

Had a question, does this support struct with nesting level more than 1? like Struct{Struct{Struct}}, adding a test case to cover that scenario would be helpful.

Rest looks good.

Thanks for the recommendation. Nested structs should be supported, as the interleaving process is called recursively on the children columns. I have added a test for this (2 nested levels should be enough for verifying this case).

@rapidsai rapidsai deleted a comment from codecov bot Aug 17, 2021
@ttnghia
Copy link
Contributor Author

ttnghia commented Aug 17, 2021

Rerun tests.

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@34523c7). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head ce20429 differs from pull request most recent head 9b5ddba. Consider uploading reports for the commit 9b5ddba to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9012   +/-   ##
===============================================
  Coverage                ?   10.71%           
===============================================
  Files                   ?      114           
  Lines                   ?    18699           
  Branches                ?        0           
===============================================
  Hits                    ?     2003           
  Misses                  ?    16696           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34523c7...9b5ddba. Read the comment docs.

@ttnghia
Copy link
Contributor Author

ttnghia commented Aug 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b3c1caf into rapidsai:branch-21.10 Aug 17, 2021
@ttnghia ttnghia deleted the interleave_columns_for_structs branch August 18, 2021 15:12
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 feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Support creating lists of structs
3 participants