alamb commented on a change in pull request #1734:
URL: https://github.com/apache/arrow-datafusion/pull/1734#discussion_r798933169



##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -2189,6 +2189,20 @@ pub fn exprlist_to_fields<'a>(
     expr.into_iter().map(|e| e.to_field(input_schema)).collect()
 }
 
+/// Calls a named built in function
+/// ```
+/// use datafusion::logical_plan::*;
+///
+/// // create the expression sin(x) < 0.2
+/// let expr = call_builtin_scalar_fn("sin", 
vec![col("x")]).unwrap().lt(lit(0.2));
+/// ```
+pub fn call_builtin_scalar_fn(name: impl AsRef<str>, args: Vec<Expr>) -> 
Result<Expr> {
+    match name.as_ref().parse::<functions::BuiltinScalarFunction>() {
+        Ok(fun) => Ok(Expr::ScalarFunction { fun, args }),
+        Err(e) => Err(e),
+    }

Review comment:
       One way to write this more idiomatically is
   
   ```suggestion
       name.as_ref().parse::<functions::BuiltinScalarFunction>()
           .map(|fun| Expr::ScalarFunction { fun, args }),
       }
   ```
   
   (not required, I am just pointing it out because it took me a while to get 
my head around working with Option and Results, and we are all learning 
together)

##########
File path: datafusion/src/logical_plan/mod.rs
##########
@@ -37,17 +37,17 @@ pub use dfschema::{DFField, DFSchema, DFSchemaRef, 
ToDFSchema};
 pub use display::display_schema;
 pub use expr::{
     abs, acos, and, approx_distinct, approx_percentile_cont, array, ascii, 
asin, atan,
-    avg, binary_expr, bit_length, btrim, case, ceil, character_length, chr, 
col,
-    columnize_expr, combine_filters, concat, concat_ws, cos, count, 
count_distinct,
-    create_udaf, create_udf, date_part, date_trunc, digest, exp, 
exprlist_to_fields,
-    floor, in_list, initcap, left, length, lit, lit_timestamp_nano, ln, log10, 
log2,
-    lower, lpad, ltrim, max, md5, min, normalize_col, normalize_cols, now, 
octet_length,
-    or, random, regexp_match, regexp_replace, repeat, replace, replace_col, 
reverse,
-    rewrite_sort_cols_by_aggs, right, round, rpad, rtrim, sha224, sha256, 
sha384, sha512,
-    signum, sin, split_part, sqrt, starts_with, strpos, substr, sum, tan, 
to_hex,
-    translate, trim, trunc, unalias, unnormalize_col, unnormalize_cols, upper, 
when,
-    Column, Expr, ExprRewriter, ExpressionVisitor, Literal, Recursion, 
RewriteRecursion,
-    SimplifyInfo,
+    avg, binary_expr, bit_length, btrim, call_builtin_scalar_fn, case, ceil,

Review comment:
       Seeing all these other names, what would you think of changing the name 
from `call_builtin_scalar_fn` to `call_fn` to make it less verbose 🤔 

##########
File path: datafusion/src/optimizer/simplify_expressions.rs
##########
@@ -1010,46 +1010,34 @@ mod tests {
     #[test]
     fn test_const_evaluator_scalar_functions() {
         // concat("foo", "bar") --> "foobar"
-        let expr = Expr::ScalarFunction {
-            args: vec![lit("foo"), lit("bar")],
-            fun: BuiltinScalarFunction::Concat,
-        };
+        let expr =

Review comment:
       well that is certainly nicer looking 👍 




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


Reply via email to