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.

Sign up for GitHub

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

Jump to bottom

Convert byte_array_view to use std::byte #11424

Merged
merged 4 commits into from
Aug 3, 2022
Merged

Convert byte_array_view to use std::byte #11424

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3144ebd
converting byte_array_view to use std::byte
hyperbolic2346 Aug 1, 2022
e2bc8a8
Update cpp/include/cudf/utilities/traits.hpp
hyperbolic2346 Aug 2, 2022
15540bf
changing to drop const and volatile
hyperbolic2346 Aug 2, 2022
59218fd
Merge branch 'mwilson/byte_array_as_byte' of github.com:hyperbolic234…
hyperbolic2346 Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion cpp/include/cudf/utilities/traits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,19 @@ constexpr inline bool is_floating_point(data_type type)
return cudf::type_dispatcher(type, is_floating_point_impl{});
}

/**
* @brief Indicates whether `T` is a std::byte type.
*
* @param type The `data_type` to verify
hyperbolic2346 marked this conversation as resolved.
Show resolved Hide resolved
* @return true `type` is std::byte
* @return false `type` is not std::byte
*/
template <typename T>
constexpr inline bool is_byte()
{
return std::is_same_v<std::remove_const_t<T>, std::byte>;
Copy link
Contributor

@bdice bdice Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want a full decay, cv removal, reference removal, or just const removal? I’m not certain what choice is most appropriate for a trait like this

edit: removed suggestion for decay in favor of alternative remove_cv below

Copy link
Contributor

Choose a reason for hiding this comment

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

I would even go as far as claiming something like is_byte<volatile T&> should fail to compile, so this seems sensible to me.

Copy link
Contributor

@bdice bdice Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@upsj I’m not sure if I follow. Do you mean you prefer the current state of remove_const?

On further inquiry, the std::is_integral and std::is_floating_point traits return true for any cv-qualifiers but not references. We probably want to remove volatile qualifiers but not reference-ness. (Not that we use volatile often, but I would like to align trait behaviors with the standard where possible.)

Suggested change
return std::is_same_v<std::remove_const_t<T>, std::byte>;
return std::is_same_v<std::remove_cv_t<T>, std::byte>;

Copy link
Contributor

@upsj upsj Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah sorry, that was ambiguous. Yes, I believe a trait like is_byte should be rather restrictive, or are there use cases in cuDF where you need to call it on a decltype of an expression, or a volatile type? remove_cv seems more appropriate than decay though, I agree with staying consistent with the std library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove volatile because I wasn't sure the use-case where it was needed and I would rather have it fail to compile and force a decision at that point than to assume it would be ok. I'm ok with changing it to remove_cv_t though.

Copy link
Contributor

@bdice bdice Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

re: #11424 (comment)

@bdice: On further inquiry, the std::is_integral and std::is_floating_point traits return true for any cv-qualifiers but not references. We probably want to remove volatile qualifiers but not reference-ness.

@upsj: I agree with staying consistent with the std library

@hyperbolic2346 I think this is consensus among reviewers that remove_cv_t is a good change for consistency with std / <type_traits>.

}

/**
* @brief Indicates whether `T` is a Boolean type.
*
Expand Down Expand Up @@ -561,7 +574,8 @@ constexpr inline bool is_chrono(data_type type)
template <typename T>
constexpr bool is_rep_layout_compatible()
{
return cudf::is_numeric<T>() or cudf::is_chrono<T>() or cudf::is_boolean<T>();
return cudf::is_numeric<T>() or cudf::is_chrono<T>() or cudf::is_boolean<T>() or
cudf::is_byte<T>();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/io/statistics/byte_array_view.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ namespace cudf::io::statistics {
*/
class byte_array_view {
public:
using element_type = uint8_t const; ///< The type of the elements in the byte array
using element_type = std::byte const; ///< The type of the elements in the byte array

constexpr byte_array_view() noexcept {}
/**
Expand Down
7 changes: 4 additions & 3 deletions cpp/src/io/statistics/statistics.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ struct t_array_stats {
__host__ __device__ __forceinline__ operator ReturnType() { return ReturnType(ptr, length); }
};
using string_stats = t_array_stats<string_view, char>;
using byte_array_stats = t_array_stats<statistics::byte_array_view, uint8_t>;
using byte_array_view = statistics::byte_array_view;
using byte_array_stats = t_array_stats<byte_array_view, byte_array_view::element_type>;

union statistics_val {
string_stats str_val; //!< string columns
Expand Down Expand Up @@ -129,10 +130,10 @@ template <typename T, std::enable_if_t<std::is_same_v<T, statistics::byte_array_
__device__ T get_element(column_device_view const& col, uint32_t row)
{
using et = typename T::element_type;
size_type index = row + col.offset(); // account for this view's _offset
size_type const index = row + col.offset(); // account for this view's _offset
auto const* d_offsets = col.child(lists_column_view::offsets_column_index).data<offset_type>();
auto const* d_data = col.child(lists_column_view::child_column_index).data<et>();
offset_type offset = d_offsets[index];
auto const offset = d_offsets[index];
return T(d_data + offset, d_offsets[index + 1] - offset);
}

Expand Down