peasee commented on code in PR #517:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/517#discussion_r2457491728


##########
src/lib.rs:
##########
@@ -646,10 +646,19 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + 
Debug + 'static {
     ///
     /// See [`GetRange::Bounded`] for more details on how `range` gets 
interpreted
     async fn get_range(&self, location: &Path, range: Range<u64>) -> 
Result<Bytes> {
-        let options = GetOptions {
-            range: Some(range.into()),
-            ..Default::default()
-        };
+        let options = GetOptions::new().with_range(range);
+        self.get_opts(location, options).await?.bytes().await
+    }
+
+    /// Return the bytes that are stored at the specified location
+    /// in the given byte range with options.
+    /// 
+    /// The `options` provided will be augmented with the specified `range`.
+    /// Any existing `range` in `options` will be overridden.
+    ///
+    /// See [`GetRange::Bounded`] for more details on how `range` gets 
interpreted
+    async fn get_range_opts(&self, location: &Path, range: Range<u64>, 
options: GetOptions) -> Result<Bytes> {

Review Comment:
   I think that is fair. My main point of improvement was `get_ranges`, because 
it hides away the call over the list of ranges you provide. If you want to get 
a variety of ranges against a single set of options, that would simplify it for 
you.
   
   I read the linked issue for cleaning up `ObjectStore` though, and I agree 
these extra `_opts` functions can be removed.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to