Uh oh!
There was an error while loading. Please reload this page.
Replace unwrap in convert_to_ordered_float and add downcast_value - #3347
Conversation
iajoiner
commented
Sep 2, 2022
| let array = values.as_any().downcast_ref::<Float64Array>().ok_or( | ||
| DataFusionError::Internal(format!( | ||
| "A Float64 type array, {:?} is somehow not a Float64Array", | ||
| values |
There was a problem hiding this comment.
I assume this won't print out 1GB of values in binary?
There was a problem hiding this comment.
This won't. The following function is usually used when implementing Debug.
https://github.com/apache/arrow-rs/blob/master/arrow/src/array/array.rs#L719-L757
Codecov Report
@@ Coverage Diff @@## master #3347 +/- ##
==========================================
+ Coverage 85.59% 85.61% +0.01%
==========================================
Files 296 297 +1 Lines 54214 54490 +276 ==========================================
+ Hits 46403 46649 +246 - Misses 7811 7841 +30
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> { | ||
| assert_eq!(1, states.len(), "expect only 1 element in the states"); | ||
| let binary_array = states[0].as_any().downcast_ref::<BinaryArray>().unwrap(); | ||
| let binary_array = states[0].as_any().downcast_ref::<BinaryArray>().ok_or( |
There was a problem hiding this comment.
It might be useful to introduce a helper function or macro for downcasting arrays to expected tyes. I think there might be one in arrow somewhere that I added a long time ago. I will try and find it.
There was a problem hiding this comment.
There is already a macro defined in this file that looks like what we need
macro_rules! downcast_value {
($Value: expr, $Type: ident, $T: tt) => {{
$Value[0]
.as_any()
.downcast_ref::<$Type<T>>()
.ok_or_else(|| {
DataFusionError::Internal(format!(
"could not cast value to {}",
type_name::<$Type<T>>()
))
})?
}};
}
There was a problem hiding this comment.
@andygrove Fixed! Actually I expanded the macro and put it in datafusion-common since it is everywhere.
unwrap in convert_to_ordered_floatunwrap in convert_to_ordered_float and add downcast_value
alamb
left a comment
There was a problem hiding this comment.
Seems like a nice improvement to me -- thanks @iajoiner and @avantgardnerio
| pub use scalar::{ScalarType, ScalarValue}; | ||
| #[macro_export] | ||
| macro_rules! downcast_value { |
There was a problem hiding this comment.
It would be great to add some doc comments here (especially as this will now become part of the public API). It could be done as a follow on PR too
There was a problem hiding this comment.
I took the liberty of adding some brief docs here
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ursabot
commented
Sep 10, 2022
Benchmark runs are scheduled for baseline = 24f2770 and contender = 4d22076. 4d22076 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3316.
Rationale for this change
Replace unwraps with Results
What changes are included in this PR?
Are there any user-facing changes?