alamb commented on code in PR #9226:
URL: https://github.com/apache/arrow-datafusion/pull/9226#discussion_r1491090843
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1204,6 +1204,18 @@ impl FunctionRegistry for SessionContext {
fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
self.state.read().udwf(name)
}
+ fn register_udf(&mut self, udf: Arc<ScalarUDF>) ->
Result<Option<Arc<ScalarUDF>>> {
Review Comment:
👍
##########
datafusion/proto/src/bytes/registry.rs:
##########
@@ -42,4 +42,16 @@ impl FunctionRegistry for NoRegistry {
fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>> {
plan_err!("No function registry provided to deserialize, so can not
deserialize User Defined Window Function '{name}'")
}
+ fn register_udaf(
+ &mut self,
+ _udaf: Arc<AggregateUDF>,
+ ) -> Result<Option<Arc<AggregateUDF>>> {
+ unimplemented!()
Review Comment:
Likewise can you please change this to `plan_err` to mirror the function
calls above?
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -140,6 +140,24 @@ impl Serializeable for Expr {
Arc::new(|| unimplemented!()),
)))
}
+ fn register_udaf(
+ &mut self,
+ _udaf: Arc<AggregateUDF>,
+ ) -> Result<Option<Arc<AggregateUDF>>> {
+ unimplemented!()
Review Comment:
While `unimplemented` is consistent with the code above (that also will
panic) I agree it would be better to return an error here
perhaps a internal error would be most appropriate as this code "should
never be hit"
```suggestion
internal_err!("register_udaf called in Placeholder Registry")
```
Likewise for the locations below
##########
datafusion/execution/src/registry.rs:
##########
@@ -44,10 +44,7 @@ pub trait FunctionRegistry {
///
/// Returns an error (the default) if the function can not be registered,
/// for example if the registry is read only.
- fn register_udf(&mut self, _udf: Arc<ScalarUDF>) ->
Result<Option<Arc<ScalarUDF>>> {
- not_impl_err!("Registering ScalarUDF")
- }
-
+ fn register_udf(&mut self, _udf: Arc<ScalarUDF>) ->
Result<Option<Arc<ScalarUDF>>>;
Review Comment:
Why was the default implementation removed?
If we remove the default implementation, it means that this change will no
longer be backwards compatible because any code that implements
FunctionRegistry will now have to implement all the methods
Thus, I think it would be better to leave the default (erroring)
implementation and be backwards compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
