martin-g commented on code in PR #19369:
URL: https://github.com/apache/datafusion/pull/19369#discussion_r2627119710
##########
datafusion/functions/src/math/power.rs:
##########
@@ -149,22 +151,112 @@ where
/// Binary function to calculate a math power to float exponent
/// for scaled integer types.
-/// Returns error if exponent is negative or non-integer, or base invalid
fn pow_decimal_float<T>(base: T, scale: i8, exp: f64) -> Result<T, ArrowError>
where
T: From<i32> + ArrowNativeTypeOp,
{
- if !exp.is_finite() || exp.trunc() != exp {
+ if exp.is_finite() && exp.trunc() == exp && exp >= 0f64 && exp < u32::MAX
as f64 {
+ return pow_decimal_int(base, scale, exp as i64);
+ }
+
+ if !exp.is_finite() {
return Err(ArrowError::ComputeError(format!(
- "Cannot use non-integer exp: {exp}"
+ "Cannot use non-finite exp: {exp}"
+ )));
+ }
+
+ pow_decimal_float_fallback(base, scale, exp)
+}
+
+/// Fallback implementation using f64 for negative or non-integer exponents.
+/// This handles cases that cannot be computed using integer arithmetic.
+fn pow_decimal_float_fallback<T>(base: T, scale: i8, exp: f64) -> Result<T,
ArrowError>
+where
+ T: From<i32> + ArrowNativeTypeOp,
+{
+ let scale_factor = 10f64.powi(scale as i32);
+ let base_f64 = format!("{base:?}")
+ .parse::<f64>()
+ .map(|v| v / scale_factor)
+ .map_err(|_| {
+ ArrowError::ComputeError(format!("Cannot convert base {base:?} to
f64"))
+ })?;
+
+ let result_f64 = base_f64.powf(exp);
+
+ if !result_f64.is_finite() {
+ return Err(ArrowError::ArithmeticOverflow(format!(
+ "Result of {base_f64}^{exp} is not finite"
)));
}
- if exp < 0f64 || exp >= u32::MAX as f64 {
+
+ let result_scaled = result_f64 * scale_factor;
+ let result_rounded = result_scaled.round();
+
+ if result_rounded.abs() > i128::MAX as f64 {
return Err(ArrowError::ArithmeticOverflow(format!(
- "Unsupported exp value: {exp}"
+ "Result {result_rounded} is too large for the target decimal type"
)));
}
- pow_decimal_int(base, scale, exp as i64)
+
+ decimal_from_i128::<T>(result_rounded as i128)
+}
+
+fn decimal_from_i128<T>(value: i128) -> Result<T, ArrowError>
Review Comment:
Is this intentionally not supporting Decimal256 (i.e. i256) ?
##########
datafusion/functions/src/math/power.rs:
##########
@@ -149,22 +151,112 @@ where
/// Binary function to calculate a math power to float exponent
/// for scaled integer types.
-/// Returns error if exponent is negative or non-integer, or base invalid
fn pow_decimal_float<T>(base: T, scale: i8, exp: f64) -> Result<T, ArrowError>
where
T: From<i32> + ArrowNativeTypeOp,
{
- if !exp.is_finite() || exp.trunc() != exp {
+ if exp.is_finite() && exp.trunc() == exp && exp >= 0f64 && exp < u32::MAX
as f64 {
+ return pow_decimal_int(base, scale, exp as i64);
+ }
+
+ if !exp.is_finite() {
return Err(ArrowError::ComputeError(format!(
- "Cannot use non-integer exp: {exp}"
+ "Cannot use non-finite exp: {exp}"
+ )));
+ }
+
+ pow_decimal_float_fallback(base, scale, exp)
+}
+
+/// Fallback implementation using f64 for negative or non-integer exponents.
+/// This handles cases that cannot be computed using integer arithmetic.
+fn pow_decimal_float_fallback<T>(base: T, scale: i8, exp: f64) -> Result<T,
ArrowError>
+where
+ T: From<i32> + ArrowNativeTypeOp,
+{
+ let scale_factor = 10f64.powi(scale as i32);
Review Comment:
There are no new tests so it seems it is not intended but the fallback
approach also supports negative scales too, not just exponents.
The integer based approach does not support negative scales (yet) -
https://github.com/apache/datafusion/pull/19369/changes#diff-598356e1af0ad23287c968ebf6310366e5127b91a037e076bc264dac3a55e768R125
I'd suggest to either add a check and return error like the integer approach
or add unit tests and document it as supported.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
