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.

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

Cleaner API to create Expr::ScalarFunction programatically #1718

Closed
alamb opened this issue Jan 31, 2022 · 5 comments · Fixed by #1734
Closed

Cleaner API to create Expr::ScalarFunction programatically #1718

alamb opened this issue Jan 31, 2022 · 5 comments · Fixed by #1734
Labels
datafusion Changes in the datafusion crate enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Jan 31, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
I am trying to programatically create Exprs that represent function calls, for example to_timetsamp(x, 4) or something

To do so today you have to do something like this (from simplify_expressions.rs) which is 🤮

        // to_timestamp("2020-09-08T12:00:00+00:00")
        let expr = Expr::ScalarFunction {
            args: vec![lit("2020-09-08T12:00:00+00:00")],
            fun: BuiltinScalarFunction::ToTimestamp,
        };

Describe the solution you'd like
I would like to write code like this

        // to_timestamp("2020-09-08T12:00:00+00:00") --> timestamp(1599566400000000000i64)
        let expr = call("to_timestamp", vec![lit("2020-09-08T12:00:00+00:00")])
          .unwrap();
```rust

Perhap with an api like this

```rust
/// Calls a named built in function
///
/// ```
/// use datafusion::logical_plan::*;
///
/// // create the expression sin(x) < 0.2
/// let expr = call("sin", vec![col("x")]).unwrap().lt(lit(0.2));
///
/// ```
pub fn call(name: impl AsRef<str>, args: Vec<Expr>) -> Result<Expr> {
...
}

Note you can lookup a str to scalar function by name with something like:

    let fun = name.as_ref().parse::<BuiltinScalarFunction>()?;

Additional context
There are a bunch of this nonsense in simplify_expression.rs which could be cleaned up

@alamb alamb added enhancement New feature or request good first issue Good for newcomers datafusion Changes in the datafusion crate labels Jan 31, 2022
@mkmik
Copy link
Contributor

mkmik commented Jan 31, 2022

What about something like:

call_builtin_scalar_fn!(ToTimestamp, lit("2020-09-08T12:00:00+00:00"))

@alamb
Copy link
Contributor Author

alamb commented Jan 31, 2022

call_builtin_scalar_fn!(ToTimestamp, lit("2020-09-08T12:00:00+00:00"))

That would have the upside that the compiler could check the name

It has the downside that the user has to know the mapping from the sql level name (e.g. concat to Concat) which maybe isn't a big deal.

Perhaps we could do both 🤔

@HaoYang670
Copy link
Contributor

I'd like to have a try if no one has been doing it😀

@mkmik
Copy link
Contributor

mkmik commented Feb 1, 2022

@HaoYang670 thanks

@HaoYang670
Copy link
Contributor

I find some duplicate tests in simplify_expression.rs.
Tracked in issue #1727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants