Minor: Move functionality into BuildInScalarFunction
#6612
Conversation
github-actions
bot
added
logical-expr
alamb
force-pushed
the
alamb/encapsulate_bulit_in_functions
branch
from
f11a589
to
5bfd5ef
Compare
| @@ -107,7 +105,7 @@ impl ExprSchemable for Expr { | |||
| .iter() | |||
| .map(|e| e.get_type(schema)) | |||
| .collect::<Result<Vec<_>>>()?; | |||
| function::return_type(fun, &data_types) | |||
| fun.return_type(&data_types) | |||
There was a problem hiding this comment.
this is a pretty good example of the point of this PR -- the functionality is now part of fun rather than a free function that takes &self as its first argument
There was a problem hiding this comment.
(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type
There was a problem hiding this comment.
(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type
🚀agree with it
| @@ -1,125 +0,0 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
This is recently added code from @2010YOUY01 (which was awesome) which I simply moved into BuiltInScalarFunction
| @@ -107,7 +105,7 @@ impl ExprSchemable for Expr { | |||
| .iter() | |||
| .map(|e| e.get_type(schema)) | |||
| .collect::<Result<Vec<_>>>()?; | |||
| function::return_type(fun, &data_types) | |||
| fun.return_type(&data_types) | |||
There was a problem hiding this comment.
(I hope to apply the same pattern to window_function::return_type and aggregate_function::return_type
🚀agree with it
I finally had a chance to whip this out: #6748 |
(Note this looks like a large change but it is actually quite small -- it is just moving code around)
Which issue does this PR close?
N/A
(though inspired by #5781 )
Rationale for this change
This has been a pet peeve of mine and I think it makes working with this code harder than needed as finding important functionality like "what is the signature of this function" harder because they aren't documented on https://docs.rs/datafusion-expr/26.0.0/datafusion_expr/enum.BuiltinScalarFunction.html
What changes are included in this PR?
signature,return_typeandgenerate_signature_error_msgto functions onBuiltinScalarFunctionAre these changes tested?
existing coverage
Are there any user-facing changes?
Hopefully easier to navigate code