Refactor Parquet kernel_error #14464
Conversation
vuule
added
improvement
cpp/src/io/parquet/error.hpp
Outdated
| @@ -32,8 +32,12 @@ namespace cudf::io::parquet { | |||
| * the object's lifetime. | |||
| */ | |||
| class kernel_error { | |||
| public: | |||
| using error_type = uint32_t; | |||
There was a problem hiding this comment.
I'm trying to come up with a consistent API. We currently expose data() and value(), i.e. the device_scalar API. If that does not change, we should probably stay consistent and declare value_type and pointer aliases.
https://docs.rapids.ai/api/librmm/stable/classrmm_1_1device__scalar.html
|
/ok to test |
| @@ -426,7 +426,7 @@ __global__ void __launch_bounds__(decode_block_size) | |||
| device_span<ColumnChunkDesc const> chunks, | |||
| size_t min_row, | |||
| size_t num_rows, | |||
| int32_t* error_code) | |||
| kernel_error::pointer error_code) | |||
There was a problem hiding this comment.
This looks awkward IMO. I feel much better with kernel_error::value_type*. Type aliasing a pointer to take away the * character makes it more error-prone as we can overlook and ignore the fact that this is a pointer.
There was a problem hiding this comment.
I think it's fine. pointer is a pretty standard naming.
There was a problem hiding this comment.
And is consistent with the device_scalar interface, as @vuule pointed out above.
|
/merge |
Description
While reviewing #14453 it became clear that the typing for error codes was inconsistent with their use as bitmasks. This PR changes the typing of
kernel_error::error_codeto unsigned, and adds type aliases to make any future changes tokernel_erroreasier to implement.Checklist