pitrou commented on code in PR #12660:
URL: https://github.com/apache/arrow/pull/12660#discussion_r852039451
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1397,22 +1396,8 @@ struct RoundToMultiple {
CType multiple;
- explicit RoundToMultiple(const State& state, const DataType& out_ty) {
- const auto& options = state.options;
- DCHECK(options.multiple);
- DCHECK(options.multiple->is_valid);
- DCHECK(is_floating(options.multiple->type->id()));
- switch (options.multiple->type->id()) {
- case Type::FLOAT:
- multiple =
static_cast<CType>(UnboxScalar<FloatType>::Unbox(*options.multiple));
- break;
- case Type::DOUBLE:
- multiple =
static_cast<CType>(UnboxScalar<DoubleType>::Unbox(*options.multiple));
- break;
- default:
- DCHECK(false);
- }
- }
+ explicit RoundToMultiple(const State& state, const DataType& out_ty)
+ : multiple(UnboxScalar<ArrowType>::Unbox(*state.options.multiple)) {}
Review Comment:
Can we still keep the DCHECKs in the constructor body?
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -2004,21 +2004,19 @@ TEST_F(TestUnaryArithmeticDecimal,
RoundToMultipleTowardsInfinity) {
set_multiple(ty, 1);
CheckScalar(func, {values}, values, &options);
set_multiple(ty, 0);
- CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")},
- "Rounding multiple must be positive", &options);
+ CheckRaises(func, {values}, "Rounding multiple must be positive",
&options);
+ options.multiple =
+ std::make_shared<Decimal128Scalar>(Decimal128(0), decimal128(4, 2));
+ CheckRaises(func, {values}, "Rounding multiple must be positive",
&options);
set_multiple(ty, -10);
CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")},
"Rounding multiple must be positive", &options);
set_multiple(ty, 100);
CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")},
"Rounded value 100.00 does not fit in precision", &options);
options.multiple = std::make_shared<DoubleScalar>(1.0);
- CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")}, "scalar, not
double",
- &options);
- options.multiple =
- std::make_shared<Decimal128Scalar>(Decimal128(0), decimal128(3, 0));
- CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")}, "scalar, not
decimal128(3, 0)",
- &options);
+ CheckRaises(func, {ArrayFromJSON(ty, R"(["99.99"])")},
+ "Rounded value 100.00 does not fit in precision", &options);
Review Comment:
It seems this is the only place where casting the multiple is tested? Can
you add a successful rounding test somewhere as well?
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1453,17 +1438,11 @@ struct RoundToMultiple<ArrowType, kRoundMode,
enable_if_decimal<ArrowType>> {
bool has_halfway_point;
explicit RoundToMultiple(const State& state, const DataType& out_ty)
- : ty(checked_cast<const ArrowType&>(out_ty)) {
- const auto& options = state.options;
- DCHECK(options.multiple);
- DCHECK(options.multiple->is_valid);
- DCHECK(options.multiple->type->Equals(out_ty));
- multiple = UnboxScalar<ArrowType>::Unbox(*options.multiple);
- half_multiple = multiple;
- half_multiple /= 2;
- neg_half_multiple = -half_multiple;
- has_halfway_point = multiple.low_bits() % 2 == 0;
- }
+ : ty(checked_cast<const ArrowType&>(out_ty)),
+ multiple(UnboxScalar<ArrowType>::Unbox(*state.options.multiple)),
+ half_multiple(multiple / 2),
+ neg_half_multiple(-half_multiple),
+ has_halfway_point(multiple.low_bits() % 2 == 0) {}
Review Comment:
Here as well, keep the DCHECKs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
