-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement UDF Plan #11263
Implement UDF Plan #11263
Conversation
0e6e1ce
to
90b9c05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I don't think this is a good abstration, because extract and position are actually quite different concept. For the point of view to user-defined planner, they could have their own implementation that may not be UDF, so it does not make sense to place them to plan_udf for me.
I would prefer to have plan_extract
and plan_position
separately.
Btw, I suggest using plan_udaf
in #11229, as we are customizing the query of aggregate function. Similarly, I think here should be the correct place for plan_udf
@xinlifoobar What do you think?
planner_context: &mut PlannerContext, | ||
) -> Result<Expr> { | ||
let sql_not_moved = sql.clone(); | ||
let mut extracted_args = match sql { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems not to be extract specific anymore, we could name it args
Here is a PR for planning position: #11243 It isn't clear to me what we should do with this PR now (should we close it?) |
I agree that even though the functions are somewhat repetitively named, it is better to be explicit in the function names rather than being implicit (via function name) as the API proposed in this PR would be Sorry I didn't see the comments on #11207 (comment) on #11207 earlier |
Which issue does this PR close?
Closes #11242 and part of #11207
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?