-
Notifications
You must be signed in to change notification settings - Fork 982
Description
I'm having a hard time understanding the semantics of i256::to_f64
, but it seems to be a provided method of impl ToPrimitive for i256, which
tries to convert through
to_i64()
, and failing that throughto_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:
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 should do the trick, and would always return a finite value.
Originally posted by @scovich in #7887 (comment)
I believe Ryan is suggesting updating the i256
struct, which is implemented in arrow-rs here: https://github.com/apache/arrow-rs/blob/a2cc42639b4ad5579d052e6f2317a413e2407e0f/arrow-buffer/src/bigint/mod.rs#L58-L57
Specifically, potentially to provide a new implementation for ToPrimitive::to_f64
that better handles the conversions here: https://github.com/apache/arrow-rs/blob/a2cc42639b4ad5579d052e6f2317a413e2407e0f/arrow-buffer/src/bigint/mod.rs#L801-L800