Add plugable handler for `CREATE FUNCTION` #9333

milenkovicm · 2024-02-24T13:39:58Z

Which issue does this PR close?

Closes #9332.

Rationale for this change

explained in #9332

What changes are included in this PR?

this is minimum viable product, with some open question which i hope to resolve in the process of PR review (note TODOs)

Are these changes tested?

There is a test in place but would add more if community aggress to accept the change.
Usage example can be found at https://github.com/milenkovicm/torchfusion

Are there any user-facing changes?

There are using facing changes, but they are backward compatible. Would update docs if community aggress to accept the change.

A problem I see is from perspective of user experience, as generic parser does not support CREATE FUNCTION statement, user should change parser configuration to PostgreSQL. Maybe it would make sense after this change is merged adding CREATE FUNCTION to generic flavour

milenkovicm · 2024-02-24T18:09:49Z

Drop function will be added as well

alamb · 2024-02-28T12:21:03Z

Hi @milenkovicm -- I plan to review this PR shortly

alamb

Thank you for this PR @milenkovicm -- I think the idea and the PR is really neat and the code was easy to follow. ❤️

I left some API suggestions, and comments in regards to your questions. Let me know what you think.

datafusion/core/src/execution/context/mod.rs

datafusion/expr/src/logical_plan/statement.rs

datafusion/core/src/execution/context/mod.rs

datafusion/core/tests/sql/sql_api.rs

alamb · 2024-02-28T21:56:17Z

datafusion/core/tests/sql/sql_api.rs

@@ -39,6 +52,99 @@ async fn unsupported_ddl_returns_error() {
    ctx.sql_with_options(sql, options).await.unwrap();
 }

+struct MockFunctionFactory;


This is very cool -- I think it would be great (as a follow on PR) if we turned this into an example (with docs, etc like the others in https://github.com/apache/arrow-datafusion/tree/main/datafusion-examples/examples)

Right, this functionality looks important and definitely needs to be covered with docs, examples in following PR, looking forward for it

datafusion/core/tests/sql/sql_api.rs

milenkovicm · 2024-02-29T09:15:54Z

Comments totally make sense thanks @alamb,
it would involve a bit of creativity to nail the trait interface, let's try. I'm a bit tied up over next few days will follow up over the weekend probably

milenkovicm · 2024-02-29T10:40:57Z

Items to follow up (new issues or discussion)

Create an example and documents? Add example for CREATE FUNCTION & FunctionFactory #9448
Support postgres style CREATE FUNCTION in GenericDialect sqlparser-rs/sqlparser-rs#1159?
Add unregister function in FunctionRegistry?
should we keep FunctionFactory.remove

datafusion/core/src/execution/context/mod.rs

alamb

Looking very 👨‍🍳 👌 nice @milenkovicm

datafusion/core/src/execution/context/mod.rs

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

datafusion/core/src/execution/context/mod.rs

alamb

Thank you @milenkovicm -- I think this PR is almost good to go from my perspective. I had a few minor comments, but nothing major.

I think we should have at least one other maintainer give it a look. Perhaps @comphead or @universalmind303 ?

One interesting implication of this feature is that once we migrate all functions to be "udfs" (e.g. #8045) it means we can now drop functions that weren't explicitly made with CREATE FUNCTION

For example, on this branch I can drop the abs function:

DataFusion CLI v36.0.0
❯ DROP FUNCTION abs;
0 rows in set. Query took 0.009 seconds.

Although I do get an appropriate error when I try to drop a function that is still a BuiltInScalarFunction like add:

❯ DROP FUNCTION add;
Execution error: Function does not exist

People could

datafusion/core/src/execution/context/mod.rs

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

datafusion/expr/src/logical_plan/ddl.rs

datafusion/sql/src/statement.rs

alamb

Thanks again @milenkovicm -- the only thing left from my perspective is to "error on multi-part identifiers" which I think we could do as a follow on PR if needed.

However, I do think we should leave this open for a few more days to let other people have a chance to comment on it before we merge.

Thanks again. This is a really neat feature that I think will let people build all sorts of cool things using DataFusion 🙏

datafusion/core/src/execution/context/mod.rs

datafusion/expr/src/logical_plan/ddl.rs

milenkovicm · 2024-03-02T12:36:56Z

Thanks again @milenkovicm -- the only thing left from my perspective is to "error on multi-part identifiers" which I think we could do as a follow on PR if needed.

However, I do think we should leave this open for a few more days to let other people have a chance to comment on it before we merge.

Thanks again. This is a really neat feature that I think will let people build all sorts of cool things using DataFusion 🙏

@alamb multi-part identifiers fixed as well, please have a look.

also, can you please let me know once new version of sqlparser lands, will remove dialect configuration from test.

thanks a lot for all your help

milenkovicm · 2024-03-02T12:40:30Z

and please squash commit on merge, thanks

alamb · 2024-03-02T15:42:01Z

and please squash commit on merge, thanks

Yes, this will be done (as on all commits)

alamb

Thanks again @milenkovicm

datafusion/core/tests/sql/sql_api.rs

datafusion/core/src/execution/context/mod.rs

comphead

Thanks @milenkovicm
I would say it is nice to get also tests what will happen if the function is invalid/not compilable or something

another question if function can call another function?

milenkovicm · 2024-03-02T18:36:05Z

Thanks @milenkovicm I would say it is nice to get also tests what will happen if the function is invalid/not compilable or something

another question if function can call another function?

Thanks for your comments @comphead ,
can you please elaborate your comment, thanks

comphead · 2024-03-02T18:46:59Z

Thanks @milenkovicm I would say it is nice to get also tests what will happen if the function is invalid/not compilable or something
another question if function can call another function?

Thanks for your comments @comphead , can you please elaborate your comment, thanks

Its great to have a test with invalid function, the function which is not parseable or compilable.

And the next question if the user created functions f1 and f2, can f1 be called inside f2?

... `DROP FUNCTION` will look for function name in all available registries (udf, udaf, udwf). `remove` may be necessary if UDaF and UDwF do not get `simplify` method from apache#9304.

FunctionDefinition already exists, DefinitionStatement makes more sense.

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

…ns.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

... as `create function` gets support in latest sqlparser.

alamb · 2024-03-05T21:56:24Z

I merged up from main to pick up a fix for the CI (I hope 🤞 )

milenkovicm · 2024-03-05T21:58:11Z

Thanks @alamb

alamb · 2024-03-05T22:19:20Z

Merged up from main again to resolve conflicts

alamb · 2024-03-05T23:05:26Z

Thanks again @milenkovicm -- this is a pretty epic feature

github-actions bot added sql logical-expr Logical plan and expressions core Core datafusion crate labels Feb 24, 2024

milenkovicm changed the title ~~Add plugable function factory~~ Add plugable handler for CREATE FUNCTION Feb 24, 2024

alamb mentioned this pull request Feb 28, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 26, 2024 #9345

Closed

9 tasks

alamb reviewed Feb 28, 2024

View reviewed changes

milenkovicm force-pushed the create_function_factory branch from b9bf00e to acdb4b5 Compare February 29, 2024 20:15

alamb mentioned this pull request Feb 29, 2024

Support postgres style CREATE FUNCTION in GenericDialect sqlparser-rs/sqlparser-rs#1159

Merged

alamb reviewed Feb 29, 2024

View reviewed changes

datafusion/core/src/execution/context/mod.rs Show resolved Hide resolved

datafusion/core/src/execution/context/mod.rs Outdated Show resolved Hide resolved

alamb reviewed Feb 29, 2024

View reviewed changes

datafusion/core/src/execution/context/mod.rs Outdated Show resolved Hide resolved

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs Outdated Show resolved Hide resolved

milenkovicm force-pushed the create_function_factory branch 2 times, most recently from 25206aa to 45c7edb Compare February 29, 2024 21:41

milenkovicm mentioned this pull request Mar 2, 2024

Add a ScalarUDFImpl::simplfy() API, move SimplifyInfo et al to datafusion_expr #9304

Merged

milenkovicm commented Mar 2, 2024

View reviewed changes

datafusion/core/src/execution/context/mod.rs Show resolved Hide resolved

milenkovicm force-pushed the create_function_factory branch from 0b93b6a to a1d75a5 Compare March 2, 2024 07:48

alamb reviewed Mar 2, 2024

View reviewed changes

alamb approved these changes Mar 2, 2024

View reviewed changes

datafusion/core/src/execution/context/mod.rs Show resolved Hide resolved

datafusion/core/src/execution/context/mod.rs Outdated Show resolved Hide resolved

datafusion/expr/src/logical_plan/ddl.rs Show resolved Hide resolved

datafusion/expr/src/logical_plan/ddl.rs Show resolved Hide resolved

alamb approved these changes Mar 2, 2024

View reviewed changes

comphead reviewed Mar 2, 2024

View reviewed changes

datafusion/core/tests/sql/sql_api.rs Outdated Show resolved Hide resolved

comphead reviewed Mar 2, 2024

View reviewed changes

datafusion/core/src/execution/context/mod.rs Outdated Show resolved Hide resolved

comphead reviewed Mar 2, 2024

View reviewed changes

milenkovicm and others added 16 commits March 5, 2024 09:07

Address PR comments (factory interface)

a8d54b6

fix test after rebase

262f0f6

remove's gone from the trait ...

fe63c31

... `DROP FUNCTION` will look for function name in all available registries (udf, udaf, udwf). `remove` may be necessary if UDaF and UDwF do not get `simplify` method from apache#9304.

Rename FunctionDefinition and export it ...

b975df7

FunctionDefinition already exists, DefinitionStatement makes more sense.

Update datafusion/expr/src/logical_plan/ddl.rs

9d1d715

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/core/src/execution/context/mod.rs

7e7d896

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/core/tests/user_defined/user_defined_scalar_functio…

0430c11

…ns.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/expr/src/logical_plan/ddl.rs

210b194

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

resolve part of follow up comments

1d5d739

Qualified functions are not supported anymore

84b0fbd

update docs and todos

b642570

fix clippy

b8f8991

address additional comments

5a9ad09

Add sqllogicteset for CREATE/DROP function

58479e3

Add coverage for DROP FUNCTION IF EXISTS

83acc8c

fix multiline error

383602c

milenkovicm force-pushed the create_function_factory branch from f17a50f to 383602c Compare March 5, 2024 09:09

milenkovicm and others added 2 commits March 5, 2024 09:19

revert dialect back to generic in test ...

00b8058

... as `create function` gets support in latest sqlparser.

Merge remote-tracking branch 'apache/main' into create_function_factory

f27d800

Merge remote-tracking branch 'apache/main' into create_function_factory

d7e37ed

fmt

8a0f42f

alamb merged commit ea01e56 into apache:main Mar 5, 2024
23 checks passed

milenkovicm mentioned this pull request Mar 6, 2024

Add example for FunctionFactory #9482

Merged

This was referenced Mar 11, 2024

DataFusion weekly project plan (Andrew Lamb) - March 11, 2024 #9555

Closed

Blog post with DataFusion Jan - June 2024 #9602

Open

milenkovicm deleted the create_function_factory branch April 6, 2024 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugable handler for `CREATE FUNCTION` #9333

Add plugable handler for `CREATE FUNCTION` #9333

milenkovicm commented Feb 24, 2024 •

edited

Loading

milenkovicm commented Feb 24, 2024

alamb commented Feb 28, 2024

alamb left a comment

alamb Feb 28, 2024

comphead Mar 2, 2024

milenkovicm commented Feb 29, 2024

milenkovicm commented Feb 29, 2024 •

edited

Loading

alamb left a comment

alamb left a comment

alamb left a comment

milenkovicm commented Mar 2, 2024

milenkovicm commented Mar 2, 2024

alamb commented Mar 2, 2024

alamb left a comment

comphead left a comment

milenkovicm commented Mar 2, 2024

comphead commented Mar 2, 2024

alamb commented Mar 5, 2024 •

edited

Loading

milenkovicm commented Mar 5, 2024

alamb commented Mar 5, 2024

alamb commented Mar 5, 2024

Add plugable handler for CREATE FUNCTION #9333

Add plugable handler for CREATE FUNCTION #9333

Conversation

milenkovicm commented Feb 24, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

milenkovicm commented Feb 24, 2024

alamb commented Feb 28, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 28, 2024

Choose a reason for hiding this comment

comphead Mar 2, 2024

Choose a reason for hiding this comment

milenkovicm commented Feb 29, 2024

milenkovicm commented Feb 29, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

milenkovicm commented Mar 2, 2024

milenkovicm commented Mar 2, 2024

alamb commented Mar 2, 2024

alamb left a comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

milenkovicm commented Mar 2, 2024

comphead commented Mar 2, 2024

alamb commented Mar 5, 2024 • edited Loading

milenkovicm commented Mar 5, 2024

alamb commented Mar 5, 2024

alamb commented Mar 5, 2024

Add plugable handler for `CREATE FUNCTION` #9333

Add plugable handler for `CREATE FUNCTION` #9333

milenkovicm commented Feb 24, 2024 •

edited

Loading

milenkovicm commented Feb 29, 2024 •

edited

Loading

alamb commented Mar 5, 2024 •

edited

Loading