theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2331472397
##########
datafusion/functions/src/math/log.rs:
##########
@@ -58,21 +64,91 @@ impl Default for LogFunc {
impl LogFunc {
pub fn new() -> Self {
- use DataType::*;
Self {
signature: Signature::one_of(
vec![
- Exact(vec![Float32]),
- Exact(vec![Float64]),
- Exact(vec![Float32, Float32]),
- Exact(vec![Float64, Float64]),
+ Numeric(1),
+ Numeric(2),
+ Exact(vec![DataType::Float32]),
+ Exact(vec![DataType::Float64]),
+ Exact(vec![DataType::Float32, DataType::Float32]),
+ Exact(vec![DataType::Float64, DataType::Float64]),
+ Exact(vec![
+ DataType::Int64,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
+ ]),
+ Exact(vec![
+ DataType::Float32,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
+ ]),
+ Exact(vec![
+ DataType::Float64,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, 0),
+ ]),
+ Exact(vec![
+ DataType::Int64,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
+ ]),
+ Exact(vec![
+ DataType::Float32,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
+ ]),
+ Exact(vec![
+ DataType::Float64,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, 0),
+ ]),
],
Volatility::Immutable,
),
}
}
}
+/// Binary function to calculate an integer logarithm of Decimal128 `value`
using `base` base
+/// Returns error if base is invalid
+fn log_decimal128(value: i128, scale: i8, base: f64) -> Result<f64,
ArrowError> {
+ if !base.is_finite() || base.trunc() != base || (base as u32) < 2 {
+ Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer or small base {base}"
+ )))
+ } else {
+ let unscaled_value = decimal128_to_i128(value, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ Ok(log_value as f64)
+ } else {
+ // Reflect f64::log behaviour
+ Ok(f64::NAN)
+ }
+ }
+}
+
+/// 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
+fn log_decimal256(value: i256, scale: i8, base: f64) -> Result<f64,
ArrowError> {
+ if !base.is_finite() || base.trunc() != base || (base as u32) < 2 {
+ Err(ArrowError::ComputeError(format!(
+ "Log cannot use non-integer or small base {base}"
+ )))
+ } else {
+ match value.to_i128() {
+ Some(short_value) => {
+ // Calculate logarithm only for 128-bit decimals
+ let unscaled_value = decimal128_to_i128(short_value, scale)?;
+ if unscaled_value > 0 {
+ let log_value: u32 = unscaled_value.ilog(base as i128);
+ Ok(log_value as f64)
+ } else {
+ Ok(f64::NAN)
+ }
+ }
+ None => Err(ArrowError::ComputeError(format!(
+ "Log of a large Decimal256 is not supported: {value}"
+ ))),
+ }
+ }
Review Comment:
Good refactoring, thank you. It's worth to have `NotYetImplemented` in DF as
a sort of TODO
--
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]
