-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add example for FunctionFactory
#9482
Conversation
@alamb when you get chance 😀 thanks ! Do we need more documentation for it ? Another follow up functionality can be to expose functions in system catalog and/or using |
FunctionFactory
f6adc6a
to
cade25d
Compare
Thanks @milenkovicm -- I will review this over the next day or two. I am sorry I am behind on reviews and catching up now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @milenkovicm -- this feature is SOO cool.
Can you just imagine a library of FunctionHandlers
so people could pick from a list of types of functions they wanted to support:
- I want support for pytorch models"
- I want to support SQL macros
- Oh, and also I want to support WASM
A few imports, a cargo build
command and off they go. 🤩
I pushed some word smithing comments up to your branch, as well as some comments that are more in the "explain it line by line" style -- let me know what you think
We can revert them if you prefer. What you had was great and I just got excited as I had it checked out locally
@@ -826,11 +914,22 @@ async fn create_scalar_function_from_sql_statement() -> Result<()> { | |||
.await | |||
.is_err()); | |||
|
|||
ctx.sql("select better_add(2.0, 2.0)").await?.show().await?; | |||
let result = ctx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
#[tokio::main] | ||
async fn main() -> Result<()> { | ||
let runtime_config = RuntimeConfig::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example showed how awkward it was to create the appropriate SessionState
, so I added a convenience methods on SessionContext
to make it easier
@@ -54,6 +54,7 @@ cargo run --example csv_sql | |||
- [`deserialize_to_struct.rs`](examples/deserialize_to_struct.rs): Convert query results into rust structs using serde | |||
- [`expr_api.rs`](examples/expr_api.rs): Create, execute, simplify and analyze `Expr`s | |||
- [`flight_sql_server.rs`](examples/flight/flight_sql_server.rs): Run DataFusion as a standalone process and execute SQL queries from JDBC clients | |||
- [`function_factory.rs`](examples/function_factory.rs): Register `CREATE FUNCTION` handler to implement SQL macros |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the example
|
||
ctx.sql(sql).await?.show().await?; | ||
|
||
// Invoke f2, and we expect to see 1 + (1 + 2) = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed some of the other things that didn't really help teach people how to use this feature any more (they were more like ensuring all features got covered) to keep it shorter
internal_err!("This function should not get invoked!") | ||
} | ||
|
||
/// The simplify function is called to simply a call such as `f2(2)`. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add some comments inline that explained what the functions were doing with more words, but hopefully for someone who was seeing if this worked for their usecase
Thanks a lot @alamb, your additions totally make sense. Also, inspired by simplification you've done in let mut session_config = SessionConfig::new().with_information_schema(true);
session_config
.options_mut()
.extensions
// register torch factory configuration
.insert(TorchConfig::default()); it would be much better if config could be registered when |
Excellent idea -- I have filed |
I filed a ticket to discuss this idea: #9529 |
Thanks again @milenkovicm |
Which issue does this PR close?
Closes #9448.
Rationale for this change
Explained in #9448. It covers example for functionality added in #9304 & #9333
What changes are included in this PR?
Example demonstrating functionality
Are these changes tested?
Yes
Are there any user-facing changes?
No