alamb commented on code in PR #517:
URL:
https://github.com/apache/arrow-rs-object-store/pull/517#discussion_r2457061156
##########
src/lib.rs:
##########
@@ -1035,6 +1052,84 @@ impl GetOptions {
}
Ok(())
}
+
+ /// Create a new [`GetOptions`]
Review Comment:
these are great improvements
##########
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:
Why we would add a new method when you can already pass in a `range` on the
`GetOptions`? I think this will actually make the developer experience worse
(as now a developer has to figure out which one of the methods to call)
If we want to make DX better I suggest:
1. Adding more examples to `get_opts` and `get_range` that shows how to call
them with options / a range
2. Eventually deprecate / remove `get_range` entirely (in favor of just
`get_opts`
--
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]
