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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
