Return error from JSON writer rather than panic #1205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1205 +/- ##
==========================================
+ Coverage 82.57% 82.67% +0.09%
==========================================
Files 173 175 +2
Lines 50713 51568 +855
==========================================
+ Hits 41875 42632 +757
- Misses 8838 8936 +98
Continue to review full report at Codecov.
|
arrow/src/json/writer.rs
Outdated
| .iter() | ||
| .map(|maybe_value| match maybe_value { | ||
| Some(v) => Value::Array(array_to_json_array(&v)), | ||
| Some(v) => Value::Array(array_to_json_array(&v).unwrap()), |
There was a problem hiding this comment.
when i change unwarp() with ?
the trait bound Value: FromResidual<std::result::Result<Infallible, ArrowError>>is not satisfied
Can someone give some advice?
There was a problem hiding this comment.
Since this is inside a closure, that closure needs to have a return type of Result in order to use the ? operator. You can achieve that by wrapping the Value in Ok like this:
DataType::List(_) => as_list_array(array)
.iter()
.map(|maybe_value| {
match maybe_value {
Some(v) => Ok(Value::Array(array_to_json_array(&v)?)),
None => Ok(Value::Null),
}})
.collect(),
The collect at the end is able to create a Result<Vec<X>> from that Iterator<Result<X>>. The type is inferred based on the method return type. Often you would have to specify that explicitly like .collect::<Result<Vec<_>>().
There was a problem hiding this comment.
Thanks ! @jhorstmann this really helps me!
I have a little doubt about Ok(Value::Array(array_to_json_array(&v)?))
if ? return a Err then it will be Ok(Err), is there some info i lost (i am a Rust newbie 😂)
There was a problem hiding this comment.
if ? return a Err then it will be Ok(Err), is there some info i lost (i am a Rust newbie 😂)
The difference is that ? actually returns an Err from the function (as opposed to evaluating to Err)
There was a problem hiding this comment.
I think this looks good @Ted-Jiang -- thank you very much. ❤️
I'll plan to merge it when CI is green
Thanks a lot !
There was a problem hiding this comment.
Thank you @Ted-Jiang -- this is looking quite nice and is quite close to merging. 👍
arrow/src/json/writer.rs
Outdated
| match set_column_for_json_rows( | ||
| &mut inner_objs, | ||
| row_count, | ||
| struct_col, | ||
| inner_col_names[j], | ||
| ) { | ||
| Ok(_) => {} | ||
| Err(e) => return Err(e), | ||
| } |
There was a problem hiding this comment.
This pattern is exactly what the ? operator does. There is some nice coverage of this operator in "The Book": https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html#a-shortcut-for-propagating-errors-the--operator
| match set_column_for_json_rows( | |
| &mut inner_objs, | |
| row_count, | |
| struct_col, | |
| inner_col_names[j], | |
| ) { | |
| Ok(_) => {} | |
| Err(e) => return Err(e), | |
| } | |
| set_column_for_json_rows( | |
| &mut inner_objs, | |
| row_count, | |
| struct_col, | |
| inner_col_names[j], | |
| )?; |
There was a problem hiding this comment.
@alamb Thanks! After this PR, I have learn more philosophy of the rust error handling .
There was a problem hiding this comment.
Indeed -- we are all learning together! Thank you for your contributions
There was a problem hiding this comment.
I think this looks good @Ted-Jiang -- thank you very much. ❤️
I'll plan to merge it when CI is green
Which issue does this PR close?
Closes #1157.
Rationale for this change
Not panic in JSON write, just throw up error
What changes are included in this PR?
Are there any user-facing changes?