You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/01/10 17:35:31 UTC

[GitHub] [arrow-datafusion] matthewmturner opened a new pull request #1541: Replace Datafusion Error with Generic Error for Object store

matthewmturner opened a new pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541


   # Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
   -->
   
   Closes #1540 
   
    # Rationale for this change
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   # What changes are included in this PR?
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   # Are there any user-facing changes?
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please add the `api change` label.
   -->
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r783409315



##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       Sounds good and makes sense, thanks much @houqp.  I will work on that.
   
   FYI there is existing `IoError(io::Error)` variant.  I think ill have to see how well that aligns with the types of errors we get in the `ObjectStore` (i.e are the `io::Error`?)




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] hntd187 commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r781547861



##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -40,18 +41,21 @@ use crate::error::{DataFusionError, Result};
 #[async_trait]
 pub trait ObjectReader: Send + Sync {
     /// Get reader for a part [start, start + length] in the file asynchronously
-    async fn chunk_reader(&self, start: u64, length: usize)
-        -> Result<Box<dyn AsyncRead>>;
+    async fn chunk_reader(
+        &self,
+        start: u64,
+        length: usize,
+    ) -> Result<Box<dyn AsyncRead>, Box<dyn Error>>;

Review comment:
       Shouldn't this be `Send + Sync` also?




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb merged pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541


   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1010302151


   I wrote up a suggestion on https://github.com/apache/arrow-datafusion/issues/1540#issuecomment-1010301819 which might be useful


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1011297087


   @alamb @houqp i made the discusssed change (adding `External` `DataFusionError` variant and implementing `From`).  This results in changes being needed in many places - including all the way up to `ExecutionContext` methods.  I just wanted to make sure that aligned with your expectations.  Youll see examples in my latest commit.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1009548471


   > I think you are on the right track, have you tried testing this with the s3 object store?
   
   not yet, i hadnt planned on checking with that until i got datafusion tests to pass but maybe that was the wrong approach.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] hntd187 commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r781661135



