-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve Error Handling and Readibility for downcasting StructArray
#4061
Improve Error Handling and Readibility for downcasting StructArray
#4061
Conversation
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 @retikulum -- I have a few comments but I think this PR is also an improvement as is
benchmarks/src/tpch.rs
Outdated
let array = match as_date32_array(column) { | ||
Ok(array) => array, | ||
Err(e) => panic!("{}", e), | ||
}; |
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.
I think unwrap
will also print the error message if called on an error:
https://doc.rust-lang.org/std/result/enum.Result.html#panics-1
Panics if the value is an Err, with a panic message provided by the Err’s value.
let array = match as_date32_array(column) { | |
Ok(array) => array, | |
Err(e) => panic!("{}", e), | |
}; | |
let array = as_date32_array(column).unwrap(); |
datafusion/common/src/cast.rs
Outdated
@@ -32,3 +32,13 @@ pub fn as_date32_array(array: &dyn Array) -> Result<&Date32Array, DataFusionErro | |||
)) | |||
}) | |||
} | |||
|
|||
// Downcast ArrayRef to Date32Array |
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.
// Downcast ArrayRef to Date32Array | |
// Downcast ArrayRef to StructArray |
datafusion/common/src/scalar.rs
Outdated
@@ -3611,7 +3604,10 @@ mod tests { | |||
// iter_to_array for struct scalars | |||
let array = | |||
ScalarValue::iter_to_array(vec![s0.clone(), s1.clone(), s2.clone()]).unwrap(); | |||
let array = array.as_any().downcast_ref::<StructArray>().unwrap(); | |||
let array = match as_struct_array(&array) { |
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.
see comment above
👍 Thanks again @retikulum |
Benchmark runs are scheduled for baseline = 61429f8 and contender = 761e167. 761e167 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…pache#4061) * improve error messages for StructArray * refactor newly added Date32Array downcasting and correct error string * beautify code * changes after code review * fix formatting
Which issue does this PR close?
Part of #3152.
Rationale for this change
It is very clear in the issue but there are different schemas while downcasting an ArrayRef to a related arrow array type. This is the second PR of improving downcasting of
StructArray
.What changes are included in this PR?
as_struct_array
function is implemented indatafusion\common\src\cast.rs
Are there any user-facing changes?
I don't think so.