-
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
Migrate arrow_cast
to a UDF
#9610
Migrate arrow_cast
to a UDF
#9610
Conversation
Co-authored-by: Andrew Lamb <[email protected]>
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
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?