Copilot commented on code in PR #52469:
URL: https://github.com/apache/arrow/pull/52469#discussion_r4242455099
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -192,7 +230,9 @@ Status CastFloatingToInteger(KernelContext* ctx, const
ExecSpan& batch, ExecResu
CastNumberToNumberUnsafe(batch[0].type()->id(), out->type()->id(),
batch[0].array,
out->array_span_mutable());
if (!options.allow_float_truncate) {
- RETURN_NOT_OK(CheckFloatToIntTruncation(batch[0], *out));
+ RETURN_NOT_OK(CheckFloatToInt<WasTruncated>(batch[0], *out));
+ } else if (!options.allow_int_overflow) {
+ RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out));
Review Comment:
The range check happens only after `CastNumberToNumberUnsafe` has converted
every value. An out-of-range floating-to-integer conversion is undefined in
C++; `ARROW_DISABLE_UBSAN` only suppresses the diagnostic and does not make
this safe or deterministic. Validate the input range before performing the
unsafe cast, then run the post-cast truncation check if needed.
##########
cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc:
##########
@@ -87,12 +89,48 @@ 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 {
+ // Both bounds are zero or a power of two, so they are exact in InT
+ 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) {
+Status CheckFloatToIntValues(const ArraySpan& input, const ArraySpan& output) {
auto GetErrorMessage = [&](InT 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);
Review Comment:
For `HALF_FLOAT`, `InT` is the raw `uint16_t` bit pattern, so this reports
values such as `31744` or `32256` instead of `Inf` or `NaN`. Decode with
`Float16::FromBits(val).ToFloat()` before formatting the diagnostic.
--
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]
