Jefffrey commented on code in PR #17918:
URL: https://github.com/apache/datafusion/pull/17918#discussion_r2413051817
##########
datafusion/functions/src/math/log.rs:
##########
@@ -134,15 +134,60 @@ fn log_decimal128(value: i128, scale: i8, base: f64) ->
Result<f64, ArrowError>
}
}
-/// Binary function to calculate an integer logarithm of Decimal128 `value`
using `base` base
-/// Returns error if base is invalid or if value is out of bounds of Decimal128
+/// Binary function to calculate an integer logarithm of Decimal256 `value`
using `base` base
+/// Returns error if base is invalid
fn log_decimal256(value: i256, scale: i8, base: f64) -> Result<f64,
ArrowError> {
- match value.to_i128() {
- Some(value) => log_decimal128(value, scale, base),
- None => Err(ArrowError::NotYetImplemented(format!(
- "Log of Decimal256 larger than Decimal128 is not yet supported:
{value}"
- ))),
+ if !base.is_finite() || base.trunc() != base {
+ return Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer base: {base}"
+ )));
+ }
+ if (base as u32) < 2 {
+ return Err(ArrowError::ComputeError(format!(
+ "Log base must be greater than 1: {base}"
+ )));
+ }
+
+ // Try to convert to i128 for faster calculation if possible
+ if let Some(value_i128) = value.to_i128() {
+ let unscaled_value = decimal128_to_i128(value_i128, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ return Ok(log_value as f64);
+ } else {
+ return Ok(f64::NAN);
+ }
+ }
+
+ // For values that don't fit in i128, use f64 approximation
+ let unscaled_value = decimal256_to_i256(value, scale)?;
+
+ // Check if the value is non-positive
+ if !unscaled_value.is_positive() {
+ return Ok(f64::NAN);
+ }
+
+ // Convert i256 to f64 for logarithm calculation
+ // Note: This may lose precision for very large numbers, but that's
acceptable
+ // for logarithm calculation since the result is relatively small
+ let value_f64 = i256_to_f64(unscaled_value);
+ let log_value = value_f64.log(base);
+
+ Ok(log_value.floor())
+}
+
+/// Converts i256 to f64 for logarithm calculation
+/// This may lose precision for very large numbers but is acceptable for log
calculation
+fn i256_to_f64(value: i256) -> f64 {
+ // Try to convert directly if it fits in i128
+ if let Some(value_i128) = value.to_i128() {
+ return value_i128 as f64;
Review Comment:
Is it redundant to check this again, as it was checked already in
`log_decimal256()`?
--
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]
