-
Notifications
You must be signed in to change notification settings - Fork 164
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
feat: Implement CAST between struct types #1074
Conversation
@parthchandra @mbutrovich could you review? We need more extensive tests for sure, but it will be easier to add those as part of the |
native/spark-expr/src/cast.rs
Outdated
) -> DataFusionResult<ArrayRef> { | ||
match (from_type, to_type) { | ||
(DataType::Struct(from_fields), DataType::Struct(to_fields)) => { | ||
assert!(to_fields.len() <= from_fields.len()); |
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.
Hmm, why we have this assert? In Spark, Cast
expression requires from_fields length equal to to_fields length. So we shouldn't encounter the case that they are not equal on an analyzed query plan.
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, I have removed that. I was confused by an error about an unsupported cast that was dropping a struct field, but this came from DataFusion and I understand why now.
Spark should have many Cast expression tests. As we pass Spark tests, it should be fine for general cases. |
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.
Looks good to me. Just one question about the assert added.
f448136
to
f867c04
Compare
Which issue does this PR close?
Closes #815
Rationale for this change
We need support for casting between struct types to support reading structs from Parquet using DataFusion's ParquetExec.
What changes are included in this PR?
How are these changes tested?