feat: refactor use of Error::io #5612
Conversation
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
Thanks for starting this. I have several suggestions.
Feel free to suggest that we add or change an error type. For example, maybe we want to make a more general NotFound error that's separate from FileNotFound error.
| pub fn not_found(message: impl Into<String>, location: Location) -> Self { | ||
| let message: String = message.into(); | ||
| Self::NotFound { | ||
| uri: message, | ||
| location, | ||
| } |
There was a problem hiding this comment.
We could probably simplify this a lot by just using std::panic::Location::caller(). This can even be applied to existing variants.
| pub fn not_found(message: impl Into<String>, location: Location) -> Self { | |
| let message: String = message.into(); | |
| Self::NotFound { | |
| uri: message, | |
| location, | |
| } | |
| pub fn not_found(uri: impl Into<String>) -> Self { | |
| Self::NotFound { | |
| uri: uri.into(), | |
| location: std::panic::Location::caller().to_snafu_location(), | |
| } |
| // Regular data column - validate it exists in base schema | ||
| if base.schema().field(&field.name).is_none() { | ||
| return Err(Error::io( | ||
| return Err(Error::not_found( |
There was a problem hiding this comment.
The not_found error is really a FileNotFound error, so doesn't apply here.
| return Err(Error::not_found( | |
| return Err(Error::invalid_input( |
| batch.column_by_name(&field.name).ok_or_else(|| { | ||
| Error::io( | ||
| format!("FileWriter::write: Field '{}' not found", field.name), | ||
| Error::not_found( |
There was a problem hiding this comment.
| Error::not_found( | |
| Error::invalid_input( |
| })?; | ||
|
|
||
| let value_arr = dict_info.values.as_ref().ok_or_else(|| { | ||
| Error::io( |
There was a problem hiding this comment.
Missed one here:
| Error::io( | |
| Error::invalid_input( |
| Error::io( | ||
| format!( | ||
| "Lance field {} is dictionary type, but misses the dictionary value array", | ||
| "Lance field {} is dictionary type, but misses the dictionary value array", |
There was a problem hiding this comment.
We should probably make it standard that we wrap the field names in quotes:
| "Lance field {} is dictionary type, but misses the dictionary value array", | |
| "Lance field '{}' is dictionary type, but misses the dictionary value array", |
| ) -> Result<&'a PageInfo> { | ||
| page_table.get(field.id, batch_id).ok_or_else(|| { | ||
| Error::io( | ||
| Error::not_found( |
There was a problem hiding this comment.
| Error::not_found( | |
| Error::invalid_input( |
|
|
||
| if buf.len() < 16 { | ||
| return Err(Error::io( | ||
| return Err(Error::invalid_input( |
There was a problem hiding this comment.
This seems like it might be corrupt file to me.
| return Err(Error::invalid_input( | |
| return Err(Error::corrupt_file( |
| } | ||
| if !buf.ends_with(MAGIC) { | ||
| return Err(Error::io( | ||
| return Err(Error::invalid_input( |
There was a problem hiding this comment.
Same here:
| return Err(Error::invalid_input( | |
| return Err(Error::corrupt_file( |
2 participants
fix #5601