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-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

Reply via email to