pitrou commented on code in PR #52469:
URL: https://github.com/apache/arrow/pull/52469#discussion_r4348832088
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,51 @@ TEST(Cast, FloatingToInt) {
}
}
+TEST(Cast, FloatingToIntOverflow) {
+ for (auto from : {float16(), float32(), float64()}) {
+ for (bool allow_float_truncate : {false, true}) {
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = allow_float_truncate;
+ for (auto json : {"[128.0]", "[-129.0]", "[NaN]", "[Inf]", "[-Inf]"}) {
Review Comment:
Can we also test `127.5` and `-128.5` when truncation is disabled?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,51 @@ TEST(Cast, FloatingToInt) {
}
}
+TEST(Cast, FloatingToIntOverflow) {
+ for (auto from : {float16(), float32(), float64()}) {
+ for (bool allow_float_truncate : {false, true}) {
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = allow_float_truncate;
+ for (auto json : {"[128.0]", "[-129.0]", "[NaN]", "[Inf]", "[-Inf]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[127.0, null, -128.0]"),
+ ArrayFromJSON(int8(), "[127, null, -128]"), opts);
+ CheckCast(TweakValidityBit(ArrayFromJSON(from, "[1000.0, 1.0]"), 0,
false),
+ ArrayFromJSON(int8(), "[null, 1]"), opts);
+
+ opts.to_type = uint8();
+ CheckCastFails(ArrayFromJSON(from, "[256.0]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-1.0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+ opts.to_type = uint8();
+ CheckCast(ArrayFromJSON(from, "[255.5, -0.5]"), ArrayFromJSON(uint8(),
"[255, 0]"),
+ opts);
+ }
+
+ for (auto from : {float32(), float64()}) {
+ auto opts = CastOptions::Safe(int64());
+ opts.allow_float_truncate = true;
+ CheckCastFails(ArrayFromJSON(from, "[9223372036854775808.0]"), opts);
+ CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0]"),
+ ArrayFromJSON(int64(), "[-9223372036854775808]"), opts);
+ opts.to_type = uint64();
+ CheckCastFails(ArrayFromJSON(from, "[18446744073709551616.0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("Float value
1.5"),
+ Cast(ArrayFromJSON(float16(), "[1.5]"),
opts));
+ opts.allow_float_truncate = true;
+ EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("Float value
inf out"),
Review Comment:
What does "inf out" mean?
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -87,18 +89,68 @@ struct WasTruncated<HalfFloatType, OutType> {
}
};
-// InType is a floating point type we are planning to cast to integer
template <typename InType, typename OutType, typename InT = typename
InType::c_type,
typename OutT = typename OutType::c_type>
+struct WasOutOfRange {
+ static constexpr auto kMin =
static_cast<InT>(std::numeric_limits<OutT>::min());
Review Comment:
These seems to assume that `OutT` is integral, can we add a `static_assert`
about that?
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -87,18 +89,68 @@ struct WasTruncated<HalfFloatType, OutType> {
}
};
-// InType is a floating point type we are planning to cast to integer
template <typename InType, typename OutType, typename InT = typename
InType::c_type,
typename OutT = typename OutType::c_type>
+struct WasOutOfRange {
+ static constexpr auto kMin =
static_cast<InT>(std::numeric_limits<OutT>::min());
+ static constexpr auto kMaxPlusOne =
+ static_cast<InT>(std::numeric_limits<OutT>::max() / 2 + 1) * 2;
+
+ static bool Check(OutT, InT in_val) {
+ return !(std::trunc(in_val) >= kMin) | !(in_val < kMaxPlusOne);
+ }
+
+ static bool CheckMaybeNull(OutT out_val, InT in_val, bool is_valid) {
+ return is_valid && Check(out_val, in_val);
+ }
+};
+
+template <typename OutType>
+struct WasOutOfRange<HalfFloatType, OutType> {
+ using OutT = typename OutType::c_type;
+ static bool Check(OutT out_val, uint16_t in_val) {
+ return WasOutOfRange<FloatType, OutType>::Check(out_val,
+
Float16::FromBits(in_val).ToFloat());
+ }
+
+ static bool CheckMaybeNull(OutT out_val, uint16_t in_val, bool is_valid) {
+ return is_valid && Check(out_val, in_val);
+ }
+};
+
+// InType is a floating point type we are planning to cast to integer
+template <template <typename...> class Checker, typename InType, typename
OutType,
+ typename InT = typename InType::c_type,
+ typename OutT = typename OutType::c_type>
ARROW_DISABLE_UBSAN("float-cast-overflow")
-Status CheckFloatTruncation(const ArraySpan& input, const ArraySpan& output) {
- auto GetErrorMessage = [&](InT val) {
+Status CheckFloatToIntValues(const ArraySpan& input, const ArraySpan& output) {
+ auto GetErrorMessage = [&](InT in_val) {
+ const auto val = [in_val] {
+ if constexpr (std::is_same_v<InType, HalfFloatType>) {
+ return Float16::FromBits(in_val).ToFloat();
+ } else {
+ return in_val;
+ }
+ }();
+ if constexpr (std::is_same_v<Checker<InType, OutType>,
+ WasOutOfRange<InType, OutType>>) {
+ return Status::Invalid("Float value ", val, " out of range converting to
",
+ *output.type);
+ }
return Status::Invalid("Float value ", val, " was truncated converting to
",
*output.type);
};
const InT* in_data = input.GetValues<InT>(1);
const OutT* out_data = output.GetValues<OutT>(1);
+ auto out_at = [&](int64_t i) {
+ if constexpr (std::is_same_v<Checker<InType, OutType>,
+ WasOutOfRange<InType, OutType>>) {
+ return OutT{};
+ } else {
+ return out_data[i];
+ }
+ };
Review Comment:
Why?
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -87,18 +89,68 @@ struct WasTruncated<HalfFloatType, OutType> {
}
};
-// InType is a floating point type we are planning to cast to integer
template <typename InType, typename OutType, typename InT = typename
InType::c_type,
typename OutT = typename OutType::c_type>
+struct WasOutOfRange {
+ static constexpr auto kMin =
static_cast<InT>(std::numeric_limits<OutT>::min());
+ static constexpr auto kMaxPlusOne =
+ static_cast<InT>(std::numeric_limits<OutT>::max() / 2 + 1) * 2;
+
+ static bool Check(OutT, InT in_val) {
+ return !(std::trunc(in_val) >= kMin) | !(in_val < kMaxPlusOne);
+ }
+
+ static bool CheckMaybeNull(OutT out_val, InT in_val, bool is_valid) {
+ return is_valid && Check(out_val, in_val);
+ }
+};
+
+template <typename OutType>
+struct WasOutOfRange<HalfFloatType, OutType> {
+ using OutT = typename OutType::c_type;
+ static bool Check(OutT out_val, uint16_t in_val) {
+ return WasOutOfRange<FloatType, OutType>::Check(out_val,
+
Float16::FromBits(in_val).ToFloat());
+ }
+
+ static bool CheckMaybeNull(OutT out_val, uint16_t in_val, bool is_valid) {
+ return is_valid && Check(out_val, in_val);
+ }
+};
+
+// InType is a floating point type we are planning to cast to integer
+template <template <typename...> class Checker, typename InType, typename
OutType,
+ typename InT = typename InType::c_type,
+ typename OutT = typename OutType::c_type>
ARROW_DISABLE_UBSAN("float-cast-overflow")
-Status CheckFloatTruncation(const ArraySpan& input, const ArraySpan& output) {
- auto GetErrorMessage = [&](InT val) {
+Status CheckFloatToIntValues(const ArraySpan& input, const ArraySpan& output) {
+ auto GetErrorMessage = [&](InT in_val) {
+ const auto val = [in_val] {
+ if constexpr (std::is_same_v<InType, HalfFloatType>) {
+ return Float16::FromBits(in_val).ToFloat();
+ } else {
+ return in_val;
+ }
+ }();
+ if constexpr (std::is_same_v<Checker<InType, OutType>,
+ WasOutOfRange<InType, OutType>>) {
Review Comment:
Can you please add a relevant method on the various checkers? It doesn't
make sense to put type specialization here.
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,51 @@ TEST(Cast, FloatingToInt) {
}
}
+TEST(Cast, FloatingToIntOverflow) {
+ for (auto from : {float16(), float32(), float64()}) {
+ for (bool allow_float_truncate : {false, true}) {
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = allow_float_truncate;
+ for (auto json : {"[128.0]", "[-129.0]", "[NaN]", "[Inf]", "[-Inf]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[127.0, null, -128.0]"),
+ ArrayFromJSON(int8(), "[127, null, -128]"), opts);
+ CheckCast(TweakValidityBit(ArrayFromJSON(from, "[1000.0, 1.0]"), 0,
false),
+ ArrayFromJSON(int8(), "[null, 1]"), opts);
+
+ opts.to_type = uint8();
+ CheckCastFails(ArrayFromJSON(from, "[256.0]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-1.0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+ opts.to_type = uint8();
+ CheckCast(ArrayFromJSON(from, "[255.5, -0.5]"), ArrayFromJSON(uint8(),
"[255, 0]"),
+ opts);
+ }
+
+ for (auto from : {float32(), float64()}) {
+ auto opts = CastOptions::Safe(int64());
+ opts.allow_float_truncate = true;
+ CheckCastFails(ArrayFromJSON(from, "[9223372036854775808.0]"), opts);
+ CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0]"),
Review Comment:
What about `9223372036854775807`, `9223372036854775807.5`,
`-9223372036854775808.5`?
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -189,10 +240,13 @@ Status CheckFloatToIntTruncation(const ExecValue& input,
const ExecResult& outpu
Status CastFloatingToInteger(KernelContext* ctx, const ExecSpan& batch,
ExecResult* out) {
const auto& options = checked_cast<const CastState*>(ctx->state())->options;
+ if (options.allow_float_truncate && !options.allow_int_overflow) {
+ RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out));
Review Comment:
Why check _before_ while `allow_float_truncate` checks after the conversion?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,51 @@ TEST(Cast, FloatingToInt) {
}
}
+TEST(Cast, FloatingToIntOverflow) {
+ for (auto from : {float16(), float32(), float64()}) {
+ for (bool allow_float_truncate : {false, true}) {
+ auto opts = CastOptions::Safe(int8());
+ opts.allow_float_truncate = allow_float_truncate;
+ for (auto json : {"[128.0]", "[-129.0]", "[NaN]", "[Inf]", "[-Inf]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[127.0, null, -128.0]"),
+ ArrayFromJSON(int8(), "[127, null, -128]"), opts);
+ CheckCast(TweakValidityBit(ArrayFromJSON(from, "[1000.0, 1.0]"), 0,
false),
+ ArrayFromJSON(int8(), "[null, 1]"), opts);
+
+ opts.to_type = uint8();
+ CheckCastFails(ArrayFromJSON(from, "[256.0]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-1.0]"), opts);
Review Comment:
What about `255.0` and `-0.5`?
--
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]
