Jefffrey commented on code in PR #22564:
URL: https://github.com/apache/datafusion/pull/22564#discussion_r3341282096
##########
datafusion/functions/src/math/log.rs:
##########
@@ -157,7 +174,20 @@ fn log_decimal128(value: i128, scale: i8, base: f64) ->
Result<f64, ArrowError>
return Ok(int_log as f64);
}
}
- decimal_to_f64(value, scale).map(|v| v.log(base))
+ decimal_to_f64(value, scale).and_then(|v| {
+ validate_log_value(v)?;
+ Ok(v.log(base))
+ })
+}
+
+/// Compute logarithm for Float16, Float32, and Float64 values
+#[inline]
+fn compute_float_log<T: Float + ToPrimitive>(value: T, base: T) -> Result<T,
ArrowError> {
+ let value_f64 = value.to_f64().ok_or_else(|| {
+ ArrowError::ComputeError("Cannot convert value to f64".to_string())
+ })?;
Review Comment:
we can avoid this conversion to f64 by using something like
[`is_zero()`](https://docs.rs/num-traits/latest/num_traits/identities/trait.Zero.html#tymethod.is_zero)
##########
datafusion/functions/src/math/log.rs:
##########
@@ -358,43 +386,83 @@ impl ScalarUDFImpl for LogFunc {
} else {
lit(ScalarValue::new_ten(&number_datatype)?)
};
+ let base_datatype = info.get_data_type(&base)?;
+
+ if is_zero_literal(&number, &number_datatype)?
+ || is_zero_literal(&base, &base_datatype)?
+ {
+ return Ok(ExprSimplifyResult::Original(original_log_args(
+ num_args, &base, &number,
+ )?));
+ }
+
+ let base_is_valid_literal = is_valid_log_base_literal(&base)?;
match number {
Expr::Literal(value, _)
- if value == ScalarValue::new_one(&number_datatype)? =>
+ if value == ScalarValue::new_one(&number_datatype)?
+ && base_is_valid_literal =>
{
Ok(ExprSimplifyResult::Simplified(lit(ScalarValue::new_zero(
&info.get_data_type(&base)?,
)?)))
}
Expr::ScalarFunction(ScalarFunction { func, mut args })
- if is_pow(&func) && args.len() == 2 && base == args[0] =>
+ if is_pow(&func)
+ && args.len() == 2
+ && base == args[0]
+ && base_is_valid_literal =>
{
let b = args.pop().unwrap(); // length checked above
Ok(ExprSimplifyResult::Simplified(b))
}
number => {
- if number == base {
+ if number == base && base_is_valid_literal {
Ok(ExprSimplifyResult::Simplified(lit(ScalarValue::new_one(
&number_datatype,
)?)))
} else {
- let args = match num_args {
- 1 => vec![number],
- 2 => vec![base, number],
- _ => {
- return internal_err!(
- "Unexpected number of arguments in
log::simplify"
- );
- }
- };
- Ok(ExprSimplifyResult::Original(args))
+ Ok(ExprSimplifyResult::Original(original_log_args(
+ num_args, &base, &number,
+ )?))
}
}
}
}
}
+#[inline]
+fn original_log_args(num_args: usize, base: &Expr, number: &Expr) ->
Result<Vec<Expr>> {
+ match num_args {
+ 1 => Ok(vec![number.clone()]),
+ 2 => Ok(vec![base.clone(), number.clone()]),
+ _ => {
+ internal_err!("Unexpected number of arguments in log::simplify")
+ }
+ }
+}
+
+#[inline]
+fn is_zero_literal(expr: &Expr, data_type: &DataType) -> Result<bool> {
+ match expr {
+ Expr::Literal(value, _) => Ok(*value ==
ScalarValue::new_zero(data_type)?),
Review Comment:
```suggestion
Expr::Literal(value, _) => Ok(*value ==
ScalarValue::new_zero(&value.data_type())?),
```
##########
datafusion/functions/src/math/log.rs:
##########
@@ -358,43 +386,83 @@ impl ScalarUDFImpl for LogFunc {
} else {
lit(ScalarValue::new_ten(&number_datatype)?)
};
+ let base_datatype = info.get_data_type(&base)?;
+
+ if is_zero_literal(&number, &number_datatype)?
+ || is_zero_literal(&base, &base_datatype)?
+ {
+ return Ok(ExprSimplifyResult::Original(original_log_args(
Review Comment:
its probably easier to just keep a clone of the original `args` to reuse
here instead of having a separate function to reconstruct it
##########
datafusion/sqllogictest/test_files/scalar.slt:
##########
Review Comment:
we're getting rid of the `log(1, 64)` test case here? can we preserve this
--
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]
