scovich commented on code in PR #7887:
URL: https://github.com/apache/arrow-rs/pull/7887#discussion_r2217478275
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -891,7 +893,7 @@ pub fn cast_with_options(
scale,
from_type,
to_type,
- |x: i256| x.to_f64().unwrap(),
+ |x: i256| decimal256_to_f64(x),
Review Comment:
I'm having a hard time understanding the semantics of `i256::to_64`, but it
_seems_ to be a provided method of [impl ToPrimitive for
i256](https://arrow.apache.org/rust/arrow/datatypes/struct.i256.html#impl-ToPrimitive-for-i256),
which
> tries to convert through `to_i64()`, and failing that through `to_u64()`
In contrast, the `Decimal128` case above is doing a rust cast `x as f64`,
which I would assume already handles this case Just Fine (and probably doesn't
even need to resort to +/- INF, because f64 has a dynamic range of ~2048 powers
of two (from 2\*\*-1023 to 2\*\*1023) -- far more than enough to handle even
`Decimal256` (which has dynamic range of only 512 powers of two (from 2\*\*-255
to 2\*\*255)
If we're anyway trying to convert the value to `f64`, it seems like we
should fix this bug by following the advice from the docs for
[ToPrimitive::to_f64](https://docs.rs/num-traits/0.2.19/num_traits/cast/trait.ToPrimitive.html#method.to_f64):
>Types implementing this trait should override this method if they can
represent a greater range.
This shouldn't actually be very difficult. Assuming the two i128 parts
together form the halves of one giant two's complement value, something like
[this](https://play.rust-lang.org/?version=stable&mode=release&edition=2024&gist=39d058aeece0975133f2923fc71a1104)
should do the trick.
--
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]
