alamb commented on code in PR #16502:
URL: https://github.com/apache/datafusion/pull/16502#discussion_r2167502727


##########
datafusion-cli/src/exec.rs:
##########
@@ -231,10 +231,24 @@ pub(super) async fn exec_and_print(
         let adjusted =
             
AdjustedPrintOptions::new(print_options.clone()).with_statement(&statement);
 
-        let plan = create_plan(ctx, statement).await?;
+        // Only clone the statement if it's a CreateExternalTable

Review Comment:
   This logic is pretty specific to S3 and `create_table` -- inlining it here 
obscures the rest of the control flow that is happening.
   
   Is there any way we can encapsulate these special cases somewhere? 
   
   Perhaps something like 
   ```rust
   struct StatementExecutor {
     statement: DFStatement 
     ...
     // should we automatically look up the region of a failed s3 request?
      retry_region: bool,
   }
   impl StatementExecutor {
     async fn execute(&mut self) {}
   }
   ```
   
   Then this loop could look something like
   ```for statement in statements {
     StatementExecutor::new(ctx, print_options).await?;
   }
   ```
   ?
     
   



##########
datafusion-cli/src/exec.rs:
##########
@@ -231,10 +231,24 @@ pub(super) async fn exec_and_print(
         let adjusted =
             
AdjustedPrintOptions::new(print_options.clone()).with_statement(&statement);
 
-        let plan = create_plan(ctx, statement).await?;
+        // Only clone the statement if it's a CreateExternalTable

Review Comment:
   I think then we would have a place to put `retry-region` 



##########
datafusion-cli/src/object_storage.rs:
##########
@@ -32,15 +32,28 @@ use aws_config::BehaviorVersion;
 use aws_credential_types::provider::error::CredentialsError;
 use aws_credential_types::provider::{ProvideCredentials, 
SharedCredentialsProvider};
 use log::debug;
-use object_store::aws::{AmazonS3Builder, AwsCredential};
+use object_store::aws::{AmazonS3Builder, AmazonS3ConfigKey, AwsCredential};
 use object_store::gcp::GoogleCloudStorageBuilder;
 use object_store::http::HttpBuilder;
 use object_store::{ClientOptions, CredentialProvider, ObjectStore};
 use url::Url;
 
+#[cfg(not(test))]
+use object_store::aws::resolve_bucket_region;

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: github-unsubscr...@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to