feat: function name hints for UDFs #9407
Conversation
github-actions
bot
added
sql
datafusion/sql/src/planner.rs
Outdated
| @@ -85,6 +85,10 @@ pub trait ContextProvider { | |||
|
|
|||
| /// Get configuration options | |||
| fn options(&self) -> &ConfigOptions; | |||
|
|
|||
| fn udfs(&self) -> HashMap<String, Arc<ScalarUDF>>; | |||
There was a problem hiding this comment.
This sort of exposes details of the implementation (e.g. a HashMap)
What about potentially just returning the names following the model of CatalogProvider or SchemaProvider: https://docs.rs/datafusion/latest/datafusion/catalog/schema/trait.SchemaProvider.html#tymethod.table_names
Something like
| fn udfs(&self) -> HashMap<String, Arc<ScalarUDF>>; | |
| /// returns all udf names | |
| fn udf_names(&self) -> Vec<&str> |
(or maybe Strings)
There was a problem hiding this comment.
Nice! I love consistency! Will do it.
There was a problem hiding this comment.
Do you want this function to return a Vec<&str> or Vec<String>, the suggested change uses Vec<&str>, but the table_names() function in the referenced link uses Vec<String>
SteveLauC
force-pushed
the
feat/fn_name_hint_for_scalar_udf
branch
from
0675113
to
40cd987
Compare
datafusion/sql/src/expr/function.rs
Outdated
| // All aggregate functions and builtin window functions | ||
| AggregateFunction::iter() | ||
| .map(|func| func.to_string()) | ||
| .chain(BuiltInWindowFunction::iter().map(|func| func.to_string())) |
There was a problem hiding this comment.
Would we want to add the ctx.udafs() and ctx.udwfs() here as well?
|
Hi @SteveLauC -- how is this PR going? Anything we can help with? |
SteveLauC
force-pushed
the
feat/fn_name_hint_for_scalar_udf
branch
from
40cd987
to
3a3c173
Compare
There was a problem hiding this comment.
Thank you @SteveLauC -- I tried this out and it worked great
❯ select abs2(1);
Error during planning: Invalid function 'abs2'.
Did you mean 'abs'?
❯
I also merged up from main and added a test for the functionality (undid the change fro #9388)
| @@ -483,7 +483,7 @@ statement error Did you mean 'arrow_typeof'? | |||
| SELECT arrowtypeof(v1) from test; | |||
|
|
|||
| # Scalar function | |||
| statement error Invalid function 'to_timestamps_second' | |||
| statement error Did you mean 'to_timestamp_seconds'? | |||
|
Thanks again @SteveLauC |
Which issue does this PR close?
Closes #9392.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?