alamb commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082566616
##########
parquet/src/column/mod.rs:
##########
@@ -36,18 +36,18 @@
//! repetition levels and read them to verify write/read correctness.
//!
//! ```rust,no_run
-//! use std::{fs, path::Path, sync::Arc};
-//!
-//! use parquet::{
-//! column::{reader::ColumnReader, writer::ColumnWriter},
-//! data_type::Int32Type,
-//! file::{
-//! properties::WriterProperties,
-//! reader::{FileReader, SerializedFileReader},
-//! writer::SerializedFileWriter,
-//! },
-//! schema::parser::parse_message_type,
-//! };
+//! # use std::{fs, path::Path, sync::Arc};
+//! #
+//! # use parquet::{
+//! # column::{reader::ColumnReader, writer::ColumnWriter},
+//! # data_type::Int32Type,
+//! # file::{
+//! # properties::WriterProperties,
+//! # reader::{FileReader, SerializedFileReader},
+//! # writer::SerializedFileWriter,
+//! # },
+//! # schema::parser::parse_message_type,
Review Comment:
drive by doc cleanup?
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review 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?
##########
parquet/src/errors.rs:
##########
@@ -39,66 +40,72 @@ pub enum ParquetError {
/// Returned when reading into arrow or writing from arrow.
ArrowError(String),
IndexOutOfBound(usize, usize),
+ /// An external error variant
+ External(Box<dyn Error + Send + Sync>),
}
impl std::fmt::Display for ParquetError {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
- match *self {
- ParquetError::General(ref message) => {
+ match &self {
+ ParquetError::General(message) => {
write!(fmt, "Parquet error: {}", message)
}
- ParquetError::NYI(ref message) => write!(fmt, "NYI: {}", message),
- ParquetError::EOF(ref message) => write!(fmt, "EOF: {}", message),
+ ParquetError::NYI(message) => write!(fmt, "NYI: {}", message),
+ ParquetError::EOF(message) => write!(fmt, "EOF: {}", message),
#[cfg(feature = "arrow")]
- ParquetError::ArrowError(ref message) => write!(fmt, "Arrow: {}",
message),
- ParquetError::IndexOutOfBound(ref index, ref bound) => {
+ ParquetError::ArrowError(message) => write!(fmt, "Arrow: {}",
message),
+ ParquetError::IndexOutOfBound(index, ref bound) => {
write!(fmt, "Index {} out of bound: {}", index, bound)
}
+ ParquetError::External(e) => write!(fmt, "External: {}", e),
}
}
}
-impl std::error::Error for ParquetError {
- fn cause(&self) -> Option<&dyn ::std::error::Error> {
- None
+impl Error for ParquetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ ParquetError::External(e) => Some(e.as_ref()),
+ _ => None,
+ }
}
}
impl From<io::Error> for ParquetError {
fn from(e: io::Error) -> ParquetError {
- ParquetError::General(format!("underlying IO error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
#[cfg(any(feature = "snap", test))]
impl From<snap::Error> for ParquetError {
fn from(e: snap::Error) -> ParquetError {
- ParquetError::General(format!("underlying snap error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
impl From<thrift::Error> for ParquetError {
fn from(e: thrift::Error) -> ParquetError {
- ParquetError::General(format!("underlying Thrift error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
impl From<cell::BorrowMutError> for ParquetError {
fn from(e: cell::BorrowMutError) -> ParquetError {
- ParquetError::General(format!("underlying borrow error: {}", e))
+ ParquetError::External(Box::new(e))
Review Comment:
nice
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
I don't have a great idea for Copy other than to add a `Copy` bound to
External
##########
parquet/src/errors.rs:
##########
@@ -39,66 +40,72 @@ pub enum ParquetError {
/// Returned when reading into arrow or writing from arrow.
ArrowError(String),
IndexOutOfBound(usize, usize),
+ /// An external error variant
+ External(Box<dyn Error + Send + Sync>),
}
impl std::fmt::Display for ParquetError {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
- match *self {
- ParquetError::General(ref message) => {
+ match &self {
+ ParquetError::General(message) => {
write!(fmt, "Parquet error: {}", message)
}
- ParquetError::NYI(ref message) => write!(fmt, "NYI: {}", message),
- ParquetError::EOF(ref message) => write!(fmt, "EOF: {}", message),
+ ParquetError::NYI(message) => write!(fmt, "NYI: {}", message),
+ ParquetError::EOF(message) => write!(fmt, "EOF: {}", message),
#[cfg(feature = "arrow")]
- ParquetError::ArrowError(ref message) => write!(fmt, "Arrow: {}",
message),
- ParquetError::IndexOutOfBound(ref index, ref bound) => {
+ ParquetError::ArrowError(message) => write!(fmt, "Arrow: {}",
message),
+ ParquetError::IndexOutOfBound(index, ref bound) => {
write!(fmt, "Index {} out of bound: {}", index, bound)
}
+ ParquetError::External(e) => write!(fmt, "External: {}", e),
}
}
}
-impl std::error::Error for ParquetError {
- fn cause(&self) -> Option<&dyn ::std::error::Error> {
- None
+impl Error for ParquetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
Review Comment:
❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
