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

Preserve column hierarchy when getting NULL row from LIST column #8206

Merged
merged 7 commits into from
May 24, 2021

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented May 11, 2021

This PR fixes a bug introduced in #8071, when get_element retrieves a NULL row in a nested column, the scalar returned not only should be is_valid() == false, but also should preserve the column hierarchy of the row-data, even they are invalid. Because depending libraries may use the column hierarchy to deduce the nested type of the column.

This PR also reverts make_default_constructed_scalar API for LIST type. A LIST type scalar should have complete column hierarchy as part of its type information. There isn't enough information provided to the API to construct that.

Another tiny addition: instead of hard coding the position of child column, use list_column_view::child_column_index intead.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 11, 2021
@isVoid isVoid marked this pull request as ready for review May 11, 2021 20:27
@isVoid isVoid requested a review from a team as a code owner May 11, 2021 20:27
@isVoid isVoid requested review from trxcllnt and codereport May 11, 2021 20:27
@isVoid isVoid self-assigned this May 11, 2021
@isVoid isVoid added 3 - Ready for Review Ready for review by team breaking Breaking change bug Something isn't working labels May 11, 2021
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8206   +/-   ##
===============================================
  Coverage                ?   82.83%           
===============================================
  Files                   ?      105           
  Lines                   ?    17861           
  Branches                ?        0           
===============================================
  Hits                    ?    14796           
  Misses                  ?     3065           
  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 0ebf7e6...9e2ced9. Read the comment docs.

@trxcllnt trxcllnt changed the title Preserve column hierarchy when getting NULL row from LIST column Preserve column hierarchy when getting NULL row from LIST column. May 18, 2021
@trxcllnt trxcllnt changed the title Preserve column hierarchy when getting NULL row from LIST column. Preserve column hierarchy when getting NULL row from LIST column May 18, 2021
@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 20, 2021
@isVoid
Copy link
Contributor Author

isVoid commented May 24, 2021

Don't know if I have cpp merge privilege so I'll test on this one.
@gpucibot merge

@harrism
Copy link
Member

harrism commented May 24, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7eaf3d7 into rapidsai:branch-21.06 May 24, 2021
@harrism
Copy link
Member

harrism commented May 24, 2021

@isVoid You need to put the @gpucibot merge alone in a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change bug Something isn't working libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants