-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Migrate arrow_cast to a UDF
#9610
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
Migrate arrow_cast to a UDF
#9610
Conversation
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
arrow_cast to a UDF
| "| 2020-09-04 |", | ||
| "+------------+", | ||
| "+-----------------------------------+", | ||
| "| arrow_cast(t.values,Utf8(\"Utf8\")) |", |
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.
These differences are due to the fact that arrow_cast is just a normal function now rather than a special case in the parser. Thus the naming reflects normal function naming
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.
Interesting I got stuck implementing the simpliy function because I thought it should convert arrow_cast(t.values,Utf8(\"Utf8\")) to t.values and other similar cases as well.
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.
Yeah -- this is pretty tricky. arrow_cast was quite special in the parser, so now that it is handled like a normal function it has the same (somewhat strange) function effect of column naming
| info: &dyn SimplifyInfo, | ||
| ) -> Result<ExprSimplifyResult> { | ||
| // convert this into a real cast | ||
| let target_type = data_type_from_args(&args)?; |
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.
This simplify logic mirrors the previous behavior in that arrow_cast is replaced with a normal cast
jayzhan211
left a comment
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.
LGTM!
| } | ||
|
|
||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| parse_data_type(&arg_types[1].to_string()) |
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.
If return_type_from_exprs exists, we don't need return_type. Is it better to return err or panic here?
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.
This is an excellent call -- changed to internal_err in 0c7b7be
|
Thanks again @brayanjuls and @jayzhan211 |
Which issue does this PR close?
Closes #9143
Closes #9287
Closes #9298
Rationale for this change
arrow_cast function migration.
What changes are included in this PR?
This PR is based on #9298 from @brayanjuls, updated to use the new simplify API.
Are these changes tested?
Yes, by existing tests
Are there any user-facing changes?