Refactor(optimizer): Simplify Stable ScalarUDFs using physical execution#19675
Refactor(optimizer): Simplify Stable ScalarUDFs using physical execution#19675xonx4l wants to merge 1 commit intoapache:mainfrom
Conversation
github-actions
Bot
added
optimizer
| ScalarValue::TimestampNanosecond(now_ts, self.timezone.clone()), | ||
| None, | ||
| fn invoke_with_args(&self, _args: ScalarFunctionArgs) -> Result<ColumnarValue> { | ||
| let now = chrono::Utc::now(); |
There was a problem hiding this comment.
I think the solution is a bit more involved than this; for one it makes testing this quite difficult as we don't seem to have an easy way to control this time. I think we need to be able to leverage properties during execution (e.g. how we previously got the time via execution_props() in simplify) instead of having the function call the raw time function by itself 🤔
There was a problem hiding this comment.
Oh yeah that's makes sense . I think modifying the helper to try udf.simplify() will be the right approach to go forward what you think if that make's sense ? In order to follow I will first restore the custom simplify implementation.
There was a problem hiding this comment.
Sorry I don't quite follow what you mean here
There was a problem hiding this comment.
I mean I will revert the changes to now.rs and keep its custom simplify logic and will modify the helper to try the UDF's simplify method first. where If it returns a result (like for now()) we use it and If not we fall back to the new execution logic.
There was a problem hiding this comment.
I think the original issue describes something similar toward that? But it would need wider changes to accompany such changes anyway
There was a problem hiding this comment.
I understand now can you help me outlining what needs to be done or pointing me in the right direction? Thanks!
There was a problem hiding this comment.
Apologies, I don't have the capacity to dive into this issue at the moment; perhaps you can try comment on the original issue to try get clarity from others that might have their eye on this issue to help determine a path forward
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
2 participants
Which issue does this PR close?
invoke()instead ofsimplify()#19470.Rationale for this change
This PR refactors expr_simplifier.rs to simplify Stable and Immutable ScalarUDFs by leveraging create_physical_expr and evaluate.
What changes are included in this PR?
-> modified - expr_simplifier.rs
-> modified - now.rs
Are these changes tested?
Yes
Are there any user-facing changes?
No