You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "theirix (via GitHub)" <gi...@apache.org> on 2025/08/03 11:28:02 UTC
[PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
theirix opened a new pull request, #17023:
URL: https://github.com/apache/datafusion/pull/17023
## Which issue does this PR close?
- Closes #.
## Rationale for this change
Add Decimal128 and Decimal256 support for `log` UDF.
It's a most generic function, allowing for specifying a logarithm base, but by default it is `log10`, which makes it a good candidate for long decimals. Currently, it only supports floats.
Since decimals only support integer logarithms, it's rounded and then converted back to a float.
Also, if numbers are parsed as floats, there is precision lost due to floating-point handling. So the majority of tests are targeting `parse_float_as_decimal=true` as in #14612. Otherwise, the log could differ by one or two due to rounding – see regression SLT.
Notably, we still miss math for 256-bits. [Arrow's i256 type](https://arrow.apache.org/rust/arrow_buffer/bigint/index.html) uses the [BigInt implementation], which could provide [log10](https://github.com/rust-num/num-bigint/issues/57) at least, but we can extend decimal arithmetic in Arrow as well.
## What changes are included in this PR?
- Support for decimals
- Additional unit tests
- A minor follow-up fix to #16831 for zero scale
- [chore] Allow env logging in functions crate
Please note, there are [more specialised](https://github.com/apache/datafusion/blob/6d9b76e4a30f6234ffa3f8100b5d4c2735558ca6/datafusion/functions/src/math/mod.rs#L178) functions `log2`, `log10`, `ln` (), which are not handled by `LogFunc`. They could be migrated to this UDF later, providing a base value explicitly.
## Are these changes tested?
- Unit tests
- Regression SLT tests
- Manual invocation of datafusion-cli
- Manual comparison of results to other large decimal math implementations (Python, WolframAlpha)
## Are there any user-facing changes?
No
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2339668835
##########
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),
+ ]),
Review Comment:
I think adding a null coercion for numerics could help, because it worked with float32 in main. Null handling is hard because we have to conform with PostgreSQL and/or standard
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2340008443
##########
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),
+ ]),
Review Comment:
I think we can safely remove these signatures:
```rust
Exact(vec![DataType::Float32]),
Exact(vec![DataType::Float64]),
```
As they are indeed superseded by `Numeric(1)`; `Numeric(2)` will need to remain with the rest of the signatures as it only guarantees coercing to a common numeric type across all the arguments which I don't think we want. This is the part I misunderstood, I'll look into raising a PR to clarify some of the documentation later perhaps.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2315261104
##########
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),
+ ]),
Review Comment:
Are the `Exact(...)` signatures superseded by the `Numeric(1)` and `Numeric(2)`? Considering integers, floats and decimals are considered numeric types?
##########
datafusion/functions/src/math/log.rs:
##########
@@ -121,55 +198,68 @@ impl ScalarUDFImpl for LogFunc {
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
let args = ColumnarValue::values_to_arrays(&args.args)?;
- let mut base = ColumnarValue::Scalar(ScalarValue::Float32(Some(10.0)));
-
- let mut x = &args[0];
- if args.len() == 2 {
- x = &args[1];
- base = ColumnarValue::Array(Arc::clone(&args[0]));
- }
- // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
- let arr: ArrayRef = match args[0].data_type() {
- DataType::Float64 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => {
- Arc::new(x.as_primitive::<Float64Type>().unary::<_, Float64Type>(
- |value: f64| f64::log(value, base as f64),
- ))
- }
- ColumnarValue::Array(base) => {
- let x = x.as_primitive::<Float64Type>();
- let base = base.as_primitive::<Float64Type>();
- let result = arrow::compute::binary::<_, _, _, Float64Type>(
- x,
- base,
- f64::log,
- )?;
- Arc::new(result) as _
- }
- _ => {
- return exec_err!("log function requires a scalar or array for base")
- }
- },
-
- DataType::Float32 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => Arc::new(
- x.as_primitive::<Float32Type>()
- .unary::<_, Float32Type>(|value: f32| f32::log(value, base)),
+ let (base, value) = if args.len() == 2 {
+ // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
+ (ColumnarValue::Array(Arc::clone(&args[0])), &args[1])
+ } else {
+ // log(num) - assume base is 10
+ let ret_type = if args[0].data_type().is_null() {
+ &DataType::Float64
+ } else {
+ args[0].data_type()
+ };
Review Comment:
Should this logic match the above `return_type()` function?
##########
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:
```suggestion
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}"
))),
}
```
Thoughts on reusing existing function above? Also changing error type as `NotYetImplemented` seems more appropriate (though maybe should use the DataFusion version instead of Arrow version 🤔 )
##########
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)
+ }
+ }
Review Comment:
```suggestion
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}"
)));
}
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)
}
```
Just to clarify the errors a bit
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2341870223
##########
datafusion/functions/src/math/log.rs:
##########
@@ -58,21 +64,81 @@ 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 {
+ return Err(ArrowError::ComputeError(format!(
Review Comment:
I started with pure DataFusionErrors, but then realised that the generic `calculate_binary_math` function requires all functors to return Arrow errors, since it uses Arrow's `try_unary`, `try_binary`. It means that our functors `log_decimal128` and `log_decimal256` must return `ArrowError`.
At the same time, `calculate_binary_math` returns a DataFusionError, converting all the low-level arrow errors to DF's error. It means users will see pure `DataFusionError` (as a variant with `ArrowError`)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2342759229
##########
datafusion/functions/src/math/log.rs:
##########
@@ -58,21 +64,81 @@ 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 {
+ return Err(ArrowError::ComputeError(format!(
Review Comment:
That makes sense 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2337591199
##########
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),
+ ]),
Review Comment:
Yes, `Numeric` is a class of types including decimals, floats and decimals.
The reason I included the third signature onwards is inability of `Numeric(1)` to support a null value, and we need to support `select log(null);` as per [SLT](https://github.com/apache/datafusion/blob/a281b251a293d10a585466d96694f6cea882f4ee/datafusion/sqllogictest/test_files/scalar.slt#L492) and postgres feature parity.
If you have an idea how to make it work with nulls, I'd appreciate help. I don't like the current list of signatures
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#issuecomment-3289507879
Thank you, @Jefffrey !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2338310502
##########
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),
+ ]),
Review Comment:
Hmm, good point. It also seems to make queries like this fail:
```
1. query failed: DataFusion error: Error during planning: Internal error: Function 'log' failed to match any signature, errors: Error during planning: Function 'log' expects 1 arguments but received 2,Error during planning: For function 'log' Decimal128(3, 1) and Decimal256(76, 0) are not coercible to a common numeric type.
This issue was likely caused by a bug in DataFusion's code. Please help us to resolve this by filing a bug report in our issue tracker: https://github.com/apache/datafusion/issues No function matches the given name and argument types 'log(Decimal128(3, 1), Decimal256(76, 0))'. You might need to add explicit type casts.
Candidate functions:
log(Numeric(1))
log(Numeric(2))
[SQL] select log(10.0, 100000000000000000000000000000000000::decimal(76,0));
at /Users/jeffrey/Code/datafusion/datafusion/sqllogictest/test_files/decimal.slt:836
```
I'll try look at the coercion code a bit and try to understand what's going on here 🤔
(I'm assuming `Numeric(2)` actually means coerce to a common type)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2340008443
##########
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),
+ ]),
Review Comment:
I think we can safely remove these signatures:
```rust
Exact(vec![DataType::Float32]),
Exact(vec![DataType::Float64]),
```
As they are indeed superseded by `Numeric(1)`; `Numeric(2)` will need to remain with the rest of the signatures as it only guarantees coercing to a common numeric type across all the arguments which I don't think we want for all invocations of `log`. This is the part I misunderstood, I'll look into raising a PR to clarify some of the documentation later perhaps.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2331486576
##########
datafusion/functions/src/math/log.rs:
##########
@@ -121,55 +198,68 @@ impl ScalarUDFImpl for LogFunc {
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
let args = ColumnarValue::values_to_arrays(&args.args)?;
- let mut base = ColumnarValue::Scalar(ScalarValue::Float32(Some(10.0)));
-
- let mut x = &args[0];
- if args.len() == 2 {
- x = &args[1];
- base = ColumnarValue::Array(Arc::clone(&args[0]));
- }
- // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
- let arr: ArrayRef = match args[0].data_type() {
- DataType::Float64 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => {
- Arc::new(x.as_primitive::<Float64Type>().unary::<_, Float64Type>(
- |value: f64| f64::log(value, base as f64),
- ))
- }
- ColumnarValue::Array(base) => {
- let x = x.as_primitive::<Float64Type>();
- let base = base.as_primitive::<Float64Type>();
- let result = arrow::compute::binary::<_, _, _, Float64Type>(
- x,
- base,
- f64::log,
- )?;
- Arc::new(result) as _
- }
- _ => {
- return exec_err!("log function requires a scalar or array for base")
- }
- },
-
- DataType::Float32 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => Arc::new(
- x.as_primitive::<Float32Type>()
- .unary::<_, Float32Type>(|value: f32| f32::log(value, base)),
+ let (base, value) = if args.len() == 2 {
+ // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
+ (ColumnarValue::Array(Arc::clone(&args[0])), &args[1])
+ } else {
+ // log(num) - assume base is 10
+ let ret_type = if args[0].data_type().is_null() {
+ &DataType::Float64
+ } else {
+ args[0].data_type()
+ };
Review Comment:
The handling of an additional optional argument makes it complicated and different from `return_type`. I don't see a lot of UDFs with variable-number arguments.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2340032479
##########
datafusion/functions/src/math/log.rs:
##########
@@ -58,21 +64,81 @@ 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 {
+ return Err(ArrowError::ComputeError(format!(
Review Comment:
I wonder if we should be using raising the errors as `DataFusionError`s instead of creating `ArrowError`s outside of the `arrow` crates (applicable to here and all other places in the code in this PR). It might be confusing to see an arrow compute error when the code itself is in datafusion
##########
datafusion/functions/src/utils.rs:
##########
@@ -120,6 +122,76 @@ where
}
}
+/// Computes a binary math function for input arrays using a specified function.
+/// Generic types:
+/// - `L`: Left array primitive type
+/// - `R`: Right array primitive type
+/// - `O`: Output array primitive type
+/// - `F`: Functor computing `fun(l: L, r: R) -> Result<OutputType>`
+pub fn calculate_binary_math<L, R, O, F>(
+ left: &dyn Array,
+ right: &ColumnarValue,
+ fun: F,
+) -> Result<Arc<PrimitiveArray<O>>>
+where
+ R: ArrowPrimitiveType,
+ L: ArrowPrimitiveType,
+ O: ArrowPrimitiveType,
+ F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
+ R::Native: TryFrom<ScalarValue>,
+{
+ Ok(match right {
+ ColumnarValue::Scalar(scalar) => {
+ let right_value: R::Native =
+ R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ let left_array = left.as_primitive::<L>();
+ // Bind right value
+ let result =
+ left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
+ Arc::new(result) as _
+ }
+ ColumnarValue::Array(right) => {
+ let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
+ let right_array = right_casted.as_primitive::<R>();
+
+ // Types are compatible even they are decimals with different scale or precision
+ let result = if PrimitiveArray::<L>::is_compatible(&L::DATA_TYPE) {
+ let left_array = left.as_primitive::<L>();
+ try_binary::<_, _, _, O>(left_array, right_array, &fun)?
+ } else {
+ let left_casted = arrow::compute::cast(left, &L::DATA_TYPE)?;
+ let left_array = left_casted.as_primitive::<L>();
+ try_binary::<_, _, _, O>(left_array, right_array, &fun)?
+ };
+ Arc::new(result) as _
+ }
+ })
+}
+
+/// Converts Decimal128 components (value and scale) to an unscaled i128
+pub fn decimal128_to_i128(value: i128, scale: i8) -> Result<i128, ArrowError> {
+ if scale < 0 {
+ Err(ArrowError::ComputeError(
+ "Negative scale is not supported".into(),
+ ))
+ } else if scale == 0 {
+ Ok(value)
+ } else {
+ match i128::from(10).checked_pow(scale as u32) {
+ Some(divisor) => Ok(value / divisor),
+ None => Err(ArrowError::ComputeError(format!(
+ "Cannot get a power of {scale}"
+ ))),
Review Comment:
Is this branch covered in the test below?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#issuecomment-3289097939
Created #17555 just so we have something to track the NotYetImplemented item
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2341862687
##########
datafusion/functions/src/utils.rs:
##########
@@ -120,6 +122,76 @@ where
}
}
+/// Computes a binary math function for input arrays using a specified function.
+/// Generic types:
+/// - `L`: Left array primitive type
+/// - `R`: Right array primitive type
+/// - `O`: Output array primitive type
+/// - `F`: Functor computing `fun(l: L, r: R) -> Result<OutputType>`
+pub fn calculate_binary_math<L, R, O, F>(
+ left: &dyn Array,
+ right: &ColumnarValue,
+ fun: F,
+) -> Result<Arc<PrimitiveArray<O>>>
+where
+ R: ArrowPrimitiveType,
+ L: ArrowPrimitiveType,
+ O: ArrowPrimitiveType,
+ F: Fn(L::Native, R::Native) -> Result<O::Native, ArrowError>,
+ R::Native: TryFrom<ScalarValue>,
+{
+ Ok(match right {
+ ColumnarValue::Scalar(scalar) => {
+ let right_value: R::Native =
+ R::Native::try_from(scalar.clone()).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Cannot convert scalar value {} to {}",
+ &scalar,
+ R::DATA_TYPE
+ ))
+ })?;
+ let left_array = left.as_primitive::<L>();
+ // Bind right value
+ let result =
+ left_array.try_unary::<_, O, _>(|lvalue| fun(lvalue, right_value))?;
+ Arc::new(result) as _
+ }
+ ColumnarValue::Array(right) => {
+ let right_casted = arrow::compute::cast(&right, &R::DATA_TYPE)?;
+ let right_array = right_casted.as_primitive::<R>();
+
+ // Types are compatible even they are decimals with different scale or precision
+ let result = if PrimitiveArray::<L>::is_compatible(&L::DATA_TYPE) {
+ let left_array = left.as_primitive::<L>();
+ try_binary::<_, _, _, O>(left_array, right_array, &fun)?
+ } else {
+ let left_casted = arrow::compute::cast(left, &L::DATA_TYPE)?;
+ let left_array = left_casted.as_primitive::<L>();
+ try_binary::<_, _, _, O>(left_array, right_array, &fun)?
+ };
+ Arc::new(result) as _
+ }
+ })
+}
+
+/// Converts Decimal128 components (value and scale) to an unscaled i128
+pub fn decimal128_to_i128(value: i128, scale: i8) -> Result<i128, ArrowError> {
+ if scale < 0 {
+ Err(ArrowError::ComputeError(
+ "Negative scale is not supported".into(),
+ ))
+ } else if scale == 0 {
+ Ok(value)
+ } else {
+ match i128::from(10).checked_pow(scale as u32) {
+ Some(divisor) => Ok(value / divisor),
+ None => Err(ArrowError::ComputeError(format!(
+ "Cannot get a power of {scale}"
+ ))),
Review Comment:
Good spot, just added a test case
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17023:
URL: https://github.com/apache/datafusion/pull/17023#discussion_r2338313766
##########
datafusion/functions/src/math/log.rs:
##########
@@ -121,55 +198,68 @@ impl ScalarUDFImpl for LogFunc {
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
let args = ColumnarValue::values_to_arrays(&args.args)?;
- let mut base = ColumnarValue::Scalar(ScalarValue::Float32(Some(10.0)));
-
- let mut x = &args[0];
- if args.len() == 2 {
- x = &args[1];
- base = ColumnarValue::Array(Arc::clone(&args[0]));
- }
- // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
- let arr: ArrayRef = match args[0].data_type() {
- DataType::Float64 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => {
- Arc::new(x.as_primitive::<Float64Type>().unary::<_, Float64Type>(
- |value: f64| f64::log(value, base as f64),
- ))
- }
- ColumnarValue::Array(base) => {
- let x = x.as_primitive::<Float64Type>();
- let base = base.as_primitive::<Float64Type>();
- let result = arrow::compute::binary::<_, _, _, Float64Type>(
- x,
- base,
- f64::log,
- )?;
- Arc::new(result) as _
- }
- _ => {
- return exec_err!("log function requires a scalar or array for base")
- }
- },
-
- DataType::Float32 => match base {
- ColumnarValue::Scalar(ScalarValue::Float32(Some(base))) => Arc::new(
- x.as_primitive::<Float32Type>()
- .unary::<_, Float32Type>(|value: f32| f32::log(value, base)),
+ let (base, value) = if args.len() == 2 {
+ // note in f64::log params order is different than in sql. e.g in sql log(base, x) == f64::log(x, base)
+ (ColumnarValue::Array(Arc::clone(&args[0])), &args[1])
+ } else {
+ // log(num) - assume base is 10
+ let ret_type = if args[0].data_type().is_null() {
+ &DataType::Float64
+ } else {
+ args[0].data_type()
+ };
Review Comment:
Ah right, I think I just misunderstood due to the name `ret_type` 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support log for Decimal128 and Decimal256 [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #17023:
URL: https://github.com/apache/datafusion/pull/17023
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org