Minor: Cleanup BuiltinScalarFunction's phys-expr creation #8114
Conversation
|
I wonder if this modification will lead to performance degradation. Each time we evaluate the function expression, we have to pick an implementation based on the input type. |
That's a good point, but I think for this case, the extra overhead is one or two function calls per batch(typically 8k rows) select to_timestamp(l_shipdate), to_timestamp_seconds(l_commitdate), to_timestamp_millis(l_receiptdate), to_timestamp_micros(l_shipdate), to_timestamp_nanos(l_shipdate), from_unixtime(l_orderkey), abs(l_orderkey) from lineitem; |
| @@ -17,6 +17,8 @@ | |||
|
|
|||
| //! DateTime expressions | |||
|
|
|||
| use crate::datetime_expressions; | |||
There was a problem hiding this comment.
| use crate::datetime_expressions; |
It seems that this use may not be required 🤔.
There was a problem hiding this comment.
🤔 but clippy did not flag it
There was a problem hiding this comment.
Thank you @2010YOUY01 -- looks really good
Also thank you @jonahgao for your guidance and review
| @@ -17,6 +17,8 @@ | |||
|
|
|||
| //! DateTime expressions | |||
|
|
|||
| use crate::datetime_expressions; | |||
There was a problem hiding this comment.
🤔 but clippy did not flag it
| @@ -927,11 +811,19 @@ pub fn create_physical_fun( | |||
| }), | |||
| BuiltinScalarFunction::Upper => Arc::new(string_expressions::upper), | |||
| BuiltinScalarFunction::Uuid => Arc::new(string_expressions::uuid), | |||
| _ => { | |||
Which issue does this PR close?
Cleanup for #8045
Rationale for this change
In current implementation, some function tries to generate input type specific physical expressions in
https://github.com/apache/arrow-datafusion/blob/308c35404939ed39e4e398054d6fc78c89817109/datafusion/physical-expr/src/functions.rs#L75-L204
However, this can be done inside function implementation like most other functions did, this PR would like to unify the implementation approach
What changes are included in this PR?
Move input-type-specific
BuiltinScalarFunctionphysical expression creation to inside function implementationAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
No