From 5e12b0070233cd6ad229aca6ed2fc8d95c68bf40 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 22 Apr 2024 17:11:38 -0700 Subject: [PATCH 1/5] possibility to strip error name --- datafusion/common/src/error.rs | 169 ++++++++++++++++++++++----------- 1 file changed, 111 insertions(+), 58 deletions(-) diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 4d9233d1f7c9..341729ce09ee 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -281,64 +281,7 @@ impl From for DataFusionError { impl Display for DataFusionError { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - match *self { - DataFusionError::ArrowError(ref desc, ref backtrace) => { - let backtrace = backtrace.clone().unwrap_or("".to_owned()); - write!(f, "Arrow error: {desc}{backtrace}") - } - #[cfg(feature = "parquet")] - DataFusionError::ParquetError(ref desc) => { - write!(f, "Parquet error: {desc}") - } - #[cfg(feature = "avro")] - DataFusionError::AvroError(ref desc) => { - write!(f, "Avro error: {desc}") - } - DataFusionError::IoError(ref desc) => { - write!(f, "IO error: {desc}") - } - DataFusionError::SQL(ref desc, ref backtrace) => { - let backtrace: String = backtrace.clone().unwrap_or("".to_owned()); - write!(f, "SQL error: {desc:?}{backtrace}") - } - DataFusionError::Configuration(ref desc) => { - write!(f, "Invalid or Unsupported Configuration: {desc}") - } - DataFusionError::NotImplemented(ref desc) => { - write!(f, "This feature is not implemented: {desc}") - } - DataFusionError::Internal(ref desc) => { - write!(f, "Internal error: {desc}.\nThis was likely caused by a bug in DataFusion's \ - code and we would welcome that you file an bug report in our issue tracker") - } - DataFusionError::Plan(ref desc) => { - write!(f, "Error during planning: {desc}") - } - DataFusionError::SchemaError(ref desc, ref backtrace) => { - let backtrace: &str = - &backtrace.as_ref().clone().unwrap_or("".to_owned()); - write!(f, "Schema error: {desc}{backtrace}") - } - DataFusionError::Execution(ref desc) => { - write!(f, "Execution error: {desc}") - } - DataFusionError::ResourcesExhausted(ref desc) => { - write!(f, "Resources exhausted: {desc}") - } - DataFusionError::External(ref desc) => { - write!(f, "External error: {desc}") - } - #[cfg(feature = "object_store")] - DataFusionError::ObjectStore(ref desc) => { - write!(f, "Object Store error: {desc}") - } - DataFusionError::Context(ref desc, ref err) => { - write!(f, "{}\ncaused by\n{}", desc, *err) - } - DataFusionError::Substrait(ref desc) => { - write!(f, "Substrait error: {desc}") - } - } + self.format_err(f) } } @@ -419,6 +362,9 @@ impl DataFusionError { Self::Context(description.into(), Box::new(self)) } + /// Strips backtrace out of the error message + /// If backtrace enabled then error has a format "message" [`Self::BACK_TRACE_SEP`] "backtrace" + /// The method strips the backtrace and outputs "message" pub fn strip_backtrace(&self) -> String { self.to_string() .split(Self::BACK_TRACE_SEP) @@ -450,6 +396,99 @@ impl DataFusionError { #[cfg(not(feature = "backtrace"))] "".to_owned() } + + fn get_error_name(&self) -> &'static str { + match self { + DataFusionError::ArrowError(_, _) => "Arrow error: ", + #[cfg(feature = "parquet")] + DataFusionError::ParquetError(_) => "Parquet error: ", + #[cfg(feature = "avro")] + DataFusionError::AvroError(_) => "Avro error: ", + #[cfg(feature = "object_store")] + DataFusionError::ObjectStore(_) => "Object Store error: ", + DataFusionError::IoError(_) => "IO error: ", + DataFusionError::SQL(_, _) => "SQL error: ", + DataFusionError::NotImplemented(_) => "This feature is not implemented: ", + DataFusionError::Internal(_) => "Internal error: ", + DataFusionError::Plan(_) => "Error during planning: ", + DataFusionError::Configuration(_) => "Invalid or Unsupported Configuration: ", + DataFusionError::SchemaError(_, _) => "Schema error: ", + DataFusionError::Execution(_) => "Execution error: ", + DataFusionError::ResourcesExhausted(_) => "Resources exhausted: ", + DataFusionError::External(_) => "External error: ", + DataFusionError::Context(_, _) => "", + DataFusionError::Substrait(_) => "Substrait error: ", + } + } + + fn format_err(&self, f: &mut Formatter) -> std::fmt::Result { + let error_name = self.get_error_name(); + match *self { + DataFusionError::ArrowError(ref desc, ref backtrace) => { + let backtrace = backtrace.clone().unwrap_or("".to_owned()); + write!(f, "{error_name}{desc}{backtrace}") + } + #[cfg(feature = "parquet")] + DataFusionError::ParquetError(ref desc) => { + write!(f, "{error_name}{desc}") + } + #[cfg(feature = "avro")] + DataFusionError::AvroError(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::IoError(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::SQL(ref desc, ref backtrace) => { + let backtrace: String = backtrace.clone().unwrap_or("".to_owned()); + write!(f, "{error_name}{desc:?}{backtrace}") + } + DataFusionError::Configuration(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::NotImplemented(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::Internal(ref desc) => { + write!(f, "{error_name}{desc}.\nThis was likely caused by a bug in DataFusion's \ + code and we would welcome that you file an bug report in our issue tracker") + } + DataFusionError::Plan(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::SchemaError(ref desc, ref backtrace) => { + let backtrace: &str = + &backtrace.as_ref().clone().unwrap_or("".to_owned()); + write!(f, "{error_name}{desc}{backtrace}") + } + DataFusionError::Execution(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::ResourcesExhausted(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::External(ref desc) => { + write!(f, "{error_name}{desc}") + } + #[cfg(feature = "object_store")] + DataFusionError::ObjectStore(ref desc) => { + write!(f, "{error_name}{desc}") + } + DataFusionError::Context(ref desc, ref err) => { + write!(f, "{error_name}{desc}\ncaused by\n{}", *err) + } + DataFusionError::Substrait(ref desc) => { + write!(f, "{error_name}{desc}") + } + } + } + + /// Strips error description out of the error message + /// Error has a format `{error_name}{`error`} + /// The method strips the `error_name` from level and outputs 'error`` + pub fn strip_error_name(self) -> String { + self.to_string().replace(self.get_error_name(), "") + } } /// Unwrap an `Option` if possible. Otherwise return an `DataFusionError::Internal`. @@ -778,6 +817,20 @@ mod test { ); } + #[test] + fn test_strip_error_name() { + let res: Result<(), DataFusionError> = plan_err!("Err"); + let res = res.unwrap_err(); + assert_eq!(res.strip_error_name(), "Err"); + + // Test only top level stripped + let res: Result<(), DataFusionError> = plan_err!("Err"); + let res2: Result<(), DataFusionError> = + exec_err!("{}", res.unwrap_err().strip_backtrace()); + let res2 = res2.unwrap_err(); + assert_eq!(res2.strip_error_name(), "Error during planning: Err"); + } + /// Model what happens when implementing SendableRecordBatchStream: /// DataFusion code needs to return an ArrowError fn return_arrow_error() -> arrow::error::Result<()> { From 6b1872998e60ef51833f775670059a51c1feb8f3 Mon Sep 17 00:00:00 2001 From: comphead Date: Mon, 22 Apr 2024 17:41:39 -0700 Subject: [PATCH 2/5] fix test --- datafusion/common/src/error.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 341729ce09ee..f2bd61abf7cd 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -818,6 +818,7 @@ mod test { } #[test] + #[cfg(not(feature = "backtrace"))] fn test_strip_error_name() { let res: Result<(), DataFusionError> = plan_err!("Err"); let res = res.unwrap_err(); From c7bfee8fadcf778d48b54aa3269d4b748cb944c6 Mon Sep 17 00:00:00 2001 From: comphead Date: Fri, 26 Apr 2024 12:41:45 -0700 Subject: [PATCH 3/5] comments --- datafusion/common/src/error.rs | 92 +++++++++------------------------- 1 file changed, 25 insertions(+), 67 deletions(-) diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index f2bd61abf7cd..8b9881852853 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -19,6 +19,7 @@ #[cfg(feature = "backtrace")] use std::backtrace::{Backtrace, BacktraceStatus}; +use std::borrow::Cow; use std::error::Error; use std::fmt::{Display, Formatter}; use std::io; @@ -281,7 +282,9 @@ impl From for DataFusionError { impl Display for DataFusionError { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - self.format_err(f) + let error_name = self.error_name(); + let message = self.message(); + write!(f, "{error_name}{message}") } } @@ -397,7 +400,7 @@ impl DataFusionError { "".to_owned() } - fn get_error_name(&self) -> &'static str { + fn error_name(&self) -> &'static str { match self { DataFusionError::ArrowError(_, _) => "Arrow error: ", #[cfg(feature = "parquet")] @@ -421,74 +424,44 @@ impl DataFusionError { } } - fn format_err(&self, f: &mut Formatter) -> std::fmt::Result { - let error_name = self.get_error_name(); + fn message(&self) -> Cow { match *self { DataFusionError::ArrowError(ref desc, ref backtrace) => { let backtrace = backtrace.clone().unwrap_or("".to_owned()); - write!(f, "{error_name}{desc}{backtrace}") + Cow::Owned(format!("{desc}{backtrace}")) } #[cfg(feature = "parquet")] - DataFusionError::ParquetError(ref desc) => { - write!(f, "{error_name}{desc}") - } + DataFusionError::ParquetError(ref desc) => Cow::Owned(desc.to_string()), #[cfg(feature = "avro")] - DataFusionError::AvroError(ref desc) => { - write!(f, "{error_name}{desc}") - } - DataFusionError::IoError(ref desc) => { - write!(f, "{error_name}{desc}") - } + DataFusionError::AvroError(ref desc) => Cow::Owned(desc.to_string()), + DataFusionError::IoError(ref desc) => Cow::Owned(desc.to_string()), DataFusionError::SQL(ref desc, ref backtrace) => { let backtrace: String = backtrace.clone().unwrap_or("".to_owned()); - write!(f, "{error_name}{desc:?}{backtrace}") - } - DataFusionError::Configuration(ref desc) => { - write!(f, "{error_name}{desc}") - } - DataFusionError::NotImplemented(ref desc) => { - write!(f, "{error_name}{desc}") - } - DataFusionError::Internal(ref desc) => { - write!(f, "{error_name}{desc}.\nThis was likely caused by a bug in DataFusion's \ - code and we would welcome that you file an bug report in our issue tracker") - } - DataFusionError::Plan(ref desc) => { - write!(f, "{error_name}{desc}") + Cow::Owned(format!("{desc:?}{backtrace}")) } + DataFusionError::Configuration(ref desc) => Cow::Owned(desc.to_string()), + DataFusionError::NotImplemented(ref desc) => Cow::Owned(desc.to_string()), + DataFusionError::Internal(ref desc) => Cow::Owned(format!( + "{desc}.\nThis was likely caused by a bug in DataFusion's \ + code and we would welcome that you file an bug report in our issue tracker" + )), + DataFusionError::Plan(ref desc) => Cow::Owned(desc.to_string()), DataFusionError::SchemaError(ref desc, ref backtrace) => { let backtrace: &str = &backtrace.as_ref().clone().unwrap_or("".to_owned()); - write!(f, "{error_name}{desc}{backtrace}") - } - DataFusionError::Execution(ref desc) => { - write!(f, "{error_name}{desc}") - } - DataFusionError::ResourcesExhausted(ref desc) => { - write!(f, "{error_name}{desc}") - } - DataFusionError::External(ref desc) => { - write!(f, "{error_name}{desc}") + Cow::Owned(format!("{desc}{backtrace}")) } + DataFusionError::Execution(ref desc) => Cow::Owned(desc.to_string()), + DataFusionError::ResourcesExhausted(ref desc) => Cow::Owned(desc.to_string()), + DataFusionError::External(ref desc) => Cow::Owned(desc.to_string()), #[cfg(feature = "object_store")] - DataFusionError::ObjectStore(ref desc) => { - write!(f, "{error_name}{desc}") - } + DataFusionError::ObjectStore(ref desc) => Cow::Owned(desc.to_string()), DataFusionError::Context(ref desc, ref err) => { - write!(f, "{error_name}{desc}\ncaused by\n{}", *err) - } - DataFusionError::Substrait(ref desc) => { - write!(f, "{error_name}{desc}") + Cow::Owned(format!("{desc}\ncaused by\n{}", *err)) } + DataFusionError::Substrait(ref desc) => Cow::Owned(desc.to_string()), } } - - /// Strips error description out of the error message - /// Error has a format `{error_name}{`error`} - /// The method strips the `error_name` from level and outputs 'error`` - pub fn strip_error_name(self) -> String { - self.to_string().replace(self.get_error_name(), "") - } } /// Unwrap an `Option` if possible. Otherwise return an `DataFusionError::Internal`. @@ -817,21 +790,6 @@ mod test { ); } - #[test] - #[cfg(not(feature = "backtrace"))] - fn test_strip_error_name() { - let res: Result<(), DataFusionError> = plan_err!("Err"); - let res = res.unwrap_err(); - assert_eq!(res.strip_error_name(), "Err"); - - // Test only top level stripped - let res: Result<(), DataFusionError> = plan_err!("Err"); - let res2: Result<(), DataFusionError> = - exec_err!("{}", res.unwrap_err().strip_backtrace()); - let res2 = res2.unwrap_err(); - assert_eq!(res2.strip_error_name(), "Error during planning: Err"); - } - /// Model what happens when implementing SendableRecordBatchStream: /// DataFusion code needs to return an ArrowError fn return_arrow_error() -> arrow::error::Result<()> { From 1b214f2458827883eed122d33446ab2645f00f2c Mon Sep 17 00:00:00 2001 From: comphead Date: Fri, 26 Apr 2024 13:24:32 -0700 Subject: [PATCH 4/5] Update datafusion/common/src/error.rs Co-authored-by: Andy Grove --- datafusion/common/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 8b9881852853..f79c27e27a8c 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -424,7 +424,7 @@ impl DataFusionError { } } - fn message(&self) -> Cow { + pub fn message(&self) -> Cow { match *self { DataFusionError::ArrowError(ref desc, ref backtrace) => { let backtrace = backtrace.clone().unwrap_or("".to_owned()); From e6e1f8ac62d5d805e1283847731003bc4bcca14f Mon Sep 17 00:00:00 2001 From: comphead Date: Sat, 27 Apr 2024 15:58:14 -0700 Subject: [PATCH 5/5] rename method --- datafusion/common/src/error.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index 8b9881852853..de007d10f807 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -282,9 +282,9 @@ impl From for DataFusionError { impl Display for DataFusionError { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - let error_name = self.error_name(); + let error_prefix = self.error_prefix(); let message = self.message(); - write!(f, "{error_name}{message}") + write!(f, "{error_prefix}{message}") } } @@ -400,7 +400,7 @@ impl DataFusionError { "".to_owned() } - fn error_name(&self) -> &'static str { + fn error_prefix(&self) -> &'static str { match self { DataFusionError::ArrowError(_, _) => "Arrow error: ", #[cfg(feature = "parquet")]