Simplify AsyncScalarUdfImpl so it extends ScalarUdfImpl #16523
Conversation
github-actions
bot
added
logical-expr
| async fn invoke_async_with_args( | ||
| &self, | ||
| args: AsyncScalarFunctionArgs, | ||
| args: ScalarFunctionArgs, |
There was a problem hiding this comment.
It is interesting here that invoke_async_with_args has a copy of the config_options` 🤔 -- I think that is soemthing that @Omega359 has tried to get into normal scalar functions for a while
There was a problem hiding this comment.
It makes sense to me. We allow the custom config. Allowing access to the config option can make UDF flexible.
github-actions
bot
added
the
documentation
| @@ -35,34 +35,7 @@ use std::sync::Arc; | |||
| /// | |||
| /// The name is chosen to mirror ScalarUDFImpl | |||
| #[async_trait] | |||
| pub trait AsyncScalarUDFImpl: Debug + Send + Sync { | |||
| /// the function cast as any | |||
| fn as_any(&self) -> &dyn Any; | |||
There was a problem hiding this comment.
The point of this PR is to remove this duplication from AsyncScalarUDFImpl and instead use ScalarUDFImpl instead
|
I feel like there may be some more duplication we can remove as part of the PhysicalExpr layer too but I don't have time to pursue that at the moment. Maybe @goldmedal can give it a look |
| async fn invoke_async_with_args( | ||
| &self, | ||
| args: AsyncScalarFunctionArgs, | ||
| args: ScalarFunctionArgs, |
There was a problem hiding this comment.
It makes sense to me. We allow the custom config. Allowing access to the config option can make UDF flexible.
|
Thanks again for the review @goldmedal |
* Simplify AsyncScalarUdfImpl so it extends ScalarUdfImpl * Update one example * Update one example * prettier
Which issue does this PR close?
AsyncScalarUDFImplAPI to matchScalarUDFImplAPI #16522Rationale for this change
Following @berkaysynnada 's suggestion in #14837 having parallel implementations of functions allows sync and async UDFs to drift apart.
Let's try and make the differences as small as possible
What changes are included in this PR?
ScalarUdfImplAre these changes tested?
Are there any user-facing changes?
API is changed, but it has not been released yet so this is not an external API change