feat: function name hints for UDFs#9407
Conversation
github-actions
bot
added
sql
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @SteveLauC -- this is looking very cool
datafusion/sql/src/planner.rs
Outdated
| /// 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
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think this PR also needs tests -- however, I think when you merge this branch up from main, you should have to update at least one test in a sqllogictest file, so you probably don't have to write anything new
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
alamb
left a comment
alamb
left a comment
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)
|
|
||
| # Scalar function | ||
| statement error Invalid function 'to_timestamps_second' | ||
| statement error Did you mean 'to_timestamp_seconds'? |
|
Thanks again @SteveLauC |
3 participants
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?