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

Fix Doxygen warnings in table header files #10964

Merged
merged 8 commits into from
Jun 2, 2022
Merged

Fix Doxygen warnings in table header files #10964

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
202c2f2
add missing docs in table headers
karthikeyann May 25, 2022
e8abed0
Apply suggestions from code review
karthikeyann May 27, 2022
5840fa9
address review comments
karthikeyann May 28, 2022
41f1e8c
Apply suggestions from code review
karthikeyann Jun 1, 2022
d7b715a
newline style fix
karthikeyann Jun 1, 2022
cbab55c
Merge branch 'branch-22.08' of https://github.com/rapidsai/cudf into …
karthikeyann Jun 1, 2022
ffbb181
Update cpp/include/cudf/table/table_view.hpp
karthikeyann Jun 1, 2022
7e2fe00
Update cpp/include/cudf/table/table_view.hpp
karthikeyann Jun 1, 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
127 changes: 102 additions & 25 deletions cpp/include/cudf/table/experimental/row_operators.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ namespace experimental {
*/
template <cudf::type_id t>
struct dispatch_void_if_nested {
/// The type to dispatch to if the type is nested
using type = std::conditional_t<cudf::is_nested(data_type(t)), void, id_to_type<t>>;
};

Expand Down Expand Up @@ -95,11 +96,17 @@ struct strong_index_iterator : public thrust::iterator_facade<strong_index_itera
thrust::random_access_traversal_tag,
Index,
Underlying> {
using super_t = thrust::iterator_adaptor<strong_index_iterator<Index>, Index>;
using super_t =
thrust::iterator_adaptor<strong_index_iterator<Index>, Index>; ///< The base class

/**
* @brief Constructs a strong index iterator
*
* @param n The beginning index
*/
explicit constexpr strong_index_iterator(Underlying n) : begin{n} {}

friend class thrust::iterator_core_access;
friend class thrust::iterator_core_access; ///< Allow access to the base class

private:
__device__ constexpr void increment() { ++begin; }
Expand Down Expand Up @@ -154,8 +161,8 @@ namespace lexicographic {
*/
template <typename Nullate>
class device_row_comparator {
friend class self_comparator;
friend class two_table_comparator;
friend class self_comparator; ///< Allow self_comparator to access private members
friend class two_table_comparator; ///< Allow two_table_comparator to access private members

/**
* @brief Construct a function object for performing a lexicographic
Expand Down Expand Up @@ -378,7 +385,12 @@ template <typename Comparator>
using less_equivalent_comparator =
weak_ordering_comparator_impl<Comparator, weak_ordering::LESS, weak_ordering::EQUIVALENT>;

/**
* @brief Preprocessed table for use with lexicographical comparison
*
*/
struct preprocessed_table {
/// Type of table device view owner for the preprocessed table.
using table_device_view_owner =
std::invoke_result_t<decltype(table_device_view::create), table_view, rmm::cuda_stream_view>;

Expand All @@ -396,15 +408,16 @@ struct preprocessed_table {
* values compare to all other for every column. If it is nullptr, then null precedence would be
* `null_order::BEFORE` for all columns.
* @param stream The stream to launch kernels and h->d copies on while preprocessing.
* @return A preprocessed table as a shared pointer
karthikeyann marked this conversation as resolved.
Show resolved Hide resolved
*/
static std::shared_ptr<preprocessed_table> create(table_view const& table,
host_span<order const> column_order,
host_span<null_order const> null_precedence,
rmm::cuda_stream_view stream);

private:
friend class self_comparator;
friend class two_table_comparator;
friend class self_comparator; ///< Allow self_comparator to access private members
friend class two_table_comparator; ///< Allow two_table_comparator to access private members

preprocessed_table(table_device_view_owner&& table,
rmm::device_uvector<order>&& column_order,
Expand Down Expand Up @@ -525,6 +538,8 @@ class self_comparator {
* `F(i,j)` returns true if and only if row `i` compares lexicographically less than row `j`.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
vyasr marked this conversation as resolved.
Show resolved Hide resolved
* @param nullate Indicates if either input column contains nulls
* @return A binary callable object
*/
template <typename Nullate>
less_comparator<device_row_comparator<Nullate>> device_comparator(Nullate nullate = {}) const
Expand All @@ -537,6 +552,7 @@ class self_comparator {
std::shared_ptr<preprocessed_table> d_t;
};

// @cond
template <typename Comparator>
struct strong_index_comparator_adapter {
__device__ constexpr weak_ordering operator()(lhs_index_type const lhs_index,
Expand All @@ -563,6 +579,7 @@ struct strong_index_comparator_adapter {

Comparator const comparator;
};
// @endcond

/**
* @brief An owning object that can be used to lexicographically compare rows of two different
Expand Down Expand Up @@ -636,6 +653,8 @@ class two_table_comparator {
* `j` of the left table.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
* @param nullate Indicates if either input column contains nulls.
* @return A binary callable object
*/
template <typename Nullate>
less_comparator<strong_index_comparator_adapter<device_row_comparator<Nullate>>>
Expand All @@ -662,11 +681,15 @@ class row_hasher;
}

namespace equality {

/**
* @brief Comparator for performing equality comparison between the rows of two tables.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
*/
template <typename Nullate>
class device_row_comparator {
friend class self_comparator;
friend class two_table_comparator;
friend class self_comparator; ///< Allow self_comparator to access private members
friend class two_table_comparator; ///< Allow two_table_comparator to access private members

public:
/**
Expand Down Expand Up @@ -860,24 +883,30 @@ class device_row_comparator {
null_equality const nulls_are_equal;
};

/**
* @brief Preprocess table for use with row equality comparison or row hashing
*
* Sets up the table for use with row equality comparison or row hashing. The resulting
* preprocessed table can be passed to the constructor of `equality::self_comparator` to
* avoid preprocessing again.
*
*/
struct preprocessed_table {
/**
* @brief Preprocess table for use with row equality comparison or row hashing
*
* Sets up the table for use with row equality comparison or row hashing. The resulting
* preprocessed table can be passed to the constructor of `equality::self_comparator` to
* avoid preprocessing again.
* @brief Factory to construct preprocessed_table for use with
* row equality comparison or row hashing
*
vyasr marked this conversation as resolved.
Show resolved Hide resolved
* @param table The table to preprocess
* @param stream The cuda stream to use while preprocessing.
* @return A preprocessed table as shared pointer
*/
static std::shared_ptr<preprocessed_table> create(table_view const& table,
rmm::cuda_stream_view stream);

private:
friend class self_comparator;
friend class two_table_comparator;
friend class hash::row_hasher;
friend class self_comparator; ///< Allow self_comparator to access private members
friend class two_table_comparator; ///< Allow two_table_comparator to access private members
friend class hash::row_hasher; ///< Allow row_hasher to access private members

using table_device_view_owner =
std::invoke_result_t<decltype(table_device_view::create), table_view, rmm::cuda_stream_view>;
Expand All @@ -899,6 +928,10 @@ struct preprocessed_table {
std::vector<rmm::device_buffer> _null_buffers;
};

/**
* @brief Comparator for performing equality comparisons between two rows of the same table.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Does doxygen style require this extra line after the brief even when there are no additional lines afterwards, or is that something that you are doing? It seems like an unnecessary extra line to me, but maybe it's just what we need to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doxygen does not require it. The vscode doxygen extension inserts this line by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think I saw a few other places where there were param/return docs after the brief, and there was no space in between. It would be nice to make those consistent, but if doxygen doesn't require it then I wouldn't waste time on that (except when adding new lines) until after these PRs are all set.

Copy link
Contributor Author

@karthikeyann karthikeyann May 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, new lines are for readability. I would prefer to leave it as the doxygen extension generates.
it does not affect generated docs.

I will create a single PR with other changes like removing dots at end of @param, order of https://github.com/rapidsai/cudf/blob/branch-22.06/cpp/docs/DOCUMENTATION.md#function-parameters , //!< to ///<.

do you suggest to add "compulsory newline after @brief" in that PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally yes, I like the newline after brief. Not only is it easier to read, it makes it very obvious when the brief is not actually very brief if there's an extremely long line.

*/
class self_comparator {
public:
/**
Expand Down Expand Up @@ -932,11 +965,14 @@ class self_comparator {
*
* `F(i,j)` returns true if and only if row `i` compares equal to row `j`.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
* @tparam Nullate A cudf::nullate type describing whether to check for nulls
vyasr marked this conversation as resolved.
Show resolved Hide resolved
* @param nullate Indicates if either input column contains nulls
* @param nulls_are_equal Indicates if nulls are equal
* @return A binary callable object
*/
template <typename Nullate>
device_row_comparator<Nullate> device_comparator(
Nullate nullate = {}, null_equality nulls_are_equal = null_equality::EQUAL) const
Nullate nullate = {}, null_equality nulls_are_equal = n`ull_equality::EQUAL) const
karthikeyann marked this conversation as resolved.
Show resolved Hide resolved
{
return device_row_comparator(nullate, *d_t, *d_t, nulls_are_equal);
}
Expand All @@ -945,6 +981,7 @@ class self_comparator {
std::shared_ptr<preprocessed_table> d_t;
};

// @cond
template <typename Comparator>
struct strong_index_comparator_adapter {
__device__ constexpr bool operator()(lhs_index_type const lhs_index,
Expand All @@ -962,6 +999,7 @@ struct strong_index_comparator_adapter {

Comparator const comparator;
};
// @endcond

/**
* @brief An owning object that can be used to equality compare rows of two different tables.
Expand Down Expand Up @@ -1022,7 +1060,10 @@ class two_table_comparator {
* Similarly, `F(rhs_index_type i, lhs_index_type j)` returns true if and only if row `i` of the
* right table compares equal to row `j` of the left table.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
* @tparam Nullate A cudf::nullate type describing whether to check for nulls
* @param nullate Indicates if either input column contains nulls
* @param nulls_are_equal Indicates if nulls are equal
* @return A binary callable object
*/
template <typename Nullate>
auto device_comparator(Nullate nullate = {},
Expand Down Expand Up @@ -1050,6 +1091,13 @@ namespace hash {
template <template <typename> class hash_function, typename Nullate>
class element_hasher {
public:
/**
* @brief Constructs an element_hasher object.
*
* @param nulls Indicates whether to check for nulls
* @param seed The seed to use for the hash function
* @param null_hash The hash value to use for nulls
*/
__device__ element_hasher(
Nullate nulls,
uint32_t seed = DEFAULT_HASH_SEED,
Expand All @@ -1058,6 +1106,14 @@ class element_hasher {
{
}

/**
* @brief Returns the hash value of the given element.
*
* @tparam T The type of the element to hash
* @param col The column to hash
* @param row_index The index of the row to hash
* @return The hash value of the given element
*/
template <typename T, CUDF_ENABLE_IF(column_device_view::has_element_accessor<T>())>
__device__ hash_value_type operator()(column_device_view const& col,
size_type row_index) const noexcept
Expand All @@ -1066,16 +1122,24 @@ class element_hasher {
return hash_function<T>{_seed}(col.element<T>(row_index));
}

/**
* @brief Returns the hash value of the given element.
*
* @tparam T The type of the element to hash
* @param col The column to hash
* @param row_index The index of the row to hash
* @return The hash value of the given element
*/
template <typename T, CUDF_ENABLE_IF(not column_device_view::has_element_accessor<T>())>
__device__ hash_value_type operator()(column_device_view const& col,
size_type row_index) const noexcept
{
CUDF_UNREACHABLE("Unsupported type in hash.");
}

uint32_t _seed;
hash_value_type _null_hash;
Nullate _check_nulls;
uint32_t _seed; ///< The seed to use for hashing
hash_value_type _null_hash; ///< Hash value to use for null elements
Nullate _check_nulls; ///< Whether to check for nulls
};

/**
Expand All @@ -1086,11 +1150,17 @@ class element_hasher {
*/
template <template <typename> class hash_function, typename Nullate>
class device_row_hasher {
friend class row_hasher;
friend class row_hasher; ///< Allow row_hasher to access private members.

public:
device_row_hasher() = delete;

/**
* @brief Return the hash value of a row in the given table.
*
* @param row_index The row index to compute the hash value of
* @return The hash value of the row
*/
__device__ auto operator()(size_type row_index) const noexcept
{
auto it = thrust::make_transform_iterator(_table.begin(), [=](auto const& column) {
Expand Down Expand Up @@ -1187,6 +1257,10 @@ class device_row_hasher {
// type and are interchangeable.
using preprocessed_table = row::equality::preprocessed_table;

/**
* @brief Computes the hash value of a row in the given table.
*
*/
class row_hasher {
public:
/**
Expand Down Expand Up @@ -1219,7 +1293,10 @@ class row_hasher {
*
* `F(i)` returns the hash of row i.
*
* @tparam Nullate A cudf::nullate type describing whether to check for nulls.
* @tparam Nullate A cudf::nullate type describing whether to check for nulls
* @param nullate Indicates if either input column contains nulls
* @param seed The seed to use for the hash function
* @return A hash operator to use on the device
*/
template <template <typename> class hash_function = detail::default_hash, typename Nullate>
device_row_hasher<hash_function, Nullate> device_hasher(Nullate nullate = {},
Expand Down
Loading