Purge nonempty nulls from byte_cast list outputs. #11971

bdice · 2022-10-21T20:44:16Z

Description

Resolves #11754. The byte_cast function is creating unsanitized lists from null inputs, which is a bug. This logic copies nonzero bytes even if the input element is null. The input's null mask is copied onto the output parent list column, but the null children are nonempty. This PR fixes the bug by calling cudf::purge_nonempty_nulls on the result before returning, if there are any nulls to be purged.

Depends on:

Use make_empty_lists_column instead of make_empty_column(type_id::LIST) #13099

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…st-sanitized

codecov · 2022-10-28T00:06:27Z

Codecov Report

Patch coverage has no change and project coverage change: +2.68 🎉

Comparison is base (ed9385b) 85.47% compared to head (d18c557) 88.16%.

❗ Current head d18c557 differs from pull request most recent head 92c9dfb. Consider uploading reports for the commit 92c9dfb to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.06   #11971      +/-   ##
================================================
+ Coverage         85.47%   88.16%   +2.68%     
================================================
  Files               152      133      -19     
  Lines             24650    21977    -2673     
================================================
- Hits              21069    19375    -1694     
+ Misses             3581     2602     -979

see 107 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ttnghia · 2022-10-28T04:16:02Z

This is fixing for lists. What about strings?

cpp/src/reshape/byte_cast.cu

hyperbolic2346 · 2022-10-28T19:14:57Z

Would it be better to not copy the null data to begin with instead of making a new pass to remove the data that was copied? Is there a reason that wouldn't work?

bdice · 2022-10-28T20:59:47Z

This is fixing for lists. What about strings?

Very good catch @ttnghia. I'll address this soon.

ttnghia · 2022-12-22T03:28:37Z

Would it be better to not copy the null data to begin with instead of making a new pass to remove the data that was copied? Is there a reason that wouldn't work?

We need to know the exact target location to copy to. If we want to generate empty lists directly,we have to recompute offsets before copying. I believe that it is not cheaper while more complicated.

cpp/src/reshape/byte_cast.cu

The base branch was changed.

cpp/src/reshape/byte_cast.cu

hyperbolic2346

In general this is looking good to me. Just a couple of questions and suggestions.

cpp/src/reshape/byte_cast.cu

hyperbolic2346 · 2023-04-09T16:01:41Z

cpp/src/reshape/byte_cast.cu

+}
+
+template <typename T>
+struct byte_list_conversion_fn<


Should this one be up with the other and not straddling the dispatcher?

Sorry we still have to dispatch, since there are many types in cudf::is_numeric.

cpp/src/reshape/byte_cast.cu

davidwendt

Looks good.
Can there be tests for an empty input column too?
And perhaps tests for any throw conditions as well? Or maybe I missed those.

cpp/tests/reshape/byte_cast_tests.cpp

Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>

ttnghia · 2023-04-10T03:56:19Z

Can there be tests for an empty input column too?

Thanks. Having the suggested tests, I discovered bug(s) when the input is empty/all nulls. Now they're fixed.

ttnghia · 2023-04-10T15:45:40Z

Since this also uses lists::make_empty_lists_column, it depends on:

Use make_empty_lists_column instead of make_empty_column(type_id::LIST) #13099

…/byte-cast-sanitized

…the column factory methods

davidwendt

LGTM

vyasr

I love that we had tests explicitly noting that we had unsanitized data and working around it.

One non-blocking note about east vs west constexpr, otherwise good to ship from me.

vyasr · 2023-04-12T23:07:06Z

cpp/src/reshape/byte_cast.cu

-             rmm::mr::device_memory_resource*) const
+
+// Data type of the output data column after conversion.
+data_type constexpr output_type{data_type{type_id::UINT8}};


We don't really use "east constexpr" anywhere in our codebase. Many of the reasons that east const makes sense don't really make sense for constexpr, particularly when it comes to the confusion around pointers. For instance data_type constexpr* output_type does not in fact mean "pointer to constant data_type", it means "constant pointer to data_type". For instance, check out this example: https://godbolt.org/z/o4YaEhrsE.

cpp/src/reshape/byte_cast.cu

vyasr · 2023-04-12T23:21:53Z

cpp/src/reshape/byte_cast.cu

+    auto chars_contents  = col_content.children[strings_column_view::chars_column_index]->release();
+    auto const num_chars = chars_contents.data->size();
+    auto uint8_col       = std::make_unique<column>(
+      output_type, num_chars, std::move(*(chars_contents.data)), rmm::device_buffer{}, 0);


Nice this section is cleaner now without the old null mask clutter.

ttnghia · 2023-04-13T17:46:24Z

/merge

Purge nonempty nulls from byte_cast list outputs.

a636b09

bdice added bug Something isn't working non-breaking Non-breaking change labels Oct 21, 2022

bdice self-assigned this Oct 21, 2022

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 21, 2022

bdice added 3 commits October 21, 2022 15:45

Style fix.

517e5d2

Fix purging logic.

835a0f4

Merge remote-tracking branch 'upstream/branch-22.12' into fix/byte-ca…

d18c557

…st-sanitized

bdice marked this pull request as ready for review October 28, 2022 00:44

