-
Notifications
You must be signed in to change notification settings - Fork 952
Expose stream-ordering in scalar and avro APIs #17766
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
Changes from 5 commits
3733668
40f6ba7
5d06857
ffab239
a1c5edc
b6f2c1a
610cc0b
be06eaa
49d3129
a17e5a5
95d856d
8224686
c96c365
67af752
cf8fc92
77e3ae4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||
/* | ||||||
* Copyright (c) 2019-2023, NVIDIA CORPORATION. | ||||||
* Copyright (c) 2019-2025, NVIDIA CORPORATION. | ||||||
* | ||||||
* Copyright 2018-2019 BlazingDB, Inc. | ||||||
* Copyright 2018 Christian Noboa Mardini <christian@blazingdb.com> | ||||||
|
@@ -81,7 +81,7 @@ void ASSERT_BINOP(cudf::column_view const& out, | |||||
TypeOp&& op, | ||||||
ValueComparator const& value_comparator = ValueComparator()) | ||||||
{ | ||||||
auto lhs_h = static_cast<ScalarType const&>(lhs).operator TypeLhs(); | ||||||
auto lhs_h = static_cast<ScalarType const&>(lhs).get_value(cudf::get_default_stream()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please refactor the tests like this:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a |
||||||
auto rhs_h = cudf::test::to_host<TypeRhs>(rhs); | ||||||
auto rhs_data = rhs_h.first; | ||||||
auto out_h = cudf::test::to_host<TypeOut>(out); | ||||||
|
@@ -129,7 +129,7 @@ void ASSERT_BINOP(cudf::column_view const& out, | |||||
TypeOp&& op, | ||||||
ValueComparator const& value_comparator = ValueComparator()) | ||||||
{ | ||||||
auto rhs_h = static_cast<ScalarType const&>(rhs).operator TypeRhs(); | ||||||
auto rhs_h = static_cast<ScalarType const&>(rhs).get_value(cudf::get_default_stream()); | ||||||
auto lhs_h = cudf::test::to_host<TypeLhs>(lhs); | ||||||
auto lhs_data = lhs_h.first; | ||||||
auto out_h = cudf::test::to_host<TypeOut>(out); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Copyright (c) 2023-2025, NVIDIA CORPORATION. | ||
vyasr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#include <cudf_test/base_fixture.hpp> | ||
#include <cudf_test/column_wrapper.hpp> | ||
#include <cudf_test/default_stream.hpp> | ||
#include <cudf_test/table_utilities.hpp> | ||
#include <cudf_test/testing_main.hpp> | ||
#include <cudf_test/type_lists.hpp> | ||
|
||
#include <cudf/scalar/scalar.hpp> | ||
|
||
template <typename T> | ||
struct TypedScalarTest : public cudf::test::BaseFixture {}; | ||
|
||
TYPED_TEST_SUITE(TypedScalarTest, cudf::test::FixedWidthTypes); | ||
|
||
TYPED_TEST(TypedScalarTest, DefaultValidity) | ||
{ | ||
using Type = cudf::device_storage_type_t<TypeParam>; | ||
Type value = static_cast<Type>(cudf::test::make_type_param_scalar<TypeParam>(7)); | ||
cudf::scalar_type_t<TypeParam> s(value); | ||
CUDF_EXPECT_NO_THROW(s.get_value(cudf::test::get_default_stream())); | ||
|
||
EXPECT_TRUE(s.is_valid()); | ||
EXPECT_EQ(value, s.value()); | ||
} | ||
|
||
struct StringScalarTest : public cudf::test::BaseFixture {}; | ||
|
||
TEST_F(StringScalarTest, DefaultValidity) | ||
{ | ||
std::string value = "test string"; | ||
auto s = cudf::string_scalar(value); | ||
CUDF_EXPECT_NO_THROW(s.get_value(cudf::test::get_default_stream())); | ||
EXPECT_TRUE(s.is_valid()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RMM just calls this
value
. We often try to avoid “get” in our function names.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, doesn’t
value
already exist just below here? We may not needget_value
at all. Maybe we just need to delete the conversion operator since it does not take a stream.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I chose to introduce another function here because
value
can return either host or device data depending on the type.string_scalar::value
returns acudf::string_view
on the device, whilefixed_point::value
andfixed_width::value
copy to host. I was also unsure of usingfixed_point::value
directly since the conversion operator returns thefixed_point_value
instead of the unscaled value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to modify
string_scalar::value
to return a host string view since it is used in several places that directly access the scalar on device. One option is to rename this function tostring_scalar::d_value
and introduce anotherstring_scalar::value
that returns astd::string
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
string_scalar::value()
should return acudf::string_view
which is possible from host or device since it just wraps a pointer and a size held by thestring_scalar
. No device code is needed. Actually thestream
parameter is not used at all.To return a host
std::string
one would use thestring_scalar::to_string()
function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Bradley we should not have the
get_value()
functions and just keep thevalue()
ones.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks David, Bradley. I've retained the
value
functions and got rid of the conversion operators.