Disable column_view data accessors for unsupported types by jrhemstad · Pull Request #7725 · NVIDIA/cudf

jrhemstad · 2021-03-25T16:22:49Z

column_view provides data accessors like column_view::data<T> and column_view::begin<T>. These accessors are only valid for fixed-width primitive types that can be constructed by simply casting the underlying void* to T*.

However, the accessors never actually enforced this rule, e.g., column_view::data<struct_view> should fail to compile.

This PR disables these accessors for invalid types.

This uncovered a number of places that were erroneously instantiating column_view accessors, which would lead to silent failures (e.g., scatter was failing silently for struct columns).

I added a few new things to aid me in this effort:

CUDF_ENABLE_IF macro to make it easier to SFINAE.
is_rep_layout_compatbile<T>() to identify types that are layout compatible with their rep (e.g., duration_ns is layout compatible with its int64_t rep. The decimal32 type is not layout compatible with it's int32_t rep).
column_device_view::has_element_accessor<T>() identifies if column_device_view::element<T>() has a valid overload.

Scatter was erroneously instantiating things like column_view::begin<struct_view>(). Updated the specializations to ensure that unsupported types throw an exception.

list scatter was incorrectly instantiating column_view::data<decimal32>. This should be fixed by just using the storage type dispatcher instead.

is_rep_layout_compatible is used to describe types that are safe to use with column_view accessors.

jrhemstad · 2021-03-25T21:00:14Z

The following tests are now failing as a result of this change:

	 19 - FIXED_POINT_TEST (Failed)
	 20 - UNARY_TEST (Failed)
	 21 - ROUND_TEST (Failed)
	 22 - BINARY_TEST (Failed)

I suspect this is all due to the erroneous code paths taken in the JIT code.

jrhemstad · 2021-03-26T03:17:38Z

So the fixed_point_binary_operation ultimately calls into the "regular" binary_operation API here:
https://github.com/rapidsai/cudf/blob/20509d0b75cf33e0597325fe17426766ed366eb3/cpp/src/binaryop/binaryop.cpp#L620
which calls into: https://github.com/rapidsai/cudf/blob/20509d0b75cf33e0597325fe17426766ed366eb3/cpp/src/binaryop/binaryop.cpp#L226-L230
which calls get_data_ptr<decimal32/64>
https://github.com/rapidsai/cudf/blob/20509d0b75cf33e0597325fe17426766ed366eb3/cpp/src/binaryop/binaryop.cpp#L248-L251
and in my PR, that lands you here: https://github.com/rapidsai/cudf/blob/f99318eba0b09149fbed5ddcee35a6f98581b0b2/cpp/src/jit/type.cpp#L36-L41

The binop API only knows how to access data through the column_view::data<T>(), but that doesn't work for decimalXX. It needs to go through the column_device_view::element<T> or operate on the device_storage_type<T>.

get_data_ptr was erroneously invoking column_view::data<decimal32/64>(). As it happens, this ended up being okay when the column did not have any offset as column_view::data<int32>() and column_view::data<decimal32>() will return the same initial pointer when the offset is zero. The problem occurs when you attempt to advance this pointer, or if the column had an offset (which internally will advance the pointer).

jrhemstad · 2021-03-26T14:10:38Z

The fix in a4788a0 was to just use dispatch_storage_type.

codecov · 2021-03-26T16:46:02Z

Codecov Report

Merging #7725 (4571a03) into branch-0.19 (7871e7a) will increase coverage by 0.65%.
The diff coverage is n/a.

❗ Current head 4571a03 differs from pull request most recent head f92d462. Consider uploading reports for the commit f92d462 to get more accurate results

@@               Coverage Diff               @@
##           branch-0.19    #7725      +/-   ##
===============================================
+ Coverage        81.86%   82.52%   +0.65%     
===============================================
  Files              101      101              
  Lines            16884    17458     +574     
===============================================
+ Hits             13822    14407     +585     
+ Misses            3062     3051      -11

Impacted Files	Coverage Δ
python/cudf/cudf/utils/gpu_utils.py	`53.65% <0.00%> (-4.88%)`	⬇️
python/cudf/cudf/core/column/lists.py	`87.68% <0.00%> (-3.72%)`	⬇️
python/cudf/cudf/core/column/decimal.py	`92.95% <0.00%> (-1.92%)`	⬇️
python/cudf/cudf/core/abc.py	`87.23% <0.00%> (-1.14%)`	⬇️
python/cudf/cudf/core/column/numerical.py	`94.83% <0.00%> (-0.20%)`	⬇️
python/cudf/cudf/core/column/column.py	`87.61% <0.00%> (-0.15%)`	⬇️
python/cudf/cudf/utils/utils.py	`85.36% <0.00%> (-0.07%)`	⬇️
python/cudf/cudf/io/feather.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/utils/ioutils.py	`78.71% <0.00%> (ø)`
python/cudf/cudf/comm/serialize.py	`0.00% <0.00%> (ø)`
... and 45 more

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 e73fff0...f92d462. Read the comment docs.

This test should not be testing the kernel directly.

jrhemstad · 2021-03-29T17:42:43Z

I'm calling this "non-breaking" in that it doesn't change any external APIs. It technically could change externally visible behavior where some code will throw where it used to "work", but those use cases were broken code to begin with.

mythrocks · 2021-03-29T17:44:06Z

 #include <type_traits>
 #include "../fixture/benchmark_fixture.hpp"
 #include "../synchronization/synchronization.hpp"
+#include "cudf/utilities/traits.hpp"


Nitpick: Aren't we enforcing angle brackets <> for includes that aren't in the current directory?

Ah, something in my vscode auto includes headers and uses " instead of <>. I'll need to see if I can change that.

It's clangd IWYU.

cwharris

nice!

vyasr

Looks good to me. One request (if you don't want to do it then just re-request my review and I can approve): it seems like there are a number of places in this PR that are still using the raw std::enable_if_t<...>* = nullptr to disable implementations (where it's using an non-type template parameter that's unused AFAICT). Since any code patterns introduced here will inevitably be copied into other places and your new CUDF_ENABLE_IF macro is a bit cleaner, it would be nice to use it consistently everywhere as much as possible.

Co-authored-by: Paul Taylor <paul.e.taylor@me.com>

jrhemstad · 2021-03-29T18:55:39Z

Looks good to me. One request (if you don't want to do it then just re-request my review and I can approve): it seems like there are a number of places in this PR that are still using the raw std::enable_if_t<...>* = nullptr to disable implementations (where it's using an non-type template parameter that's unused AFAICT). Since any code patterns introduced here will inevitably be copied into other places and your new CUDF_ENABLE_IF macro is a bit cleaner, it would be nice to use it consistently everywhere as much as possible.

Yeah, I got half way through and got sick of typing, so I made the macro. I was hoping reviewers would just let me be lazy :) But I agree, I'll go make stuff in this PR consistent.

codereport

Looks awesome! Small header things but not a big deal.

codereport · 2021-03-29T20:24:59Z

 #include <cudf/scalar/scalar.hpp>

 #include <rmm/cuda_stream_view.hpp>
+#include "cudf/utilities/traits.hpp"


Suggested change

#include "cudf/utilities/traits.hpp"

#include <cudf/utilities/traits.hpp>

… into disable_column_view_begin

Co-authored-by: Conor Hoekstra <36027403+codereport@users.noreply.github.com>

… into disable_column_view_begin

vyasr

Looks good!

harrism · 2021-03-30T00:57:59Z

@gpucibot merge

In #7725 I updated some of the fall through error cases to use old, [C-style ellipsis variadics ](https://github.com/jrhemstad/cudf/blob/f92d4626c7a4296075c5aa6240d6e3b88049abe6/cpp/include/cudf/detail/gather.cuh#L145 )since we don't care about the parameters in the error case. Turns out C++ forbids passing "non-POD" types through this kind of variadic ellipsis, which ends up generating a compiler warning: > warning: non-POD class type passed through ellipsis Authors: - Jake Hemstad (https://github.com/jrhemstad) Approvers: - David Wendt (https://github.com/davidwendt) - Nghia Truong (https://github.com/ttnghia) - Mark Harris (https://github.com/harrism) URL: #7781

jrhemstad added 2 commits March 25, 2021 11:16

Disable column_view data accessors for non fixed-width types.

d73219e

Fix scatter code path specialization.

2f8e0dc

Scatter was erroneously instantiating things like column_view::begin<struct_view>(). Updated the specializations to ensure that unsupported types throw an exception.

github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 25, 2021

jrhemstad added 9 commits March 25, 2021 11:23

Clarify that the static_assert will be removed in favor of enable_if.

759bf6d

Use dispatch_storage_type in list scatter.

3df7685

list scatter was incorrectly instantiating column_view::data<decimal32>. This should be fixed by just using the storage type dispatcher instead.

Add is_rep_layout_compatible trait.

c2604cb

is_rep_layout_compatible is used to describe types that are safe to use with column_view accessors.

Provide error path for dlpack.

5e6311c

Error path for from_arrow.

5f818ad

Error path for to_arrow.

55ff95b

Error path for JIT.

24d437f

Add error path for merge.

608459f

Modify specialization of gather code paths to add error path.

1453153

Format.

f99318e

jrhemstad added non-breaking Non-breaking change bug Something isn't working labels Mar 25, 2021

jrhemstad requested a review from codereport March 25, 2021 21:09

jrhemstad added 9 commits March 26, 2021 15:16

Error path for copy_range.

95d55ea

Use is_rep_layout_compatible instead of is_fixed_width

98072b0

Error path for fill.

0238112

Error path for copy test.

978af0b

This test should not be testing the kernel directly.

Update specializations and error path for copy_if_else.

9516e09

Disable column_device_view accessors for invalid types.

fb27cf1

Add error path for type dispatcher benchmark.

400dbaf

Error path in row_hasher for types that don't have an element accessor.

dcd75e2

format.

1134362

jrhemstad added 2 commits March 29, 2021 12:06

SFINAE remaining device_view accessors.

3d08a57

format.

e896a26

jrhemstad marked this pull request as ready for review March 29, 2021 17:39

jrhemstad requested a review from a team as a code owner March 29, 2021 17:39

jrhemstad requested a review from trxcllnt March 29, 2021 17:39

trxcllnt reviewed Mar 29, 2021

View reviewed changes

Comment thread cpp/benchmarks/type_dispatcher/type_dispatcher_benchmark.cu Outdated

mythrocks reviewed Mar 29, 2021

View reviewed changes

Comment thread cpp/include/cudf/column/column_device_view.cuh Outdated

cwharris approved these changes Mar 29, 2021

View reviewed changes

vyasr requested changes Mar 29, 2021

View reviewed changes

Update cpp/benchmarks/type_dispatcher/type_dispatcher_benchmark.cu

3a6405d

Co-authored-by: Paul Taylor <paul.e.taylor@me.com>

mythrocks reviewed Mar 29, 2021

View reviewed changes

Comment thread cpp/include/cudf/column/column_device_view.cuh

codereport approved these changes Mar 29, 2021

View reviewed changes

jrhemstad and others added 6 commits March 29, 2021 16:31

Use the CUDF_ENABLE_IF macro everywhere.

5982248

Merge branch 'disable_column_view_begin' of github.com:jrhemstad/cudf…

d6ce9f0

… into disable_column_view_begin

Format.

7e9abd4

Remove extra spaces.

6a56c02

Apply suggestions from code review

d20e9ae

Co-authored-by: Conor Hoekstra <36027403+codereport@users.noreply.github.com>

Update cpp/src/copying/copy.cu

a057e71

Co-authored-by: Conor Hoekstra <36027403+codereport@users.noreply.github.com>

jrhemstad requested a review from vyasr March 29, 2021 21:38

jrhemstad added 2 commits March 29, 2021 16:46

Merge branch 'disable_column_view_begin' of github.com:jrhemstad/cudf…

85c8c11

… into disable_column_view_begin

Format.

f92d462

vyasr approved these changes Mar 29, 2021

View reviewed changes

harrism added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Mar 30, 2021

rapids-bot Bot merged commit 2d24a9b into NVIDIA:branch-0.19 Mar 30, 2021

jrhemstad mentioned this pull request Mar 31, 2021

Fix compiler warning about non-POD types passed through ellipsis #7781

Merged

	#include "cudf/utilities/traits.hpp"
	#include <cudf/utilities/traits.hpp>

Uh oh!

Conversation

jrhemstad commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrhemstad commented Mar 25, 2021

Uh oh!

jrhemstad commented Mar 26, 2021

Uh oh!

jrhemstad commented Mar 26, 2021

Uh oh!

codecov Bot commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jrhemstad commented Mar 29, 2021

Uh oh!

Uh oh!

mythrocks Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhemstad Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

harrism Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cwharris left a comment

Choose a reason for hiding this comment

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrhemstad commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codereport left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codereport Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

harrism commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jrhemstad commented Mar 25, 2021 •

edited

Loading

codecov Bot commented Mar 26, 2021 •

edited

Loading

mythrocks Mar 29, 2021 •

edited

Loading

jrhemstad commented Mar 29, 2021 •

edited

Loading