Skip to content

Consider updating i256::to_f64 to handle overflow #7980

@alamb

Description

@alamb

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 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:

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions