-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement stateless udf registration #97
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
base: main
Are you sure you want to change the base?
Conversation
8ea54a1 to
ad19165
Compare
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.
Before we merge this, can you try if that API works for monolith?
9335575 to
2459318
Compare
2959efe to
b36562c
Compare
b36562c to
6db6b4d
Compare
| CREATE FUNCTION add_one() | ||
| LANGUAGE python | ||
| AS ' | ||
| def add_one(x: int) -> int: | ||
| return x + 1 | ||
| '; | ||
| CREATE FUNCTION multiply_two() | ||
| LANGUAGE python | ||
| AS ' | ||
| def multiply_two(x: int) -> int: | ||
| return x * 2 | ||
| '; | ||
| SELECT add_one(1), multiply_two(3); |
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.
That's a good test, but ON TOP of that, could we ALSO test that this works:
CREATE FUNCTION add_one()
LANGUAGE python
AS '
def add_one(x: int) -> int:
return x + 1
def multiply_two(x: int) -> int:
return x * 2
';
SELECT add_one(1), multiply_two(3);(from skimming through the code I think it should)
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.
And related, could we test the behavior of this nonsense here:
CREATE FUNCTION add_one()
LANGUAGE python
AS '';
SELECT 1;(i.e. the python code block contains NO functions)
host/src/udf_query.rs
Outdated
| /// Invoke the query, returning a result | ||
| pub async fn invoke(&mut self, udf_query: UdfQuery) -> DataFusionResult<Vec<Vec<String>>> { |
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 think for the integration it would be easier if you return the physical plan here and let the caller decide if they wanna call collect or whatever.
Closes #25
This creates a couple types to stateless-ly handle UDF registration and invocation within a query. I've opted for a draft PR as its possible things may change significantly and to leverage CI for testing.