[REVIEW] Support creating decimal vectors from scalar #6723
Conversation
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
sperlingxx
changed the title
Codecov Report
@@ Coverage Diff @@
## branch-0.17 #6723 +/- ##
============================================
Coverage 81.77% 81.77%
============================================
Files 96 96
Lines 15885 15885
============================================
Hits 12990 12990
Misses 2895 2895 Continue to review full report at Codecov.
|
cpp/src/filling/fill.cu
Outdated
| cudf::size_type end, | ||
| cudaStream_t stream = 0) | ||
| { | ||
| in_place_fill<T>(destination, begin, end, value, stream); | ||
| if (value.type().id() == cudf::type_id::DECIMAL32) { | ||
| auto s = cudf::make_numeric_scalar(cudf::data_type(cudf::type_id::INT32)); |
There was a problem hiding this comment.
Are you able to use make_fixed_point_scalar here?
There was a problem hiding this comment.
@codereport I think we need int32-typed scalar to keep align with int32-typed column view. Acually, I am not familiar with cudf native. If I am wrong, please correct me. Thank you!
codereport
changed the title
cpp/src/filling/fill.cu
Outdated
| auto in_place_view = cudf::mutable_column_view{s->type(), | ||
| destination.size(), | ||
| reinterpret_cast<void*>(destination.data<T>()), | ||
| destination.null_mask()}; |
There was a problem hiding this comment.
Hi @jrhemstad , thanks for review! I've replaced this code piece with logicial_cast. Meanwhile, I found logicial_cast can't work at here directly. There exists two obstacles:
- With
logicial_cast, we can only get (immutable) column_view. But we really need mutable_column_view here. - Fixed-point types are not logically castable in current.
I attempted to solve above two problems. But I am not quite confident in my work.
There was a problem hiding this comment.
I will review the logical_cast for fixed_point. I was just looking at this code the other day.
| @@ -54,23 +59,30 @@ struct ColumnViewAllTypesTests : public cudf::test::BaseFixture { | |||
| TYPED_TEST_CASE(ColumnViewAllTypesTests, cudf::test::FixedWidthTypes); | |||
|
|
|||
| template <typename FromType, typename ToType, typename Iterator> | |||
| void do_logical_cast(cudf::column_view const& input, Iterator begin, Iterator end) | |||
| void do_logical_cast(cudf::test::detail::column_wrapper& input, Iterator begin, Iterator end) | |||
There was a problem hiding this comment.
This should not be column_wrapper.
There was a problem hiding this comment.
I don't know how to change here. I used column_wrapper to adapt both column_view and mutable_column_view.
There was a problem hiding this comment.
mutable_column_view is implicitly convertible to column_view.
There was a problem hiding this comment.
Thanks, I've updated with implicit conversion.
| using RepType = typename T::rep; | ||
| auto s = cudf::numeric_scalar<RepType>(unscaled, value.is_valid()); | ||
| auto view = cudf::logical_cast(destination, s.type()); | ||
| in_place_fill<RepType>(view, begin, end, s, stream); |
There was a problem hiding this comment.
This looks much nicer now!
sperlingxx
force-pushed
the
fix_dec_v_from_s
branch
from
651c825
to
9905ea5
Compare
Co-authored-by: Jake Hemstad <jhemstad@nvidia.com>
Co-authored-by: Jake Hemstad <jhemstad@nvidia.com>
sperlingxx
force-pushed
the
fix_dec_v_from_s
branch
from
9905ea5
to
7d60267
Compare
Currently, we will get unexpected results when creating decimal column vectors from decimal scalar and row number. The reason is that we perform
in_place_fillwith fixed-point data type directly.This PR attempts to fix this problem through peforming
in_place_fillwith underlying data type of fixed-point columns. Related tests are provided in java package.