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

Fix concatenate_lists_ignore_null on rows of all_nulls #8312

Merged

Conversation

sperlingxx
Copy link
Contributor

@sperlingxx sperlingxx commented May 21, 2021

After the rework of cudf::lists::concatenate_rows, something changed on null handling failed corresponding cuDF Java tests.
In specific, when we apply concatenate_null_policy::IGNORE, the output lists are always null free, even if input data contains rows consisting of all nulls.

In my opinion, we had better creating null mask for input rows of all_nulls, to keep align with single column concatenate.

Signed-off-by: sperlingxx [email protected]

Signed-off-by: sperlingxx <[email protected]>
@sperlingxx sperlingxx requested a review from a team as a code owner May 21, 2021 10:40
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 21, 2021
@sperlingxx sperlingxx requested a review from ttnghia May 21, 2021 10:40
@sperlingxx sperlingxx added bug Something isn't working non-breaking Non-breaking change labels May 21, 2021
@sperlingxx sperlingxx changed the title Fix concatenate_lists_ignore_null on all null rows Fix concatenate_lists_ignore_null on rows of all_nulls May 21, 2021
@ttnghia
Copy link
Contributor

ttnghia commented May 21, 2021

In specific, when we apply concatenate_null_policy::IGNORE, the output lists are always null free, even if input data contains rows consisting of all nulls.

I see. I was considering this, but thinking it may be reasonable to just return an empty list instead of a null if we specify to ignore all null from concatenation, but forgot to consider the consistency with the case of concatenating 1 row. Thanks for the rework.

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

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

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

@@               Coverage Diff               @@
##             branch-21.06    #8312   +/-   ##
===============================================
  Coverage                ?   82.49%           
===============================================
  Files                   ?      105           
  Lines                   ?    17520           
  Branches                ?        0           
===============================================
  Hits                    ?    14453           
  Misses                  ?     3067           
  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 9a85b3b...fb22fc7. Read the comment docs.

@sperlingxx
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b84c792 into rapidsai:branch-21.06 May 21, 2021
@sperlingxx sperlingxx deleted the hotfix_list_concat_by_row branch May 24, 2021 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants