From be1d3b8ce2ee9990ca5613cf4261234d815dea9c Mon Sep 17 00:00:00 2001 From: Wonchan Lee Date: Fri, 26 Feb 2021 16:20:58 -0800 Subject: [PATCH 1/2] Pass stream and memory resource into `make_default_constructed_scalar` --- cpp/include/cudf/scalar/scalar_factories.hpp | 7 ++++- cpp/src/copying/get_element.cu | 2 +- cpp/src/reductions/minmax.cu | 4 +-- cpp/src/reductions/reductions.cpp | 2 +- cpp/src/scalar/scalar_factories.cpp | 28 ++++++++++++++------ 5 files changed, 30 insertions(+), 13 deletions(-) diff --git a/cpp/include/cudf/scalar/scalar_factories.hpp b/cpp/include/cudf/scalar/scalar_factories.hpp index 5271bed14c8..a0a0a22091e 100644 --- a/cpp/include/cudf/scalar/scalar_factories.hpp +++ b/cpp/include/cudf/scalar/scalar_factories.hpp @@ -113,8 +113,13 @@ std::unique_ptr make_string_scalar( * @throws std::bad_alloc if device memory allocation fails * * @param type The desired element type + * @param stream CUDA stream used for device memory operations. + * @param mr Device memory resource used to allocate the scalar's `data` and `is_valid` bool. */ -std::unique_ptr make_default_constructed_scalar(data_type type); +std::unique_ptr make_default_constructed_scalar( + data_type type, + rmm::cuda_stream_view stream = rmm::cuda_stream_default, + rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource()); /** * @brief Construct scalar using the given value of fixed width type diff --git a/cpp/src/copying/get_element.cu b/cpp/src/copying/get_element.cu index fc241dd9e32..446f9b0dda9 100644 --- a/cpp/src/copying/get_element.cu +++ b/cpp/src/copying/get_element.cu @@ -101,7 +101,7 @@ struct get_element_functor { stream); if (!key_index_scalar.is_valid(stream)) { - auto null_result = make_default_constructed_scalar(dict_view.keys().type()); + auto null_result = make_default_constructed_scalar(dict_view.keys().type(), stream, mr); null_result->set_valid(false, stream); return null_result; } diff --git a/cpp/src/reductions/minmax.cu b/cpp/src/reductions/minmax.cu index 58db90b600d..c99b366c2dd 100644 --- a/cpp/src/reductions/minmax.cu +++ b/cpp/src/reductions/minmax.cu @@ -252,8 +252,8 @@ std::pair, std::unique_ptr> minmax( if (col.null_count() == col.size()) { // this handles empty and all-null columns // return scalars with valid==false - return {make_default_constructed_scalar(col.type()), - make_default_constructed_scalar(col.type())}; + return {make_default_constructed_scalar(col.type(), stream, mr), + make_default_constructed_scalar(col.type(), stream, mr)}; } return type_dispatcher(col.type(), minmax_functor{}, col, stream, mr); diff --git a/cpp/src/reductions/reductions.cpp b/cpp/src/reductions/reductions.cpp index 3c10ac61643..43dd86b307f 100644 --- a/cpp/src/reductions/reductions.cpp +++ b/cpp/src/reductions/reductions.cpp @@ -112,7 +112,7 @@ std::unique_ptr reduce( rmm::cuda_stream_view stream = rmm::cuda_stream_default, rmm::mr::device_memory_resource *mr = rmm::mr::get_current_device_resource()) { - std::unique_ptr result = make_default_constructed_scalar(output_dtype); + std::unique_ptr result = make_default_constructed_scalar(output_dtype, stream, mr); result->set_valid(false, stream); // check if input column is empty diff --git a/cpp/src/scalar/scalar_factories.cpp b/cpp/src/scalar/scalar_factories.cpp index 644b320dcd5..5714eaee864 100644 --- a/cpp/src/scalar/scalar_factories.cpp +++ b/cpp/src/scalar/scalar_factories.cpp @@ -100,36 +100,48 @@ std::unique_ptr make_fixed_width_scalar(data_type type, namespace { struct default_scalar_functor { template - std::unique_ptr operator()() + std::unique_ptr operator()(rmm::cuda_stream_view stream, + rmm::mr::device_memory_resource* mr) { - using ScalarType = scalar_type_t; - return std::unique_ptr(new ScalarType); + return make_fixed_width_scalar(data_type(type_to_id()), stream, mr); } }; template <> -std::unique_ptr default_scalar_functor::operator()() +std::unique_ptr default_scalar_functor::operator()( + rmm::cuda_stream_view stream, rmm::mr::device_memory_resource* mr) +{ + return std::unique_ptr(new string_scalar("", false, stream, mr)); +} + +template <> +std::unique_ptr default_scalar_functor::operator()( + rmm::cuda_stream_view stream, rmm::mr::device_memory_resource* mr) { CUDF_FAIL("dictionary type not supported"); } template <> -std::unique_ptr default_scalar_functor::operator()() +std::unique_ptr default_scalar_functor::operator()( + rmm::cuda_stream_view stream, rmm::mr::device_memory_resource* mr) { CUDF_FAIL("list_view type not supported"); } template <> -std::unique_ptr default_scalar_functor::operator()() +std::unique_ptr default_scalar_functor::operator()( + rmm::cuda_stream_view stream, rmm::mr::device_memory_resource* mr) { CUDF_FAIL("struct_view type not supported"); } } // namespace -std::unique_ptr make_default_constructed_scalar(data_type type) +std::unique_ptr make_default_constructed_scalar(data_type type, + rmm::cuda_stream_view stream, + rmm::mr::device_memory_resource* mr) { - return type_dispatcher(type, default_scalar_functor{}); + return type_dispatcher(type, default_scalar_functor{}, stream, mr); } } // namespace cudf From 94073c6c288278a2643631c4003ab6722a9e78e3 Mon Sep 17 00:00:00 2001 From: Wonchan Lee Date: Fri, 26 Feb 2021 16:21:54 -0800 Subject: [PATCH 2/2] Modify tests to use the updated scalar factory correctly --- cpp/tests/scalar/factories_test.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cpp/tests/scalar/factories_test.cpp b/cpp/tests/scalar/factories_test.cpp index c91fb6b3b5e..fd0a92a0168 100644 --- a/cpp/tests/scalar/factories_test.cpp +++ b/cpp/tests/scalar/factories_test.cpp @@ -89,7 +89,7 @@ TYPED_TEST(TimestampScalarFactory, TypeCast) } template -struct DefaultScalarFactory : public cudf::test::BaseFixture { +struct DefaultScalarFactory : public ScalarFactoryTest { static constexpr auto factory = cudf::make_default_constructed_scalar; }; @@ -98,7 +98,8 @@ TYPED_TEST_CASE(DefaultScalarFactory, MixedTypes); TYPED_TEST(DefaultScalarFactory, FactoryDefault) { - std::unique_ptr s = this->factory(cudf::data_type{cudf::type_to_id()}); + std::unique_ptr s = + this->factory(cudf::data_type{cudf::type_to_id()}, this->stream(), this->mr()); EXPECT_EQ(s->type(), cudf::data_type{cudf::type_to_id()}); EXPECT_FALSE(s->is_valid()); @@ -106,7 +107,8 @@ TYPED_TEST(DefaultScalarFactory, FactoryDefault) TYPED_TEST(DefaultScalarFactory, TypeCast) { - std::unique_ptr s = this->factory(cudf::data_type{cudf::type_to_id()}); + std::unique_ptr s = + this->factory(cudf::data_type{cudf::type_to_id()}, this->stream(), this->mr()); auto numeric_s = static_cast*>(s.get());