Add external variant to ParquetError (#3285) #3574
Conversation
| //! # reader::{FileReader, SerializedFileReader}, | ||
| //! # writer::SerializedFileWriter, | ||
| //! # }, | ||
| //! # schema::parser::parse_message_type, |
| use std::{cell, io, result, str}; | ||
|
|
||
| #[cfg(feature = "arrow")] | ||
| use arrow_schema::ArrowError; | ||
|
|
||
| #[derive(Debug, PartialEq, Clone, Eq)] |
There was a problem hiding this comment.
The loss of PartialEq / Eq seems like it may cause a fairly major API breakage (if a ParquetError is embedded in another error type, for example)
Could we possibly add in a PartialEq impl that returns None for comparing External variants?
There was a problem hiding this comment.
I don't have a great idea for Copy other than to add a Copy bound to External
There was a problem hiding this comment.
I worry that lulls people into a false sense of security, I would rather scream test removal and see if that's ok
There was a problem hiding this comment.
I guess @alamb means a downstream project may use ParquetError embedded in another error type? For that we cannot detect in our test.
There was a problem hiding this comment.
Hence the scream test 😄
There was a problem hiding this comment.
I worry that lulls people into a false sense of security,
What is "false" about the sense of security 😆 ?
There was a problem hiding this comment.
That we don't actually provide a meaningful PartialEq implementation for a large portion of the errors we produce, I dunno it feels like a strange thing for an error to provide... I've double-checked that DataFusion doesn't provide something similar, neither do any of the other crates in this repo
| fn cause(&self) -> Option<&dyn ::std::error::Error> { | ||
| None | ||
| impl Error for ParquetError { | ||
| fn source(&self) -> Option<&(dyn Error + 'static)> { |
| } | ||
| } | ||
|
|
||
| impl From<cell::BorrowMutError> for ParquetError { | ||
| fn from(e: cell::BorrowMutError) -> ParquetError { | ||
| ParquetError::General(format!("underlying borrow error: {}", e)) | ||
| ParquetError::External(Box::new(e)) |
|
Benchmark runs are scheduled for baseline = 24e5dae and contender = 892a803. 892a803 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?
Closes #3285
Relates to #2725
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?