[GitHub] [arrow-rs] alamb commented on a diff in pull request #4351: Fix ObjectStore::get_range for GetResult::File (#4350)

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

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


##########
object_store/src/lib.rs:
##########
@@ -359,10 +359,16 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static {
     /// in the given byte range
     async fn get_range(&self, location: &Path, range: Range<usize>) -> Result<Bytes> {
         let options = GetOptions {
-            range: Some(range),
+            range: Some(range.clone()),
             ..Default::default()
         };
-        self.get_opts(location, options).await?.bytes().await
+        match self.get_opts(location, options).await? {

Review Comment:
   This doesn't seem quite right to me -- the issue is if `GetResult::File` returns a file in the first place, because `GetResult::File` doesn't have a way to represent a range of a file.
   
   I think there are two better/cleaner ways:
   1. Add an (optional) range to `GetResult::File`
   2. Update `LocalFileSystem::get_opts` to to not return `GetResult::File` if there is a range and instead return a stream
   
   I think 1 is cleaner but I think it would be an API change and thus require releasing object_store 0.7
   
   



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