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.

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

[REVIEW] Introduces make_optional_iterator for nullable column and scalars #7772

Merged
Merged
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
Prev Previous commit
Next Next commit
DYNAMIC optional iterators require users to specify has_nulls
robertmaynard committed Apr 8, 2021
commit ce6d8647a9a1ec888f38e954c2cd92d3eded219a
26 changes: 0 additions & 26 deletions cpp/include/cudf/column/column_device_view.cuh
Original file line number Diff line number Diff line change
@@ -549,20 +549,12 @@ class alignas(16) column_device_view : public detail::column_device_view_base {
* the user has stated nulls exist
* @throws cudf::logic_error if column datatype and Element type mismatch.
*/
/**@{*/
template <typename T, CUDF_ENABLE_IF(column_device_view::has_element_accessor<T>())>
robertmaynard marked this conversation as resolved.
Show resolved Hide resolved
auto optional_begin(contains_nulls::DYNAMIC, bool has_nulls) const
{
return const_optional_iterator<T, contains_nulls::DYNAMIC>{
count_it{0}, detail::optional_accessor<T, contains_nulls::DYNAMIC>{*this, has_nulls}};
}
template <typename T, CUDF_ENABLE_IF(column_device_view::has_element_accessor<T>())>
auto optional_begin(contains_nulls::DYNAMIC) const
{
return const_optional_iterator<T, contains_nulls::DYNAMIC>{
count_it{0}, detail::optional_accessor<T, contains_nulls::DYNAMIC>{*this}};
}
/**@}*/

/**
* @brief Return an optional iterator to the first element of the column.
@@ -682,20 +674,12 @@ class alignas(16) column_device_view : public detail::column_device_view_base {
* the user has stated nulls exist
* @throws cudf::logic_error if column datatype and Element type mismatch.
*/
/**@{*/
template <typename T, CUDF_ENABLE_IF(column_device_view::has_element_accessor<T>())>
auto optional_end(contains_nulls::DYNAMIC, bool has_nulls) const
{
return const_optional_iterator<T, contains_nulls::DYNAMIC>{
count_it{size()}, detail::optional_accessor<T, contains_nulls::DYNAMIC>{*this, has_nulls}};
}
template <typename T, CUDF_ENABLE_IF(column_device_view::has_element_accessor<T>())>
auto optional_end(contains_nulls::DYNAMIC) const
{
return const_optional_iterator<T, contains_nulls::DYNAMIC>{
count_it{size()}, detail::optional_accessor<T, contains_nulls::DYNAMIC>{*this}};
}
/**@}*/

/**
* @brief Return an optional iterator to the element following the last element of
@@ -1232,16 +1216,6 @@ struct optional_accessor<T, contains_nulls::DYNAMIC> {
column_device_view const col; ///< column view of column in device
bool has_nulls;

/**
* @brief constructor
* @param[in] _col column device view of cudf column
*/
optional_accessor(column_device_view const& _col)
: col{_col}, has_nulls{_col.nullable()}
{
CUDF_EXPECTS(type_id_matches_device_storage_type<T>(col.type().id()), "the data type mismatch");
}

/**
* @brief constructor
* @param[in] _col column device view of cudf column
25 changes: 0 additions & 25 deletions cpp/include/cudf/detail/iterator.cuh
Original file line number Diff line number Diff line change
@@ -188,21 +188,13 @@ auto make_null_replacement_iterator(column_device_view const& column,
* @return auto Iterator that returns valid column elements, and validity of the
* element in a thrust::optional
*/
/**@{*/
template <typename Element>
auto make_optional_iterator(column_device_view const& column,
contains_nulls::DYNAMIC,
bool has_nulls)
{
return column.optional_begin<Element>(contains_nulls::DYNAMIC{}, has_nulls);
}
template <typename Element>
auto make_optional_iterator(column_device_view const& column,
contains_nulls::DYNAMIC)
{
return column.optional_begin<Element>(contains_nulls::DYNAMIC{});
}
/**@}*/

/**
* @brief Constructs an optional iterator over a column's values and its validity.
@@ -470,11 +462,6 @@ struct scalar_optional_accessor<Element, cudf::contains_nulls::DYNAMIC>
using value_type = thrust::optional<Element>;
bool has_nulls;

scalar_optional_accessor(scalar const& scalar_value)
: scalar_value_accessor<Element>(scalar_value), has_nulls{scalar_value.is_valid()}
{
}

scalar_optional_accessor(scalar const& scalar_value, bool with_nulls)
: scalar_value_accessor<Element>(scalar_value), has_nulls{with_nulls}
{
@@ -607,7 +594,6 @@ struct scalar_representation_pair_accessor : public scalar_value_accessor<Elemen
* @return auto Iterator that returns scalar elements, and validity of the
* element in a thrust::optional
*/
/**@{*/
template <typename Element>
auto inline make_optional_iterator(scalar const& scalar_value,
contains_nulls::DYNAMIC, bool has_nulls)
@@ -618,17 +604,6 @@ auto inline make_optional_iterator(scalar const& scalar_value,
thrust::make_constant_iterator<size_type>(0),
scalar_optional_accessor<Element, contains_nulls::DYNAMIC>{scalar_value, has_nulls});
}
template <typename Element>
auto inline make_optional_iterator(scalar const& scalar_value,
contains_nulls::DYNAMIC)
{
CUDF_EXPECTS(type_id_matches_device_storage_type<Element>(scalar_value.type().id()),
"the data type mismatch");
return thrust::make_transform_iterator(thrust::make_constant_iterator<size_type>(0),
scalar_optional_accessor<Element, contains_nulls::DYNAMIC>{
scalar_value});
}
/**@}*/


/**
3 changes: 0 additions & 3 deletions cpp/tests/iterator/optional_iterator_test.cu
Original file line number Diff line number Diff line change
@@ -187,9 +187,6 @@ TYPED_TEST(IteratorTest, null_optional_iterator)
this->iterator_test_thrust(optional_values,
d_col->optional_begin<T>(cudf::contains_nulls::DYNAMIC{}, true),
host_values.size());
this->iterator_test_thrust(optional_values,
d_col->optional_begin<T>(cudf::contains_nulls::DYNAMIC{}),
host_values.size());

this->iterator_test_thrust(
optional_values, d_col->optional_begin<T>(cudf::contains_nulls::YES{}), host_values.size());