From 746d8effcddb385f8f854ad219f3485122506f04 Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Tue, 26 Jul 2022 11:00:32 -0700 Subject: [PATCH 1/9] add constructor --- cpp/include/cudf/column/column.hpp | 16 ++++++++++++++++ cpp/tests/column/column_test.cu | 17 +++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index ac3824dfc21..f59c492c8a6 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -75,6 +76,21 @@ class column { */ column(column&& other) noexcept; + /** + * @brief Move the contents from `other` to create a new column. + * + * After the move, `other.data() == NULL` + * + * @param other The device_uvector whose contents will be moved into the new column. + */ + template + column(rmm::device_uvector&& other) noexcept + : _type{cudf::type_to_id()}, + _size{static_cast(other.size())}, + _data{other.release()} + { + } + /** * @brief Construct a new column from existing device memory. * diff --git a/cpp/tests/column/column_test.cu b/cpp/tests/column/column_test.cu index 6fcabbcf823..e9343b4aa8d 100644 --- a/cpp/tests/column/column_test.cu +++ b/cpp/tests/column/column_test.cu @@ -345,6 +345,23 @@ TYPED_TEST(TypedColumnTest, MoveConstructorWithMask) EXPECT_EQ(original_mask, moved_to_view.null_mask()); } +TYPED_TEST(TypedColumnTest, DeviceUvectorConstructor) +{ + rmm::device_uvector original{static_cast(this->num_elements()), + cudf::default_stream_value}; + thrust::copy(thrust::device, + static_cast(this->data.data()), + static_cast(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> children; From 2a2f26cc0974585286cad067081fa166faea6fbd Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Thu, 28 Jul 2022 17:06:55 -0700 Subject: [PATCH 2/9] review feedback --- cpp/include/cudf/column/column.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index f59c492c8a6..eafc8a524c1 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -77,18 +77,18 @@ class column { column(column&& other) noexcept; /** - * @brief Move the contents from `other` to create a new column. - * - * After the move, `other.data() == NULL` + * @brief Construct a new column by taking ownership of the contents of a device_uvector. * * @param other The device_uvector whose contents will be moved into the new column. */ - template - column(rmm::device_uvector&& other) noexcept + template () or cudf::is_chrono())> + column(rmm::device_uvector&& other) : _type{cudf::type_to_id()}, _size{static_cast(other.size())}, _data{other.release()} { + CUDF_EXPECTS(_size < std::numeric_limits::max(), + "Input vector exceeds maximum column capacity"); } /** From e1305637b717c3db4f2e9183e98c47afcbde51ee Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Mon, 1 Aug 2022 15:45:00 -0700 Subject: [PATCH 3/9] Add optional null mask and null count --- cpp/include/cudf/column/column.hpp | 10 +++++++--- cpp/tests/column/column_test.cu | 21 ++++++++++++++++++++- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index eafc8a524c1..112f8d9c692 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -82,10 +82,14 @@ class column { * @param other The device_uvector whose contents will be moved into the new column. */ template () or cudf::is_chrono())> - column(rmm::device_uvector&& other) + column(rmm::device_uvector&& other, + rmm::device_buffer&& null_mask = {}, + size_type null_count = cudf::UNKNOWN_NULL_COUNT) : _type{cudf::type_to_id()}, - _size{static_cast(other.size())}, - _data{other.release()} + _size{static_cast(other.size())}, + _data{other.release()}, + _null_mask{std::forward(null_mask)}, + _null_count{null_count} { CUDF_EXPECTS(_size < std::numeric_limits::max(), "Input vector exceeds maximum column capacity"); diff --git a/cpp/tests/column/column_test.cu b/cpp/tests/column/column_test.cu index e9343b4aa8d..801cee285b6 100644 --- a/cpp/tests/column/column_test.cu +++ b/cpp/tests/column/column_test.cu @@ -345,7 +345,7 @@ TYPED_TEST(TypedColumnTest, MoveConstructorWithMask) EXPECT_EQ(original_mask, moved_to_view.null_mask()); } -TYPED_TEST(TypedColumnTest, DeviceUvectorConstructor) +TYPED_TEST(TypedColumnTest, DeviceUvectorConstructorNoMask) { rmm::device_uvector original{static_cast(this->num_elements()), cudf::default_stream_value}; @@ -362,6 +362,25 @@ TYPED_TEST(TypedColumnTest, DeviceUvectorConstructor) EXPECT_EQ(original_data, moved_to_view.head()); } +TYPED_TEST(TypedColumnTest, DeviceUvectorConstructorWithMask) +{ + rmm::device_uvector original{static_cast(this->num_elements()), + cudf::default_stream_value}; + thrust::copy(thrust::device, + static_cast(this->data.data()), + static_cast(this->data.data()) + this->num_elements(), + original.begin()); + auto original_data = original.data(); + auto original_mask = this->all_valid_mask.data(); + cudf::column moved_to{std::move(original), std::move(this->all_valid_mask)}; + verify_column_views(moved_to); + + // Verify move + cudf::column_view moved_to_view = moved_to; + EXPECT_EQ(original_data, moved_to_view.head()); + EXPECT_EQ(original_mask, moved_to_view.null_mask()); +} + TYPED_TEST(TypedColumnTest, ConstructWithChildren) { std::vector> children; From 2f4a650c83c9bdf434e818841904fd98d55fc9c2 Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Mon, 1 Aug 2022 16:01:10 -0700 Subject: [PATCH 4/9] Add comment --- cpp/include/cudf/column/column.hpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index 112f8d9c692..d4ea246cbe5 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -80,6 +80,11 @@ class column { * @brief Construct a new column by taking ownership of the contents of a device_uvector. * * @param other The device_uvector whose contents will be moved into the new column. + * @param[in] null_mask Optional, column's null value indicator bitmask. May + * be empty if `null_count` is 0 or `UNKNOWN_NULL_COUNT`. + * @param null_count Optional, the count of null elements. If unknown, specify + * `UNKNOWN_NULL_COUNT` to indicate that the null count should be computed on + * the first invocation of `null_count()`. */ template () or cudf::is_chrono())> column(rmm::device_uvector&& other, From e511c6b072bbd2e2289948c4a39edf4be5925334 Mon Sep 17 00:00:00 2001 From: Srikar Vanavasam Date: Tue, 2 Aug 2022 10:56:14 -0500 Subject: [PATCH 5/9] fix comment Co-authored-by: Nghia Truong --- cpp/include/cudf/column/column.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index d4ea246cbe5..e736a5a92f2 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -80,7 +80,7 @@ class column { * @brief Construct a new column by taking ownership of the contents of a device_uvector. * * @param other The device_uvector whose contents will be moved into the new column. - * @param[in] null_mask Optional, column's null value indicator bitmask. May + * @param null_mask Optional, column's null value indicator bitmask. May * be empty if `null_count` is 0 or `UNKNOWN_NULL_COUNT`. * @param null_count Optional, the count of null elements. If unknown, specify * `UNKNOWN_NULL_COUNT` to indicate that the null count should be computed on From 56f17bf3a7d704f29e872bef8885596a4d45eec6 Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Tue, 2 Aug 2022 14:58:10 -0700 Subject: [PATCH 6/9] Initialize members in constructor body --- cpp/include/cudf/column/column.hpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index d4ea246cbe5..b33d635c737 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -89,15 +89,14 @@ class column { template () or cudf::is_chrono())> column(rmm::device_uvector&& other, rmm::device_buffer&& null_mask = {}, - size_type null_count = cudf::UNKNOWN_NULL_COUNT) - : _type{cudf::type_to_id()}, - _size{static_cast(other.size())}, - _data{other.release()}, - _null_mask{std::forward(null_mask)}, - _null_count{null_count} + size_type null_count = UNKNOWN_NULL_COUNT) { - CUDF_EXPECTS(_size < std::numeric_limits::max(), - "Input vector exceeds maximum column capacity"); + CUDF_EXPECTS(other.size() <= std::numeric_limits::max()); + _type = cudf::type_to_id(); + _size = static_cast(other.size()); + _data = other.release(); + _null_mask = std::forward(null_mask); + _null_count = null_count; } /** From 64023d0556b830a95ca3ea57d0d2df731530ab3f Mon Sep 17 00:00:00 2001 From: Srikar Vanavasam Date: Tue, 2 Aug 2022 22:08:00 -0500 Subject: [PATCH 7/9] add static_cast Co-authored-by: Nghia Truong --- cpp/include/cudf/column/column.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index b8ad0a90df5..e71b7875337 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -91,7 +91,7 @@ class column { rmm::device_buffer&& null_mask = {}, size_type null_count = UNKNOWN_NULL_COUNT) { - CUDF_EXPECTS(other.size() <= std::numeric_limits::max()); + CUDF_EXPECTS(other.size() <= static_cast(std::numeric_limits::max())); _type = cudf::type_to_id(); _size = static_cast(other.size()); _data = other.release(); From dde65013c5bd21a2f7c48b046afa719e188f9184 Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Tue, 2 Aug 2022 20:11:24 -0700 Subject: [PATCH 8/9] Error message --- cpp/include/cudf/column/column.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index e71b7875337..803aaba58fb 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -91,7 +91,8 @@ class column { rmm::device_buffer&& null_mask = {}, size_type null_count = UNKNOWN_NULL_COUNT) { - CUDF_EXPECTS(other.size() <= static_cast(std::numeric_limits::max())); + CUDF_EXPECTS(other.size() <= static_cast(std::numeric_limits::max()), + "The device_uvector size exceeds the maximum size_type."); _type = cudf::type_to_id(); _size = static_cast(other.size()); _data = other.release(); From 5183cff95abb0c761bc04626448f201c3ae2befe Mon Sep 17 00:00:00 2001 From: srikarvanavasam Date: Wed, 3 Aug 2022 09:35:50 -0700 Subject: [PATCH 9/9] IIFE in member list --- cpp/include/cudf/column/column.hpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/cpp/include/cudf/column/column.hpp b/cpp/include/cudf/column/column.hpp index 803aaba58fb..c5f6d339ae9 100644 --- a/cpp/include/cudf/column/column.hpp +++ b/cpp/include/cudf/column/column.hpp @@ -90,14 +90,17 @@ class column { column(rmm::device_uvector&& other, rmm::device_buffer&& null_mask = {}, size_type null_count = UNKNOWN_NULL_COUNT) + : _type{cudf::data_type{cudf::type_to_id()}}, + _size{[&]() { + CUDF_EXPECTS( + other.size() <= static_cast(std::numeric_limits::max()), + "The device_uvector size exceeds the maximum size_type."); + return static_cast(other.size()); + }()}, + _data{other.release()}, + _null_mask{std::move(null_mask)}, + _null_count{null_count} { - CUDF_EXPECTS(other.size() <= static_cast(std::numeric_limits::max()), - "The device_uvector size exceeds the maximum size_type."); - _type = cudf::type_to_id(); - _size = static_cast(other.size()); - _data = other.release(); - _null_mask = std::forward(null_mask); - _null_count = null_count; } /**