-
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
Support struct_expr generate struct in sql #2389
Conversation
datafusion/expr/src/function.rs
Outdated
BuiltinScalarFunction::Struct => { | ||
let fields = input_expr_types | ||
.iter() | ||
.map(|x| Field::new("item", x.clone(), true)) |
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 looks like it is generating a struct where all of the fields have the same name. I would expect that to cause an error.
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.
Fix, It should only need the DataType info, no need for fields name
.map(|(i, arg)| -> (Field, ArrayRef) { | ||
match arg.data_type() { | ||
DataType::Utf8 => ( | ||
Field::new(&*format!("f_{}", i), DataType::Utf8, true), |
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.
format!("f_{}", i)
is duplicated in each match. Could we create the column name once before the match statement?
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.
Sure! Thanks for your advice.
Field::new(field_name.as_str(), arg.data_type().clone(), true), | ||
arg.clone(), | ||
), | ||
data_type => unimplemented!("struct not support {} type", data_type), |
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.
We should return an Err
rather than 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.
LGTM once the error handling is updated to return Err
rather than panic. Thanks @Ted-Jiang
Co-authored-by: Andy Grove <[email protected]>
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.
Thanks @Ted-Jiang
Which issue does this PR close?
Related #2043.
Rationale for this change
Support construct a struct in SQL.
What changes are included in this PR?
Are there any user-facing changes?
Now the struct Field only support default name like
f_n
I want to use `select struct(1 as a)' as field name, but got error in parsing
I think it should fix in sql-parse, if wrong plz correct me.
I will create follow up ticket for this, if this PR is fine.