-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[task#9512] move from_unixtime, now, current_date, current_time to da… #9513
Conversation
…tafusion_functions Signed-off-by: tangruilin <tang.ruilin@foxmail.com>
now, current_date, current_time need to get query_execution_start_time. But Now I see there is no |
I Think |
// TODO: add execution_props in ScalarUDF, so that we can get some params like query_execution_start_time
/// Create a physical expression of the UDF.
///
/// Arguments:
pub fn create_physical_expr(
fun: &ScalarUDF,
input_phy_exprs: &[Arc<dyn PhysicalExpr>],
return_type: DataType,
) -> Result<Arc<dyn PhysicalExpr>> {
Ok(Arc::new(ScalarFunctionExpr::new(
fun.name(),
fun.fun(),
input_phy_exprs.to_vec(),
return_type,
fun.monotonicity()?,
fun.signature().type_signature.supports_zero_argument(),
)))
} I think I can add execution_props in ScalarUDF, so that we can get some params like query_execution_start_time in next PR how do you think of it? |
I think we can use ScalarUdfImpl::simplify in this case: https://github.com/apache/arrow-datafusion/blob/b1d8082c6271e49c1de6e20002b853ca1503ffde/datafusion/expr/src/udf.rs#L373-L380 Specifically, the Thus for a function like
Does that make sense? |
(thank you for working on this, by the way @Tangruilin 🙏 ) |
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
Dup of #9537 |
Maybe you need to close this |
…tafusion_functions
Which issue does this PR close?
Closes part of #9512.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?