scovich commented on code in PR #7986:
URL: https://github.com/apache/arrow-rs/pull/7986#discussion_r2229827409
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -821,6 +821,20 @@ impl ToPrimitive for i256 {
}
}
+ fn to_f64(&self) -> Option<f64> {
+ let mag = if let Some(u) = self.checked_abs() {
+ let (low, high) = u.to_parts();
+ (high as f64) * 2_f64.powi(128) + (low as f64)
Review Comment:
I suspect we could do better by manually twiddling bits, but that's probably
a good follow-up.
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -1995,15 +1995,37 @@ where
}
}
-/// Convert a [`i256`] to `f64` saturating to infinity on overflow.
-fn decimal256_to_f64(v: i256) -> f64 {
- v.to_f64().unwrap_or_else(|| {
- if v.is_negative() {
- f64::NEG_INFINITY
- } else {
- f64::INFINITY
- }
- })
+/// Converts a 256-bit signed integer to a 64-bit floating point number.
+///
+/// This function is primarily used for converting Decimal256 values to f64,
+/// where the Decimal256 is represented as an i256 (256-bit signed integer).
+///
+/// # Arguments
+///
+/// * `val` - The 256-bit signed integer value to convert
+///
+/// # Returns
+///
+/// Returns the floating point representation of the input value.
+///
+/// All `i256` values are within the representable range of `f64`. The
+/// conversion therefore cannot overflow, although large values may lose
+/// precision.
+///
+/// # Examples
+////// ```
+/// use arrow_buffer::i256;
+/// use arrow_cast::cast::decimal256_to_f64;
+///
+/// let val = i256::from(123456789);
+/// let result = decimal256_to_f64(val);
+/// assert_eq!(result, 123456789.0);
+/// ```
+pub fn decimal256_to_f64(val: i256) -> f64 {
+ match val.to_f64() {
+ Some(v) => v,
+ None => unreachable!("All i256 values fit in f64"),
+ }
Review Comment:
```suggestion
val.to_f64().expect("All i256 values fit in f64")
```
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -1995,15 +1995,37 @@ where
}
}
-/// Convert a [`i256`] to `f64` saturating to infinity on overflow.
-fn decimal256_to_f64(v: i256) -> f64 {
- v.to_f64().unwrap_or_else(|| {
- if v.is_negative() {
- f64::NEG_INFINITY
- } else {
- f64::INFINITY
- }
- })
+/// Converts a 256-bit signed integer to a 64-bit floating point number.
+///
+/// This function is primarily used for converting Decimal256 values to f64,
+/// where the Decimal256 is represented as an i256 (256-bit signed integer).
+///
+/// # Arguments
+///
+/// * `val` - The 256-bit signed integer value to convert
+///
+/// # Returns
+///
+/// Returns the floating point representation of the input value.
+///
+/// All `i256` values are within the representable range of `f64`. The
+/// conversion therefore cannot overflow, although large values may lose
+/// precision.
+///
+/// # Examples
+////// ```
+/// use arrow_buffer::i256;
+/// use arrow_cast::cast::decimal256_to_f64;
+///
+/// let val = i256::from(123456789);
+/// let result = decimal256_to_f64(val);
+/// assert_eq!(result, 123456789.0);
+/// ```
+pub fn decimal256_to_f64(val: i256) -> f64 {
Review Comment:
Aside: This function is badly-named. It doesn't convert the _decimal_ to
f64. Rather, it converts the decimal's _unscaled value_ to f64, and the caller
must then apply the appropriate scalaing as needed.
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -821,6 +821,20 @@ impl ToPrimitive for i256 {
}
}
+ fn to_f64(&self) -> Option<f64> {
+ let mag = if let Some(u) = self.checked_abs() {
+ let (low, high) = u.to_parts();
+ (high as f64) * 2_f64.powi(128) + (low as f64)
Review Comment:
Thinking this through:
* If `high` is zero (no significant bits), then conversion to f64 is exact
(tho useless)
* ... and we return the value of `low`, converted to f64
* If `high` has `1..=53` significant bits, then conversion to f64 is exact
(no rounding)
* ... and scaling is also exact
* ... and adding `low` (already converted to f64) will round as needed
* If `high` has `54..` significant bits, then conversion to f64 will use the
54th bit to round
* ... tho scaling is still exact
* ... and it doesn't matter what value `low` takes, because it's so small
that adding it doesn't change the answer
A bit expensive, but I think it covers all the cases with no weird rounding
effects?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -1995,15 +1995,37 @@ where
}
}
-/// Convert a [`i256`] to `f64` saturating to infinity on overflow.
-fn decimal256_to_f64(v: i256) -> f64 {
- v.to_f64().unwrap_or_else(|| {
- if v.is_negative() {
- f64::NEG_INFINITY
- } else {
- f64::INFINITY
- }
- })
+/// Converts a 256-bit signed integer to a 64-bit floating point number.
+///
+/// This function is primarily used for converting Decimal256 values to f64,
+/// where the Decimal256 is represented as an i256 (256-bit signed integer).
+///
+/// # Arguments
+///
+/// * `val` - The 256-bit signed integer value to convert
+///
+/// # Returns
+///
+/// Returns the floating point representation of the input value.
+///
+/// All `i256` values are within the representable range of `f64`. The
+/// conversion therefore cannot overflow, although large values may lose
+/// precision.
+///
+/// # Examples
+////// ```
+/// use arrow_buffer::i256;
+/// use arrow_cast::cast::decimal256_to_f64;
+///
+/// let val = i256::from(123456789);
+/// let result = decimal256_to_f64(val);
+/// assert_eq!(result, 123456789.0);
+/// ```
+pub fn decimal256_to_f64(val: i256) -> f64 {
Review Comment:
That said, I believe this function is newly-added after the most recent
arrow release. So it's not yet in the wild. We should just remove it, and
revert the call site back to calling `ToPrimitive::to_f64` directly, like it
originally did.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