bdice requested a review from a team as a code owner October 28, 2022 00:44

bdice requested review from hyperbolic2346 and davidwendt October 28, 2022 00:44

karthikeyann reviewed Oct 28, 2022

View reviewed changes

cpp/src/reshape/byte_cast.cu Show resolved Hide resolved

bdice changed the base branch from branch-22.12 to branch-23.02 November 17, 2022 17:21

hyperbolic2346 previously approved these changes Dec 30, 2022

View reviewed changes

davidwendt reviewed Jan 3, 2023

View reviewed changes

cpp/src/reshape/byte_cast.cu Outdated Show resolved Hide resolved

ttnghia reviewed Jan 12, 2023

View reviewed changes

cpp/src/reshape/byte_cast.cu Outdated Show resolved Hide resolved

bdice changed the base branch from branch-23.02 to branch-23.04 January 26, 2023 16:59

ttnghia reviewed Feb 15, 2023

View reviewed changes

cpp/src/reshape/byte_cast.cu Outdated Show resolved Hide resolved

ttnghia added 3 - Ready for Review Ready for review by team and removed conda labels Apr 8, 2023

ttnghia marked this pull request as ready for review April 8, 2023 02:00

hyperbolic2346 requested changes Apr 9, 2023

View reviewed changes

ttnghia added 2 commits April 9, 2023 13:35

Cleanup

464392f

Rename variables

1aa4c3a

davidwendt requested changes Apr 9, 2023

View reviewed changes

cpp/tests/reshape/byte_cast_tests.cpp Outdated Show resolved Hide resolved

ttnghia and others added 3 commits April 9, 2023 18:01

Update cpp/tests/reshape/byte_cast_tests.cpp

7d6dc94

Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>

Add empty/all-nulls tests

22c5c16

Fix return type when empty input

06f2d84

ttnghia and others added 2 commits April 9, 2023 21:02

Add more assert statements

7443a0c

Merge branch 'branch-23.06' into fix/byte-cast-sanitized

8318c2a

ttnghia and others added 4 commits April 12, 2023 06:31

Merge branch 'branch-23.06' into fix/byte-cast-sanitized

2c95f81

Merge remote-tracking branch 'bdice/fix/byte-cast-sanitized' into fix…

7455766

…/byte-cast-sanitized

Merge branch 'branch-23.06' into fix/byte-cast-sanitized

ed56341

Implement cudf::lists::detail::make_all_nulls_lists_column and use …

cf486fe

…the column factory methods

ttnghia force-pushed the fix/byte-cast-sanitized branch from 194520f to cf486fe Compare April 12, 2023 13:46

Define output_type and use it instead of hard code type

2562d0d

ttnghia requested review from hyperbolic2346 and davidwendt April 12, 2023 13:56

davidwendt approved these changes Apr 12, 2023

View reviewed changes

Merge branch 'branch-23.06' into fix/byte-cast-sanitized

79a12c2

vyasr approved these changes Apr 12, 2023

View reviewed changes

vyasr and others added 2 commits April 12, 2023 16:54

Merge branch 'branch-23.06' into fix/byte-cast-sanitized

94a0b6b

Fix constexpr position

92c9dfb

hyperbolic2346 approved these changes Apr 13, 2023

View reviewed changes

rapids-bot bot merged commit 4b34831 into rapidsai:branch-23.06 Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purge nonempty nulls from byte_cast list outputs. #11971

Purge nonempty nulls from byte_cast list outputs. #11971

bdice commented Oct 21, 2022 •

edited by ttnghia

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading

ttnghia commented Oct 28, 2022

hyperbolic2346 commented Oct 28, 2022

bdice commented Oct 28, 2022

ttnghia commented Dec 22, 2022

hyperbolic2346 left a comment

hyperbolic2346 Apr 9, 2023

ttnghia Apr 9, 2023

davidwendt left a comment

ttnghia commented Apr 10, 2023

ttnghia commented Apr 10, 2023

davidwendt left a comment

vyasr left a comment

vyasr Apr 12, 2023

vyasr Apr 12, 2023

ttnghia commented Apr 13, 2023

Purge nonempty nulls from byte_cast list outputs. #11971

Purge nonempty nulls from byte_cast list outputs. #11971

Conversation

bdice commented Oct 21, 2022 • edited by ttnghia Loading

Description

Checklist

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

ttnghia commented Oct 28, 2022

hyperbolic2346 commented Oct 28, 2022

bdice commented Oct 28, 2022

ttnghia commented Dec 22, 2022

hyperbolic2346 left a comment

Choose a reason for hiding this comment

hyperbolic2346 Apr 9, 2023

Choose a reason for hiding this comment

ttnghia Apr 9, 2023

Choose a reason for hiding this comment

davidwendt left a comment

Choose a reason for hiding this comment

ttnghia commented Apr 10, 2023

ttnghia commented Apr 10, 2023

davidwendt left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

vyasr Apr 12, 2023

Choose a reason for hiding this comment

vyasr Apr 12, 2023

Choose a reason for hiding this comment

ttnghia commented Apr 13, 2023

bdice commented Oct 21, 2022 •

edited by ttnghia

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading