[GitHub] [arrow-rs] alamb commented on a diff in pull request #4212: Add ObjectStore::get_opts (#2241)

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212#discussion_r1193128952


##########
object_store/src/lib.rs:
##########
@@ -558,6 +575,62 @@ pub struct ObjectMeta {
     pub e_tag: Option<String>,
 }
 
+/// Options for a get request
+#[derive(Debug, Default)]
+pub struct GetOptions {
+    /// Request will succeed if the ETag matches

Review Comment:
   Could you please define `ETag` somewhere? I am not familiar with the term and I think a summary would be nicer UX than a IETF RFC link)
   
   Also could we summarize explicitly  what happens if the option is set bit the request doesn't match (e.g "request will _only_ succeed" (if that is indeed the case))



##########
object_store/src/aws/mod.rs:
##########
@@ -246,39 +247,26 @@ impl ObjectStore for AmazonS3 {
             .await
     }
 
-    async fn get(&self, location: &Path) -> Result<GetResult> {
-        let response = self.client.get_request(location, None, false).await?;
+    async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> {
+        let response = self.client.get_request(location, options, false).await?;
         let stream = response
             .bytes_stream()
             .map_err(|source| crate::Error::Generic {
-                store: "S3",
+                store: STORE,
                 source: Box::new(source),
             })
             .boxed();
 
         Ok(GetResult::Stream(stream))
     }
 
-    async fn get_range(&self, location: &Path, range: Range<usize>) -> Result<Bytes> {

Review Comment:
   Where did this go?



##########
object_store/src/lib.rs:
##########
@@ -346,11 +346,24 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static {
     }
 
     /// Return the bytes that are stored at the specified location.
-    async fn get(&self, location: &Path) -> Result<GetResult>;
+    async fn get(&self, location: &Path) -> Result<GetResult> {
+        self.get_opts(location, GetOptions::default()).await
+    }
+
+    /// Perform a get request with options
+    ///
+    /// Note: options.range will be ignored if [`GetResult::File`]
+    async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult>;

Review Comment:
   I think this is a breaking API change and thus we will need to make it a major version number upgrade -- any existing `impl ObjectStore` will stop compiling. I see you have marked the API as such 👍 



##########
object_store/src/lib.rs:
##########
@@ -558,6 +575,62 @@ pub struct ObjectMeta {
     pub e_tag: Option<String>,
 }
 
+/// Options for a get request

Review Comment:
   I think it would help if we mentioned "for example, request range"



-- 
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