pitrou commented on code in PR #52469:
URL: https://github.com/apache/arrow/pull/52469#discussion_r4387348657
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -85,18 +92,60 @@ struct WasTruncated<HalfFloatType, OutType> {
static bool CheckMaybeNull(OutT out_val, uint16_t in_val, bool is_valid) {
return is_valid && static_cast<float>(out_val) !=
Float16::FromBits(in_val).ToFloat();
}
+
+ static Status Error(uint16_t in_val, const DataType& out_type) {
+ return WasTruncated<FloatType,
OutType>::Error(Float16::FromBits(in_val).ToFloat(),
+ out_type);
+ }
};
-// 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>
-ARROW_DISABLE_UBSAN("float-cast-overflow")
-Status CheckFloatTruncation(const ArraySpan& input, const ArraySpan& output) {
- auto GetErrorMessage = [&](InT val) {
- return Status::Invalid("Float value ", val, " was truncated converting to
",
- *output.type);
- };
+struct WasOutOfRange {
+ static_assert(std::is_integral_v<OutT>);
+
+ 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);
Review Comment:
There are a lot of negatives here, can you make this simpler and more
readable?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,66 @@ 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);
+ CheckCast(ArrayFromJSON(from, "[255.0, null, 0.0]"),
+ ArrayFromJSON(uint8(), "[255, null, 0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ CheckCastFails(ArrayFromJSON(from, "[127.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-128.5]"), opts);
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+
+ opts = CastOptions::Safe(uint8());
+ CheckCastFails(ArrayFromJSON(from, "[255.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-0.5]"), opts);
+ opts.allow_float_truncate = true;
+ 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;
+ for (auto json : {"[9223372036854775807]", "[9223372036854775807.5]",
+ "[9223372036854775808.0]"}) {
Review Comment:
Can we use only the exact float representations? Otherwise the test is
confusing.
So: keep `9223372036854775808.0` but not the two other ones.
Also, add `-9223372036854777856.0` for negative numbers.
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,66 @@ 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);
+ CheckCast(ArrayFromJSON(from, "[255.0, null, 0.0]"),
+ ArrayFromJSON(uint8(), "[255, null, 0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ CheckCastFails(ArrayFromJSON(from, "[127.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-128.5]"), opts);
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+
+ opts = CastOptions::Safe(uint8());
+ CheckCastFails(ArrayFromJSON(from, "[255.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-0.5]"), opts);
+ opts.allow_float_truncate = true;
+ 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;
+ for (auto json : {"[9223372036854775807]", "[9223372036854775807.5]",
+ "[9223372036854775808.0]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0,
-9223372036854775808.5]"),
+ ArrayFromJSON(int64(), "[-9223372036854775808,
-9223372036854775808]"),
+ opts);
+ opts.to_type = uint64();
+ CheckCastFails(ArrayFromJSON(from, "[18446744073709551616.0]"), opts);
Review Comment:
Also -1.0?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,66 @@ 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);
+ CheckCast(ArrayFromJSON(from, "[255.0, null, 0.0]"),
+ ArrayFromJSON(uint8(), "[255, null, 0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ CheckCastFails(ArrayFromJSON(from, "[127.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-128.5]"), opts);
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+
+ opts = CastOptions::Safe(uint8());
+ CheckCastFails(ArrayFromJSON(from, "[255.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-0.5]"), opts);
+ opts.allow_float_truncate = true;
+ 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;
+ for (auto json : {"[9223372036854775807]", "[9223372036854775807.5]",
+ "[9223372036854775808.0]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0,
-9223372036854775808.5]"),
+ ArrayFromJSON(int64(), "[-9223372036854775808,
-9223372036854775808]"),
+ opts);
Review Comment:
Same question here.
Also test `9223372036854774784.0` for positive numbers.
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -398,6 +398,66 @@ 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);
+ CheckCast(ArrayFromJSON(from, "[255.0, null, 0.0]"),
+ ArrayFromJSON(uint8(), "[255, null, 0]"), opts);
+ }
+
+ auto opts = CastOptions::Safe(int8());
+ CheckCastFails(ArrayFromJSON(from, "[127.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-128.5]"), opts);
+ opts.allow_float_truncate = true;
+ CheckCast(ArrayFromJSON(from, "[127.5, -128.5]"),
+ ArrayFromJSON(int8(), "[127, -128]"), opts);
+
+ opts = CastOptions::Safe(uint8());
+ CheckCastFails(ArrayFromJSON(from, "[255.5]"), opts);
+ CheckCastFails(ArrayFromJSON(from, "[-0.5]"), opts);
+ opts.allow_float_truncate = true;
+ 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;
+ for (auto json : {"[9223372036854775807]", "[9223372036854775807.5]",
+ "[9223372036854775808.0]"}) {
+ CheckCastFails(ArrayFromJSON(from, json), opts);
+ }
+ CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0,
-9223372036854775808.5]"),
+ ArrayFromJSON(int64(), "[-9223372036854775808,
-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.500000 was truncated converting to
int8"),
+ Cast(ArrayFromJSON(float16(), "[1.5]"), opts));
+ opts.allow_float_truncate = true;
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::HasSubstr("Float value inf out of range converting
to int8"),
+ Cast(ArrayFromJSON(float16(), "[Inf]"), opts));
Review Comment:
Why these tests? float16 is already tested above.
--
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]
