Jefffrey commented on code in PR #19315:
URL: https://github.com/apache/datafusion/pull/19315#discussion_r2621767358
##########
datafusion/functions/src/math/log.rs:
##########
@@ -1125,4 +1193,120 @@ mod tests {
"Arrow error: Not yet implemented: Log of Decimal256 larger than
Decimal128 is not yet supported: 170141183460469231731687303715884106727"
);
}
+
+ #[test]
+ fn test_log_decimal128_negative_scale() {
Review Comment:
Could we move these to SLTs instead?
##########
datafusion/functions/src/math/log.rs:
##########
@@ -116,13 +116,38 @@ fn log_decimal128(value: i128, scale: i8, base: f64) ->
Result<f64, ArrowError>
)));
}
- 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)
+ // Handle negative scales using logarithmic property:
+ // log_base(value * 10^(-scale)) = log_base(value) + (-scale) *
log_base(10)
+ if scale < 0 {
+ // For negative scale, the actual value is value * 10^(-scale) where
-scale > 0
+ // Use property: log_base(a * 10^(-s)) = log_base(a) + (-s) *
log_base(10)
Review Comment:
These comments seem to repeat each other
##########
datafusion/functions/src/math/log.rs:
##########
@@ -291,6 +316,49 @@ impl ScalarUDFImpl for LogFunc {
}
let number = args.pop().unwrap();
let number_datatype = arg_types.pop().unwrap();
+
+ // Check if base has negative scale (if provided)
+ let base_has_negative_scale = if num_args == 2 {
Review Comment:
Base is always float type so this check is redundant
##########
datafusion/sqllogictest/test_files/decimal.slt:
##########
@@ -918,6 +918,58 @@ select log(2.0, null);
----
NULL
+# log with negative scale decimals
+# Using scientific notation to create decimals with negative scales
+# 1e4 = 10000 with scale -4, log10(10000) = 4.0
+query R
+select log(1e4);
Review Comment:
Are we sure `1e4` is parsed as a decimal here?
##########
datafusion/functions/src/math/log.rs:
##########
@@ -291,6 +316,49 @@ impl ScalarUDFImpl for LogFunc {
}
let number = args.pop().unwrap();
let number_datatype = arg_types.pop().unwrap();
+
+ // Check if base has negative scale (if provided)
+ let base_has_negative_scale = if num_args == 2 {
+ if let Some(
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale),
+ ) = arg_types.last()
+ {
+ *scale < 0
+ } else {
+ false
+ }
+ } else {
+ false
+ };
+
+ // Skip simplification for negative scale decimals as ScalarValue
doesn't support them yet
+ let has_negative_scale = match &number_datatype {
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale) => *scale < 0,
+ _ => false,
+ };
+
+ if has_negative_scale || base_has_negative_scale {
Review Comment:
We should fold this into the existing match below to reduce duplication
##########
datafusion/functions/src/math/log.rs:
##########
@@ -116,13 +116,38 @@ fn log_decimal128(value: i128, scale: i8, base: f64) ->
Result<f64, ArrowError>
)));
}
- 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)
+ // Handle negative scales using logarithmic property:
+ // log_base(value * 10^(-scale)) = log_base(value) + (-scale) *
log_base(10)
+ if scale < 0 {
+ // For negative scale, the actual value is value * 10^(-scale) where
-scale > 0
+ // Use property: log_base(a * 10^(-s)) = log_base(a) + (-s) *
log_base(10)
+ if value > 0 {
+ let value_f64 = value as f64;
+
+ // Compute log_base(value) - use natural log and convert
+ // log_base(x) = ln(x) / ln(base)
+ let log_value = value_f64.ln() / base.ln();
+
+ // Add the adjustment: (-scale) * log_base(10)
+ // log_base(10) = ln(10) / ln(base)
+ let log_10_base = 10.0_f64.ln() / base.ln();
+ let adjustment = (-scale as f64) * log_10_base;
Review Comment:
I do wonder if we're better off just casting to float and doing log there?
There seem to be multiple log operations here, does it provide more accuracy?
--
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]
