Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create built-in scalar functions programmatically #1734

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Create built-in scalar functions programmatically #1734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e1e9f3
create build-in scalar functions programatically
HaoYang670 Feb 3, 2022
78db02b
merge master branch
HaoYang670 Feb 3, 2022
9cb1146
solve conflict
HaoYang670 Feb 3, 2022
330dd8c
fix spelling mistake
HaoYang670 Feb 3, 2022
2b1cc01
rename to call_fn
HaoYang670 Feb 7, 2022
75539c7
Merge branch 'master' into issue1718_cleaner_API_to_create_scalar_fun…
HaoYang670 Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions datafusion/src/logical_plan/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2235,6 +2235,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_fn("sin", vec![col("x")]).unwrap().lt(lit(0.2));
/// ```
pub fn call_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),
}
Comment on lines +2246 to +2249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to write this more idiomatically is

Suggested change
match name.as_ref().parse::<functions::BuiltinScalarFunction>() {
Ok(fun) => Ok(Expr::ScalarFunction { fun, args }),
Err(e) => Err(e),
}
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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if it makes sense to make it a macro rather than a function call so that nonexisteng built-in functions will be caught during compile time not runtime

Copy link
Member

@houqp houqp Feb 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, call_builtin_scalar_fn!(ToTimestamp, vec![lit("2020-09-08T12:00:00+00:00")]) is as readable.

Copy link
Contributor Author

@HaoYang670 HaoYang670 Feb 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jimexist !

And alamb 's opinion of the trade-off between macro and function call is here:
#1718 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed to implement both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't think so. If we want a macro we can always add that as a follow on PR.

Thanks @HaoYang670 !

}

#[cfg(test)]
mod tests {
use super::super::{col, lit, when};
Expand Down
2 changes: 1 addition & 1 deletion datafusion/src/logical_plan/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ 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,
avg, binary_expr, bit_length, btrim, call_fn, 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,
Expand Down
45 changes: 11 additions & 34 deletions datafusion/src/optimizer/simplify_expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,8 +735,8 @@ mod tests {
use super::*;
use crate::assert_contains;
use crate::logical_plan::{
and, binary_expr, col, create_udf, lit, lit_timestamp_nano, DFField, Expr,
LogicalPlanBuilder,
and, binary_expr, call_fn, col, create_udf, lit, lit_timestamp_nano, DFField,
Expr, LogicalPlanBuilder,
};
use crate::physical_plan::functions::{make_scalar_function, BuiltinScalarFunction};
use crate::physical_plan::udf::ScalarUDF;
Expand Down Expand Up @@ -1010,46 +1010,29 @@ 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 = call_fn("concat", vec![lit("foo"), lit("bar")]).unwrap();
test_evaluate(expr, lit("foobar"));

// ensure arguments are also constant folded
// concat("foo", concat("bar", "baz")) --> "foobarbaz"
let concat1 = Expr::ScalarFunction {
args: vec![lit("bar"), lit("baz")],
fun: BuiltinScalarFunction::Concat,
};
let expr = Expr::ScalarFunction {
args: vec![lit("foo"), concat1],
fun: BuiltinScalarFunction::Concat,
};
let concat1 = call_fn("concat", vec![lit("bar"), lit("baz")]).unwrap();
let expr = call_fn("concat", vec![lit("foo"), concat1]).unwrap();
test_evaluate(expr, lit("foobarbaz"));

// Check non string arguments
// to_timestamp("2020-09-08T12:00:00+00:00") --> timestamp(1599566400000000000i64)
let expr = Expr::ScalarFunction {
args: vec![lit("2020-09-08T12:00:00+00:00")],
fun: BuiltinScalarFunction::ToTimestamp,
};
let expr =
call_fn("to_timestamp", vec![lit("2020-09-08T12:00:00+00:00")]).unwrap();
test_evaluate(expr, lit_timestamp_nano(1599566400000000000i64));

// check that non foldable arguments are folded
// to_timestamp(a) --> to_timestamp(a) [no rewrite possible]
let expr = Expr::ScalarFunction {
args: vec![col("a")],
fun: BuiltinScalarFunction::ToTimestamp,
};
let expr = call_fn("to_timestamp", vec![col("a")]).unwrap();
test_evaluate(expr.clone(), expr);

// check that non foldable arguments are folded
// to_timestamp(a) --> to_timestamp(a) [no rewrite possible]
let expr = Expr::ScalarFunction {
args: vec![col("a")],
fun: BuiltinScalarFunction::ToTimestamp,
};
let expr = call_fn("to_timestamp", vec![col("a")]).unwrap();
test_evaluate(expr.clone(), expr);

// volatile / stable functions should not be evaluated
Expand Down Expand Up @@ -1090,10 +1073,7 @@ mod tests {
}

fn now_expr() -> Expr {
Expr::ScalarFunction {
args: vec![],
fun: BuiltinScalarFunction::Now,
}
call_fn("now", vec![]).unwrap()
}

fn cast_to_int64_expr(expr: Expr) -> Expr {
Expand All @@ -1104,10 +1084,7 @@ mod tests {
}

fn to_timestamp_expr(arg: impl Into<String>) -> Expr {
Expr::ScalarFunction {
args: vec![lit(arg.into())],
fun: BuiltinScalarFunction::ToTimestamp,
}
call_fn("to_timestamp", vec![lit(arg.into())]).unwrap()
}

#[test]
Expand Down