Jefffrey commented on code in PR #19315:
URL: https://github.com/apache/datafusion/pull/19315#discussion_r2623651443
##########
datafusion/functions/src/math/log.rs:
##########
@@ -291,43 +303,78 @@ impl ScalarUDFImpl for LogFunc {
}
Review Comment:
Actually I think we can just insert this right after the `num_args` check:
```rust
// ScalarValue doesn't support negative scale, remove when it does
// See: https://github.com/apache/datafusion/issues/19354
match arg_types.last().unwrap() {
DataType::Decimal32(_, scale)
| DataType::Decimal64(_, scale)
| DataType::Decimal128(_, scale)
| DataType::Decimal256(_, scale)
if *scale < 0 =>
{
return Ok(ExprSimplifyResult::Original(args));
}
_ => (),
};
```
And leave the rest of the code untouched from main, for simplicity 🤔
(Also I raised #19354 regarding support for negative scale in ScalarValue)
##########
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);
+----
+4
+
+# log with negative scale and explicit base 10
+query R
+select log(10, 1e4);
+----
+4
+
+# log with negative scale and base 2
+# 8e1 = 80 with scale -1, log2(80) ≈ 6.321928
+query R
+select log(2.0, 8e1);
+----
+6.321928094887
+
+# log with negative scale and base 2 (another value)
+# 16e1 = 160 with scale -1, log2(160) ≈ 7.321928
+query R
+select log(2.0, 16e1);
+----
+7.321928094887
+
+# log with negative scale -3
+# 5e3 = 5000 with scale -3, log10(5000) ≈ 3.69897
+query R
+select log(5e3);
+----
+3.698970004336
+
+# log with negative scale array values
+query R rowsort
+select log(value) from (values (1e3), (1e4), (1e5)) as t(value);
+----
+3
+4
+5
+
+# log with negative scale and different bases
+# Note: Results may come in different order depending on execution
Review Comment:
We should add a rowsort here to ensure our test suite is deterministic
##########
datafusion/functions/src/math/log.rs:
##########
@@ -116,13 +116,25 @@ 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)
+ if scale < 0 {
+ if value > 0 {
+ // Compute actual value: value * 10^(-scale) = value * 10^|scale|
+ let actual_value = (value as f64) * 10.0_f64.powi(-scale as i32);
+ Ok(actual_value.log(base))
+ } else {
+ // Reflect f64::log behaviour
+ Ok(f64::NAN)
Review Comment:
nit: can pull this check out of both if arms
--
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]