##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -40,18 +41,21 @@ use crate::error::{DataFusionError, Result};
 #[async_trait]
 pub trait ObjectReader: Send + Sync {
     /// Get reader for a part [start, start + length] in the file asynchronously
-    async fn chunk_reader(&self, start: u64, length: usize)
-        -> Result<Box<dyn AsyncRead>>;
+    async fn chunk_reader(
+        &self,
+        start: u64,
+        length: usize,
+    ) -> Result<Box<dyn AsyncRead>, Box<dyn Error>>;

Review comment:
       Not disagreeing with the overall point of this PR, error mapping is a pretty laborious task in Rust, I'm just trying to save a second PR when/if the Send + Sync bites you. :)




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1013349401


   > I am a little confused that all the object store traits are changed as well (I thought this PR was going to add the `DataFusionError::External` bit and then implementers of the object store trait would use `DataFusionError::External` to wrap their errors
   > 
   
   I understand the confusion.  The original intent of the PR was to make the Error generic / not a DataFusionError.  Then we added the External variant.  I suppose i merged those but in reality only one is needed. 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r783397832



##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       I should probably have a IOError or ObjectStoreError enum variant specifically for wrapping object store errors :)
   
   As for downcasting error trait objects, it's best to be managed by the users because datafusion core will not have access to all different types of error that plugin object store will return. We can do that for arrow compute kernels because arrow is a dependency of datafusion.

##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       We should probably have a IOError or ObjectStoreError enum variant specifically for wrapping object store errors :)
   
   As for downcasting error trait objects, it's best to be managed by the users because datafusion core will not have access to all different types of error that plugin object store will return. We can do that for arrow compute kernels because arrow is a dependency of datafusion.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r781667154



##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -40,18 +41,21 @@ use crate::error::{DataFusionError, Result};
 #[async_trait]
 pub trait ObjectReader: Send + Sync {
     /// Get reader for a part [start, start + length] in the file asynchronously
-    async fn chunk_reader(&self, start: u64, length: usize)
-        -> Result<Box<dyn AsyncRead>>;
+    async fn chunk_reader(
+        &self,
+        start: u64,
+        length: usize,
+    ) -> Result<Box<dyn AsyncRead>, Box<dyn Error>>;

Review comment:
       aha i hear you and agree - this was more painful than i was hoping.  i will def add those traits if needed. fyi context for the request comes from this https://github.com/datafusion-contrib/datafusion-objectstore-s3/pull/12




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] houqp commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1009547274


   I think you are on the right track, have you tried testing this with the s3 object store?


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r783380684



##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       @houqp apologies if im misunderstanding, but is this what youre referring to?  Since i cant match on trait object i thought i was limited on the types of `DataFusionError` I could return with this.  I was definitely trying to make changes that would "bubble up" automatically but i must have done something incorrectly as thats clearly not happening.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1013271090


   @houqp @alamb ready for review


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r785044426



##########
File path: datafusion/src/datasource/listing/helpers.rs
##########
@@ -191,12 +191,13 @@ pub async fn pruned_partition_list(
         Ok(Box::pin(
             store
                 .list_file_with_suffix(table_path, file_extension)
+                // .map_err(DataFusionError::from)

Review comment:
       probably can be removed

##########
File path: datafusion/src/datasource/listing/helpers.rs
##########
@@ -230,6 +231,7 @@ pub async fn pruned_partition_list(
             // all the files anyway. This number will need to be adjusted according to the object
             // store if we switch to a streaming-stlye pruning of the files. For instance S3 lists
             // 1000 items at a time so batches of 1000 would be ideal with S3 as store.
+            .map_err(DataFusionError::from)

Review comment:
       ```suggestion
   ```
   
   I don't think this is needed (the `?` does it "magically")

##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -39,19 +38,22 @@ pub struct LocalFileSystem;
 
 #[async_trait]
 impl ObjectStore for LocalFileSystem {
-    async fn list_file(&self, prefix: &str) -> Result<FileMetaStream> {
+    async fn list_file(&self, prefix: &str) -> Result<FileMetaStream, GenericError> {

Review comment:
       this is fine to me, though I also think it is fine to keep this returning a `DataFusion` error too as it is straightforward to construct them from `Box<..>` errors. 🤔 
   
   

##########
File path: datafusion/src/datasource/listing/helpers.rs
##########
@@ -191,12 +191,13 @@ pub async fn pruned_partition_list(
         Ok(Box::pin(
             store
                 .list_file_with_suffix(table_path, file_extension)
+                // .map_err(DataFusionError::from)
                 .await?
                 .filter_map(move |f| {
                     let stream_path = stream_path.clone();
                     let table_partition_cols_stream = table_partition_cols_stream.clone();
                     async move {
-                        let file_meta = match f {
+                        let file_meta = match f.map_err(DataFusionError::from) {
                             Ok(fm) => fm,
                             Err(err) => return Some(Err(err)),

Review comment:
       ```suggestion
                           let file_meta = match f {
                               Ok(fm) => fm,
                               Err(err) => return Some(Err(err.into())),
   ```




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1009538806


   @houqp @yjshen its possible i underestimated the work required for this.
   
   would you be able to check out what ive done so far to see if its going in the right direction?


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1012392279


   Odd - i didnt get these errors / everything compiles and passes cargo locally.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1012397147


   Nevermind - found the issue


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r781558561



##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -40,18 +41,21 @@ use crate::error::{DataFusionError, Result};
 #[async_trait]
 pub trait ObjectReader: Send + Sync {
     /// Get reader for a part [start, start + length] in the file asynchronously
-    async fn chunk_reader(&self, start: u64, length: usize)
-        -> Result<Box<dyn AsyncRead>>;
+    async fn chunk_reader(
+        &self,
+        start: u64,
+        length: usize,
+    ) -> Result<Box<dyn AsyncRead>, Box<dyn Error>>;

Review comment:
       Probably.  
   
   But it seems a majority of the issues currently are coming from converting `DataFusionError`.  I havent had chance to look into this yet though.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] houqp commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1011336232


   Have you considered using `impl From<Box<dyn Error + Send + Sync>> for DataFusionError` trait to convert `Error` trait objects into Datafusioned errors at object store api call site so we won't need to bubble it up all the way to high level public datafusion APIs?


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r783382875



##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       I was referring to something like this `let mut reader = obj_reader?.sync_reader().map_err(DatafusionError::from)?;` so the low level functions that call these object store apis can hide the Error trait objects from their callers.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1016782342


   @alamb ive made updates based on your comments.  let me know if any other concerns.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#issuecomment-1012427494


   looks like ill need to rebase to pick up the clippy changes.
   
   On the clippy error that `Box<dyn Error + Send + Sync>` is too complex and to use a type for it - what do you guys (@houqp @alamb) think about `pub type GenericError = Box<dyn Error + Send + Sync>`


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [arrow-datafusion] matthewmturner commented on a change in pull request #1541: Replace Datafusion Error with Generic Error for Object store

Posted by GitBox <gi...@apache.org>.
matthewmturner commented on a change in pull request #1541:
URL: https://github.com/apache/arrow-datafusion/pull/1541#discussion_r783395275



##########
File path: datafusion/src/error.rs
##########
@@ -101,6 +104,12 @@ impl From<ParserError> for DataFusionError {
     }
 }
 
+impl From<Box<dyn Error + Send + Sync>> for DataFusionError {
+    fn from(err: Box<dyn Error + Send + Sync>) -> Self {
+        DataFusionError::External(err)
+    }
+}
+

Review comment:
       Ah - thank you for the clarification. I can try that. So all of these sites would now have External errors. 
   
   I'm wondering if there would be value in having a macro or function that could downcast the trait object to concrete datafusionerrors (kind of like what we do with compute kernels to downcast to concrete array types)  - so we could really use external for true external code.
   
   Maybe that's a next step though. Just thinking out loud.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org