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

[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