fix: do NOT convert errors to strings (#4436)

Wrap them into proper containers instead. Fixes #4434.
apache · Dec 1, 2022 · ffe97e5 · ffe97e5
1 parent d9e58db
commit ffe97e5
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 27 deletions.
diff --git a/datafusion-cli/src/command.rs b/datafusion-cli/src/command.rs
@@ -59,22 +59,16 @@ impl Command {
     ) -> Result<()> {
         let now = Instant::now();
         match self {
-            Self::Help => print_options
-                .print_batches(&[all_commands_info()], now)
-                .map_err(|e| DataFusionError::Execution(e.to_string())),
+            Self::Help => print_options.print_batches(&[all_commands_info()], now),
             Self::ListTables => {
                 let df = ctx.sql("SHOW TABLES").await?;
                 let batches = df.collect().await?;
-                print_options
-                    .print_batches(&batches, now)
-                    .map_err(|e| DataFusionError::Execution(e.to_string()))
+                print_options.print_batches(&batches, now)
             }
             Self::DescribeTable(name) => {
                 let df = ctx.sql(&format!("SHOW COLUMNS FROM {}", name)).await?;
                 let batches = df.collect().await?;
-                print_options
-                    .print_batches(&batches, now)
-                    .map_err(|e| DataFusionError::Execution(e.to_string()))
+                print_options.print_batches(&batches, now)
             }
             Self::Include(filename) => {
                 if let Some(filename) = filename {

diff --git a/datafusion-cli/src/object_storage.rs b/datafusion-cli/src/object_storage.rs
@@ -60,7 +60,7 @@ fn build_s3_object_store(url: &Url) -> Result<Arc<dyn object_store::ObjectStore>
     let host = get_host_name(url)?;
     match AmazonS3Builder::from_env().with_bucket_name(host).build() {
         Ok(s3) => Ok(Arc::new(s3)),
-        Err(err) => Err(DataFusionError::Execution(err.to_string())),
+        Err(err) => Err(DataFusionError::External(Box::new(err))),
     }
 }
 
@@ -73,7 +73,7 @@ fn build_gcs_object_store(url: &Url) -> Result<Arc<dyn object_store::ObjectStore
     }
     match builder.build() {
         Ok(gcs) => Ok(Arc::new(gcs)),
-        Err(err) => Err(DataFusionError::Execution(err.to_string())),
+        Err(err) => Err(DataFusionError::External(Box::new(err))),
     }
 }
 

diff --git a/datafusion-cli/src/print_format.rs b/datafusion-cli/src/print_format.rs
@@ -49,7 +49,7 @@ macro_rules! batches_to_json {
             writer.write_batches($batches)?;
             writer.finish()?;
         }
-        String::from_utf8(bytes).map_err(|e| DataFusionError::Execution(e.to_string()))?
+        String::from_utf8(bytes).map_err(|e| DataFusionError::External(Box::new(e)))?
     }};
 }
 
@@ -64,8 +64,8 @@ fn print_batches_with_sep(batches: &[RecordBatch], delimiter: u8) -> Result<Stri
             writer.write(batch)?;
         }
     }
-    let formatted = String::from_utf8(bytes)
-        .map_err(|e| DataFusionError::Execution(e.to_string()))?;
+    let formatted =
+        String::from_utf8(bytes).map_err(|e| DataFusionError::External(Box::new(e)))?;
     Ok(formatted)
 }
 

diff --git a/datafusion/core/src/datasource/listing_table_factory.rs b/datafusion/core/src/datasource/listing_table_factory.rs
@@ -103,9 +103,9 @@ impl TableProviderFactory for ListingTableFactory {
                 .table_partition_cols
                 .iter()
                 .map(|col| {
-                    schema.field_with_name(col).map_err(|arrow_err| {
-                        DataFusionError::Execution(arrow_err.to_string())
-                    })
+                    schema
+                        .field_with_name(col)
+                        .map_err(DataFusionError::ArrowError)
                 })
                 .collect::<datafusion_common::Result<Vec<_>>>()?
                 .into_iter()

diff --git a/datafusion/core/src/physical_plan/repartition.rs b/datafusion/core/src/physical_plan/repartition.rs
@@ -30,7 +30,7 @@ use crate::physical_plan::{
 };
 use arrow::array::{ArrayRef, UInt64Builder};
 use arrow::datatypes::SchemaRef;
-use arrow::error::Result as ArrowResult;
+use arrow::error::{ArrowError, Result as ArrowResult};
 use arrow::record_batch::RecordBatch;
 use log::debug;
 use tokio_stream::wrappers::UnboundedReceiverStream;
@@ -482,18 +482,25 @@ impl RepartitionExec {
         match input_task.await {
             // Error in joining task
             Err(e) => {
+                let e = Arc::new(e);
+
                 for (_, tx) in txs {
-                    let err = DataFusionError::Execution(format!("Join Error: {}", e));
-                    let err = Err(err.into());
+                    let err = Err(ArrowError::ExternalError(Box::new(
+                        DataFusionError::Context(
+                            "Join Error".to_string(),
+                            Box::new(DataFusionError::External(Box::new(Arc::clone(&e)))),
+                        ),
+                    )));
                     tx.send(Some(err)).ok();
                 }
             }
             // Error from running input task
             Ok(Err(e)) => {
+                let e = Arc::new(e);
+
                 for (_, tx) in txs {
                     // wrap it because need to send error to all output partitions
-                    let err = DataFusionError::Execution(e.to_string());
-                    let err = Err(err.into());
+                    let err = Err(ArrowError::ExternalError(Box::new(e.clone())));
                     tx.send(Some(err)).ok();
                 }
             }

diff --git a/datafusion/physical-expr/src/regex_expressions.rs b/datafusion/physical-expr/src/regex_expressions.rs
@@ -136,7 +136,7 @@ pub fn regexp_replace<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef>
                                     patterns.insert(pattern.to_string(), re.clone());
                                     Ok(re)
                                 },
-                                Err(err) => Err(DataFusionError::Execution(err.to_string())),
+                                Err(err) => Err(DataFusionError::External(Box::new(err))),
                             }
                         }
                     };
@@ -182,7 +182,7 @@ pub fn regexp_replace<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef>
                                     patterns.insert(pattern, re.clone());
                                     Ok(re)
                                 },
-                                Err(err) => Err(DataFusionError::Execution(err.to_string())),
+                                Err(err) => Err(DataFusionError::External(Box::new(err))),
                             }
                         }
                     };
@@ -254,8 +254,8 @@ fn _regexp_replace_static_pattern_replace<T: OffsetSizeTrait>(
         None => (pattern.to_string(), 1),
     };
 
-    let re = Regex::new(&pattern)
-        .map_err(|err| DataFusionError::Execution(err.to_string()))?;
+    let re =
+        Regex::new(&pattern).map_err(|err| DataFusionError::External(Box::new(err)))?;
 
     // Replaces the posix groups in the replacement string
     // with rust ones.
@@ -522,7 +522,7 @@ mod tests {
         let pattern_err = re.expect_err("broken pattern should have failed");
         assert_eq!(
             pattern_err.to_string(),
-            "Execution error: regex parse error:\n    [\n    ^\nerror: unclosed character class"
+            "External error: regex parse error:\n    [\n    ^\nerror: unclosed character class"
         );
     }