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

Add column constructor from device_uvector&& #11356

Merged
merged 10 commits into from
Aug 3, 2022
Merged

Add column constructor from device_uvector&& #11356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
746d8ef
add constructor
SrikarVanavasam Jul 26, 2022
2a2f26c
review feedback
SrikarVanavasam Jul 29, 2022
e130563
Add optional null mask and null count
SrikarVanavasam Aug 1, 2022
2f4a650
Add comment
SrikarVanavasam Aug 1, 2022
e511c6b
fix comment
SrikarVanavasam Aug 2, 2022
56f17bf
Initialize members in constructor body
SrikarVanavasam Aug 2, 2022
b7066b4
Merge branch 'column_constructor_from_device_uvector&&' of https://gi…
SrikarVanavasam Aug 2, 2022
64023d0
add static_cast
SrikarVanavasam Aug 3, 2022
dde6501
Error message
SrikarVanavasam Aug 3, 2022
5183cff
IIFE in member list
SrikarVanavasam Aug 3, 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: 16 additions & 0 deletions cpp/include/cudf/column/column.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include <rmm/cuda_stream_view.hpp>
#include <rmm/device_buffer.hpp>
#include <rmm/device_uvector.hpp>
#include <rmm/mr/device/per_device_resource.hpp>

#include <memory>
Expand Down Expand Up @@ -75,6 +76,21 @@ class column {
*/
column(column&& other) noexcept;

/**
* @brief Move the contents from `other` to create a new column.
SrikarVanavasam marked this conversation as resolved.
Show resolved Hide resolved
*
* After the move, `other.data() == NULL`
*
SrikarVanavasam marked this conversation as resolved.
Show resolved Hide resolved
* @param other The device_uvector whose contents will be moved into the new column.
*/
template <typename T>
column(rmm::device_uvector<T>&& other) noexcept
: _type{cudf::type_to_id<T>()},
_size{static_cast<cudf::size_type>(other.size())},
_data{other.release()}
SrikarVanavasam marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@jrhemstad jrhemstad Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This needs to be constrained in some way to prevent construction from a device_uvector<string_view> or device_uvector<list_view>.

It should probably be:

template <typename T, CUDF_ENABLE_IF(is_fixed_width<T>())>

Double check if fixed-point types are considered fixed-width, because I don't think you want this for a device_uvector<decimal32/64> either.

Copy link
Contributor

@bdice bdice Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed point types are fixed width, and we don't want those. We probably want the same condition as what we use for the non-owning column_view from device_span construction: cudf::is_numeric<T>() or cudf::is_chrono<T>()

cudf/cpp/include/cudf/column/column_view.hpp

Line 428 in e7e5f45

template <typename T, CUDF_ENABLE_IF(cudf::is_numeric<T>() or cudf::is_chrono<T>())>

Copy link
Contributor

@davidwendt davidwendt Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Another reason this should probably just be a make_fixed_width_column factory function instead of a constructor.
This factory: https://docs.rapids.ai/api/libcudf/stable/group__column__factories.html#ga595aed5d1e6f3d4b3d8da9eeffed916d
takes a buffer for a null mask. It seems reasonable to add one that accepts a device-uvector.
Here is one we use for strings that takes 2 uvectors with move semantics: https://docs.rapids.ai/api/libcudf/stable/group__column__factories.html#ga86f7623f0d230c96491ef88d665385cc

{
}

/**
* @brief Construct a new column from existing device memory.
*
Expand Down
17 changes: 17 additions & 0 deletions cpp/tests/column/column_test.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,23 @@ TYPED_TEST(TypedColumnTest, MoveConstructorWithMask)
EXPECT_EQ(original_mask, moved_to_view.null_mask());
}

TYPED_TEST(TypedColumnTest, DeviceUvectorConstructor)
{
rmm::device_uvector<TypeParam> original{static_cast<std::size_t>(this->num_elements()),
cudf::default_stream_value};
thrust::copy(thrust::device,
static_cast<TypeParam*>(this->data.data()),
static_cast<TypeParam*>(this->data.data()) + this->num_elements(),
original.begin());
auto original_data = original.data();
cudf::column moved_to{std::move(original)};
verify_column_views(moved_to);

// Verify move
cudf::column_view moved_to_view = moved_to;
EXPECT_EQ(original_data, moved_to_view.head());
}

TYPED_TEST(TypedColumnTest, ConstructWithChildren)
{
std::vector<std::unique_ptr<cudf::column>> children;
Expand Down