You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "clbarnes (via GitHub)" <gi...@apache.org> on 2023/12/18 15:18:39 UTC
[PR] object_store: full HTTP range support [arrow-rs]
clbarnes opened a new pull request, #5222:
URL: https://github.com/apache/arrow-rs/pull/5222
- Support suffix and offset ranges in GetOptions and get_opts
- Ensure that, if a range is requested, the response contains exactly that range
# Which issue does this PR close?
Closes #4611 . Supercedes #5206 .
# Rationale for this change
Motivated by fetching file suffixes without needing 2 requests to find the length first.
# What changes are included in this PR?
- Add utility struct `GetRange` which represents bounded ranges, offsets, and suffixes
- `GetOptions` now stores a `GetRange` rather than a `Range`
- Implementations of `ObjectStore` are updated with these changes
# Are there any user-facing changes?
`GetOptions::range` is now a `crate::utils::GetRange` rather than a `std::ops::Range<usize>`. It implements `From<RangeBounds<usize>>` so this simply means adding `.into()` to current code.
There are some additional error cases which previously went undetected if a server didn't return exactly the requested range.
# Open questions
LocalFileSystem is a special-case store whose `get_opts` returns a whole file which then must be sliced using the `GetResult::range`. Other stores return a response containing only the requested bytes, with the range returned only for informational purposes. There is currently little need for users to deal with this complexity as `get_range(s)` handles it internally, but this PR may guide more users towards having to do so.
There are a bunch of utilities in the `GetRange` impl and associated free functions which seemed like they could be useful but are not currently used, and could be deleted for simplicity.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876757526
> The only query I had was no longer explicitly checking for StartTooLarge in the GetRange::Bounded::as_range
I removed this because given an object of size `x` a range of `x..x` would erroneously fail it.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876782278
> Which would imply that the logic in this PR is still overly strict
True! If we want to allow the EndTooLarge case then we also need to update the `GetRange::as_range` logic to just clamp the last value.
> These were changed because they were resulting in failures of existing tests, running against actual object stores.
What it's running against shouldn't matter, should it? As it just catches a few extra cases where where we have requested the wrong thing. It's just a case of whether we want to allow 0-length requests, and whether we want to allow a range where the first byte is after the end of the resource. Are those tests fuzzed or are we explicitly making such requests in the tests?
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1431295926
##########
object_store/src/local.rs:
##########
@@ -416,9 +419,16 @@ impl ObjectStore for LocalFileSystem {
let meta = convert_metadata(metadata, location)?;
options.check_preconditions(&meta)?;
+ let range = if let Some(r) = options.range {
+ r.as_range(meta.size)
+ .map_err(|e| as_generic_err(STORE, e))?
Review Comment:
The perhaps more "idiomatic" way to do this would be to define a new Error variant for this module, e.g.
```
UnexpectedRange { source: UnexpectedRange }
```
And then this would be `.context(UnexpectedRangeSnafu)`
##########
object_store/Cargo.toml:
##########
@@ -54,6 +54,7 @@ reqwest = { version = "0.11", default-features = false, features = ["rustls-tls-
ring = { version = "0.17", default-features = false, features = ["std"], optional = true }
rustls-pemfile = { version = "1.0", default-features = false, optional = true }
tokio = { version = "1.25.0", features = ["sync", "macros", "rt", "time", "io-util"] }
+http-content-range = "0.1.2"
Review Comment:
We typically try very hard to avoid additional dependencies, especially ones that don't have very obvious long-term maintenance stories (don't have some sort of community backing them). Is this strictly necessary?
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
Review Comment:
See comments below, but I would be inclined to not include this for now
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
+ ranges: &[T],
+ coalesce: usize,
+) -> (Vec<GetRange>, Option<GetRange>) {
+ if ranges.is_empty() {
+ return (vec![], None);
+ }
+ let mut v = Vec::with_capacity(ranges.len());
+ let mut o = None::<usize>;
+
+ for rng in ranges.iter().cloned().map(|r| r.into()) {
+ match rng {
+ GetRange::Suffix(n) => {
+ if let Some(suff) = o {
+ o = Some(suff.max(n));
+ } else {
+ o = Some(n);
+ }
+ }
+ _ => v.push(rng),
+ }
+ }
+
+ let mut ret = Vec::with_capacity(v.len() + 1);
+ let suff = o.map(|s| GetRange::Suffix(s));
+
+ if v.is_empty() {
+ if let Some(s) = suff.as_ref() {
+ ret.push(s.clone());
+ }
+ return (ret, suff);
+ }
+
+ // unwrap is fine because we've already filtered out the suffixes
+ v.sort_by(|a, b| a.partial_cmp(b).unwrap());
+ let mut it = v.into_iter();
+ let mut prev = it.next().unwrap();
+
+ for curr in it {
+ match prev {
+ GetRange::Offset { .. } => {
+ match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr);
+ }
+ }
+
+ let Some(s) = suff else {
+ return (ret, None);
+ };
+
+ if ret.last().unwrap().is_whole().unwrap() {
+ return (ret, None);
+ } else {
+ return (ret, Some(s));
+ }
+ }
+ GetRange::Bounded { .. } => match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr.clone());
+ }
+ },
+ GetRange::Suffix { .. } => unreachable!(),
+ }
+ prev = curr;
+ }
+
+ (ret, suff)
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ GetRange::Bounded(r) => f.write_fmt(format_args!("{}-{}", r.start, r.end - 1)),
+ GetRange::Offset(o) => f.write_fmt(format_args!("{o}-")),
+ GetRange::Suffix(n) => f.write_fmt(format_args!("-{n}")),
+ }
+ }
+}
+
+impl<T: RangeBounds<usize>> From<T> for GetRange {
+ fn from(value: T) -> Self {
+ use std::ops::Bound::*;
+ let first = match value.start_bound() {
+ Included(i) => *i,
+ Excluded(i) => i + 1,
+ Unbounded => 0,
+ };
+ match value.end_bound() {
+ Included(i) => GetRange::Bounded(first..(i + 1)),
+ Excluded(i) => GetRange::Bounded(first..*i),
+ Unbounded => GetRange::Offset(first),
+ }
+ }
+}
+
+/// Takes a function `fetch` that can fetch a range of bytes and uses this to
+/// fetch the provided byte `ranges`
+///
+/// To improve performance it will:
+///
+/// * Combine ranges less than `coalesce` bytes apart into a single call to `fetch`
+/// * Make multiple `fetch` requests in parallel (up to maximum of 10)
+pub async fn coalesce_get_ranges<F, E, Fut>(
Review Comment:
Do we strictly need this, it feels exceedingly niche to be selecting ranges from a file without knowing how large the file is?
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
Review Comment:
Perhaps we could keep things simple and not include all these methods to start with?
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
Review Comment:
Perhaps we could omit these for now, it seems pretty niche to me to be selecting multiple ranges from a file without knowing its size, and the behaviour is somewhat subtle.
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
Review Comment:
See below, I would prefer to not provide this as the behaviour is subtle and the use-cases relatively limited.
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
+ ranges: &[T],
+ coalesce: usize,
+) -> (Vec<GetRange>, Option<GetRange>) {
+ if ranges.is_empty() {
+ return (vec![], None);
+ }
+ let mut v = Vec::with_capacity(ranges.len());
+ let mut o = None::<usize>;
+
+ for rng in ranges.iter().cloned().map(|r| r.into()) {
+ match rng {
+ GetRange::Suffix(n) => {
+ if let Some(suff) = o {
+ o = Some(suff.max(n));
+ } else {
+ o = Some(n);
+ }
+ }
+ _ => v.push(rng),
+ }
+ }
+
+ let mut ret = Vec::with_capacity(v.len() + 1);
+ let suff = o.map(|s| GetRange::Suffix(s));
+
+ if v.is_empty() {
+ if let Some(s) = suff.as_ref() {
+ ret.push(s.clone());
+ }
+ return (ret, suff);
+ }
+
+ // unwrap is fine because we've already filtered out the suffixes
+ v.sort_by(|a, b| a.partial_cmp(b).unwrap());
+ let mut it = v.into_iter();
+ let mut prev = it.next().unwrap();
+
+ for curr in it {
+ match prev {
+ GetRange::Offset { .. } => {
+ match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr);
+ }
+ }
+
+ let Some(s) = suff else {
+ return (ret, None);
+ };
+
+ if ret.last().unwrap().is_whole().unwrap() {
+ return (ret, None);
+ } else {
+ return (ret, Some(s));
+ }
+ }
+ GetRange::Bounded { .. } => match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr.clone());
+ }
+ },
+ GetRange::Suffix { .. } => unreachable!(),
+ }
+ prev = curr;
+ }
+
+ (ret, suff)
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ GetRange::Bounded(r) => f.write_fmt(format_args!("{}-{}", r.start, r.end - 1)),
+ GetRange::Offset(o) => f.write_fmt(format_args!("{o}-")),
+ GetRange::Suffix(n) => f.write_fmt(format_args!("-{n}")),
+ }
+ }
+}
+
+impl<T: RangeBounds<usize>> From<T> for GetRange {
+ fn from(value: T) -> Self {
+ use std::ops::Bound::*;
+ let first = match value.start_bound() {
+ Included(i) => *i,
+ Excluded(i) => i + 1,
+ Unbounded => 0,
+ };
+ match value.end_bound() {
+ Included(i) => GetRange::Bounded(first..(i + 1)),
+ Excluded(i) => GetRange::Bounded(first..*i),
+ Unbounded => GetRange::Offset(first),
+ }
+ }
+}
+
+/// Takes a function `fetch` that can fetch a range of bytes and uses this to
+/// fetch the provided byte `ranges`
+///
+/// To improve performance it will:
+///
+/// * Combine ranges less than `coalesce` bytes apart into a single call to `fetch`
+/// * Make multiple `fetch` requests in parallel (up to maximum of 10)
+pub async fn coalesce_get_ranges<F, E, Fut>(
+ ranges: &[GetRange],
+ fetch: F,
+ coalesce: usize,
+) -> Result<Vec<Bytes>, E>
+where
+ F: Send + FnMut(GetRange) -> Fut,
+ E: Send,
+ Fut: std::future::Future<Output = Result<Bytes, E>> + Send,
+{
+ let (mut fetch_ranges, suff_opt) = merge_get_ranges(ranges, coalesce);
+ if let Some(suff) = suff_opt.as_ref() {
+ fetch_ranges.push(suff.clone());
+ }
+ if fetch_ranges.is_empty() {
+ return Ok(vec![]);
+ }
+
+ let mut fetched: Vec<_> = futures::stream::iter(fetch_ranges.iter().cloned())
+ .map(fetch)
+ .buffered(OBJECT_STORE_COALESCE_PARALLEL)
+ .try_collect()
+ .await?;
+
+ let suff = suff_opt.map(|r| {
+ let nbytes = match r {
+ GetRange::Suffix(n) => n,
+ _ => unreachable!(),
+ };
+ fetch_ranges.pop().unwrap();
+ let b = fetched.pop().unwrap();
+ if nbytes >= b.len() {
+ b
+ } else {
+ b.slice((b.len() - nbytes)..)
+ }
+ });
+
+ Ok(ranges
+ .iter()
+ .map(|range| {
+ match range {
+ GetRange::Suffix(n) => {
+ let b = suff.as_ref().unwrap();
+ let start = b.len().saturating_sub(*n);
+ return b.slice(start..b.len());
+ }
+ _ => (),
+ }
+ // unwrapping range.first_byte() is fine because we've early-returned suffixes
+ let idx = fetch_ranges
+ .partition_point(|v| v.first_byte().unwrap() <= range.first_byte().unwrap())
+ - 1;
+ let fetch_range = &fetch_ranges[idx];
+ let fetch_bytes = &fetched[idx];
+
+ let start = range.first_byte().unwrap() - fetch_range.first_byte().unwrap();
+ let end = range.last_byte().map_or(fetch_bytes.len(), |range_last| {
+ fetch_bytes
+ .len()
+ .max(range_last - fetch_range.first_byte().unwrap() + 1)
+ });
+ fetch_bytes.slice(start..end)
+ })
+ .collect())
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidRangeResponse {
+ #[snafu(display("Response was not PARTIAL_CONTENT; length {length:?}"))]
+ NotPartial { length: Option<usize> },
+ #[snafu(display("Content-Range header not present"))]
+ NoContentRange,
+ #[snafu(display("Content-Range header could not be parsed: {value:?}"))]
+ InvalidContentRange { value: Vec<u8> },
+}
+
+pub(crate) fn response_range(r: &Response) -> Result<Range<usize>, InvalidRangeResponse> {
+ use InvalidRangeResponse::*;
+
+ if r.status() != StatusCode::PARTIAL_CONTENT {
+ return Err(NotPartial {
+ length: r.content_length().map(|s| s as usize),
+ });
+ }
+
+ let val_bytes = r
+ .headers()
+ .get(CONTENT_RANGE)
+ .ok_or(NoContentRange)?
+ .as_bytes();
+
+ match ContentRange::parse_bytes(val_bytes) {
+ ContentRange::Bytes(c) => Ok(c.first_byte as usize..(c.last_byte as usize + 1)),
+ ContentRange::UnboundBytes(c) => Ok(c.first_byte as usize..(c.last_byte as usize + 1)),
+ _ => Err(InvalidContentRange {
+ value: val_bytes.to_vec(),
+ }),
+ }
+}
+
+pub(crate) fn as_generic_err<E: std::error::Error + Send + Sync + 'static>(
Review Comment:
As mentioned above, the more idiomatic way to handle this is mapping via the module local error enumeration
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
+ ranges: &[T],
+ coalesce: usize,
+) -> (Vec<GetRange>, Option<GetRange>) {
+ if ranges.is_empty() {
+ return (vec![], None);
+ }
+ let mut v = Vec::with_capacity(ranges.len());
+ let mut o = None::<usize>;
+
+ for rng in ranges.iter().cloned().map(|r| r.into()) {
+ match rng {
+ GetRange::Suffix(n) => {
+ if let Some(suff) = o {
+ o = Some(suff.max(n));
+ } else {
+ o = Some(n);
+ }
+ }
+ _ => v.push(rng),
+ }
+ }
+
+ let mut ret = Vec::with_capacity(v.len() + 1);
+ let suff = o.map(|s| GetRange::Suffix(s));
+
+ if v.is_empty() {
+ if let Some(s) = suff.as_ref() {
+ ret.push(s.clone());
+ }
+ return (ret, suff);
+ }
+
+ // unwrap is fine because we've already filtered out the suffixes
+ v.sort_by(|a, b| a.partial_cmp(b).unwrap());
+ let mut it = v.into_iter();
+ let mut prev = it.next().unwrap();
+
+ for curr in it {
+ match prev {
+ GetRange::Offset { .. } => {
+ match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr);
+ }
+ }
+
+ let Some(s) = suff else {
+ return (ret, None);
+ };
+
+ if ret.last().unwrap().is_whole().unwrap() {
+ return (ret, None);
+ } else {
+ return (ret, Some(s));
+ }
+ }
+ GetRange::Bounded { .. } => match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr.clone());
+ }
+ },
+ GetRange::Suffix { .. } => unreachable!(),
+ }
+ prev = curr;
+ }
+
+ (ret, suff)
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ GetRange::Bounded(r) => f.write_fmt(format_args!("{}-{}", r.start, r.end - 1)),
+ GetRange::Offset(o) => f.write_fmt(format_args!("{o}-")),
+ GetRange::Suffix(n) => f.write_fmt(format_args!("-{n}")),
+ }
+ }
+}
+
+impl<T: RangeBounds<usize>> From<T> for GetRange {
+ fn from(value: T) -> Self {
+ use std::ops::Bound::*;
+ let first = match value.start_bound() {
+ Included(i) => *i,
+ Excluded(i) => i + 1,
+ Unbounded => 0,
+ };
+ match value.end_bound() {
+ Included(i) => GetRange::Bounded(first..(i + 1)),
+ Excluded(i) => GetRange::Bounded(first..*i),
+ Unbounded => GetRange::Offset(first),
+ }
+ }
+}
+
+/// Takes a function `fetch` that can fetch a range of bytes and uses this to
+/// fetch the provided byte `ranges`
+///
+/// To improve performance it will:
+///
+/// * Combine ranges less than `coalesce` bytes apart into a single call to `fetch`
+/// * Make multiple `fetch` requests in parallel (up to maximum of 10)
+pub async fn coalesce_get_ranges<F, E, Fut>(
+ ranges: &[GetRange],
+ fetch: F,
+ coalesce: usize,
+) -> Result<Vec<Bytes>, E>
+where
+ F: Send + FnMut(GetRange) -> Fut,
+ E: Send,
+ Fut: std::future::Future<Output = Result<Bytes, E>> + Send,
+{
+ let (mut fetch_ranges, suff_opt) = merge_get_ranges(ranges, coalesce);
+ if let Some(suff) = suff_opt.as_ref() {
+ fetch_ranges.push(suff.clone());
+ }
+ if fetch_ranges.is_empty() {
+ return Ok(vec![]);
+ }
+
+ let mut fetched: Vec<_> = futures::stream::iter(fetch_ranges.iter().cloned())
+ .map(fetch)
+ .buffered(OBJECT_STORE_COALESCE_PARALLEL)
+ .try_collect()
+ .await?;
+
+ let suff = suff_opt.map(|r| {
+ let nbytes = match r {
+ GetRange::Suffix(n) => n,
+ _ => unreachable!(),
+ };
+ fetch_ranges.pop().unwrap();
+ let b = fetched.pop().unwrap();
+ if nbytes >= b.len() {
+ b
+ } else {
+ b.slice((b.len() - nbytes)..)
+ }
+ });
+
+ Ok(ranges
+ .iter()
+ .map(|range| {
+ match range {
+ GetRange::Suffix(n) => {
+ let b = suff.as_ref().unwrap();
+ let start = b.len().saturating_sub(*n);
+ return b.slice(start..b.len());
+ }
+ _ => (),
+ }
+ // unwrapping range.first_byte() is fine because we've early-returned suffixes
+ let idx = fetch_ranges
+ .partition_point(|v| v.first_byte().unwrap() <= range.first_byte().unwrap())
+ - 1;
+ let fetch_range = &fetch_ranges[idx];
+ let fetch_bytes = &fetched[idx];
+
+ let start = range.first_byte().unwrap() - fetch_range.first_byte().unwrap();
+ let end = range.last_byte().map_or(fetch_bytes.len(), |range_last| {
+ fetch_bytes
+ .len()
+ .max(range_last - fetch_range.first_byte().unwrap() + 1)
+ });
+ fetch_bytes.slice(start..end)
+ })
+ .collect())
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidRangeResponse {
+ #[snafu(display("Response was not PARTIAL_CONTENT; length {length:?}"))]
+ NotPartial { length: Option<usize> },
+ #[snafu(display("Content-Range header not present"))]
+ NoContentRange,
+ #[snafu(display("Content-Range header could not be parsed: {value:?}"))]
+ InvalidContentRange { value: Vec<u8> },
+}
+
+pub(crate) fn response_range(r: &Response) -> Result<Range<usize>, InvalidRangeResponse> {
+ use InvalidRangeResponse::*;
+
+ if r.status() != StatusCode::PARTIAL_CONTENT {
+ return Err(NotPartial {
+ length: r.content_length().map(|s| s as usize),
+ });
+ }
+
+ let val_bytes = r
+ .headers()
+ .get(CONTENT_RANGE)
+ .ok_or(NoContentRange)?
+ .as_bytes();
+
+ match ContentRange::parse_bytes(val_bytes) {
Review Comment:
The format seems extremely simple - https://www.rfc-editor.org/rfc/rfc9110#field.content-range
I therefore wonder if we can avoid needing another dependency?
Possibly something like (not tested)
```
fn parse_content_range(s: &str) -> Option<Range<usize>> {
let rem = s.strip_prefix("bytes ")?;
let (range, _) = rem.split_once('/')?;
let (start, end) = range.split_once('-')?;
Some((start.try_into().ok()?, end.try_into().ok()?))
}
```
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439558827
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
+ ranges: &[T],
+ coalesce: usize,
+) -> (Vec<GetRange>, Option<GetRange>) {
+ if ranges.is_empty() {
+ return (vec![], None);
+ }
+ let mut v = Vec::with_capacity(ranges.len());
+ let mut o = None::<usize>;
+
+ for rng in ranges.iter().cloned().map(|r| r.into()) {
+ match rng {
+ GetRange::Suffix(n) => {
+ if let Some(suff) = o {
+ o = Some(suff.max(n));
+ } else {
+ o = Some(n);
+ }
+ }
+ _ => v.push(rng),
+ }
+ }
+
+ let mut ret = Vec::with_capacity(v.len() + 1);
+ let suff = o.map(|s| GetRange::Suffix(s));
+
+ if v.is_empty() {
+ if let Some(s) = suff.as_ref() {
+ ret.push(s.clone());
+ }
+ return (ret, suff);
+ }
+
+ // unwrap is fine because we've already filtered out the suffixes
+ v.sort_by(|a, b| a.partial_cmp(b).unwrap());
+ let mut it = v.into_iter();
+ let mut prev = it.next().unwrap();
+
+ for curr in it {
+ match prev {
+ GetRange::Offset { .. } => {
+ match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr);
+ }
+ }
+
+ let Some(s) = suff else {
+ return (ret, None);
+ };
+
+ if ret.last().unwrap().is_whole().unwrap() {
+ return (ret, None);
+ } else {
+ return (ret, Some(s));
+ }
+ }
+ GetRange::Bounded { .. } => match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr.clone());
+ }
+ },
+ GetRange::Suffix { .. } => unreachable!(),
+ }
+ prev = curr;
+ }
+
+ (ret, suff)
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ GetRange::Bounded(r) => f.write_fmt(format_args!("{}-{}", r.start, r.end - 1)),
+ GetRange::Offset(o) => f.write_fmt(format_args!("{o}-")),
+ GetRange::Suffix(n) => f.write_fmt(format_args!("-{n}")),
+ }
+ }
+}
+
+impl<T: RangeBounds<usize>> From<T> for GetRange {
+ fn from(value: T) -> Self {
+ use std::ops::Bound::*;
+ let first = match value.start_bound() {
+ Included(i) => *i,
+ Excluded(i) => i + 1,
+ Unbounded => 0,
+ };
+ match value.end_bound() {
+ Included(i) => GetRange::Bounded(first..(i + 1)),
+ Excluded(i) => GetRange::Bounded(first..*i),
+ Unbounded => GetRange::Offset(first),
+ }
+ }
+}
+
+/// Takes a function `fetch` that can fetch a range of bytes and uses this to
+/// fetch the provided byte `ranges`
+///
+/// To improve performance it will:
+///
+/// * Combine ranges less than `coalesce` bytes apart into a single call to `fetch`
+/// * Make multiple `fetch` requests in parallel (up to maximum of 10)
+pub async fn coalesce_get_ranges<F, E, Fut>(
+ ranges: &[GetRange],
+ fetch: F,
+ coalesce: usize,
+) -> Result<Vec<Bytes>, E>
+where
+ F: Send + FnMut(GetRange) -> Fut,
+ E: Send,
+ Fut: std::future::Future<Output = Result<Bytes, E>> + Send,
+{
+ let (mut fetch_ranges, suff_opt) = merge_get_ranges(ranges, coalesce);
+ if let Some(suff) = suff_opt.as_ref() {
+ fetch_ranges.push(suff.clone());
+ }
+ if fetch_ranges.is_empty() {
+ return Ok(vec![]);
+ }
+
+ let mut fetched: Vec<_> = futures::stream::iter(fetch_ranges.iter().cloned())
+ .map(fetch)
+ .buffered(OBJECT_STORE_COALESCE_PARALLEL)
+ .try_collect()
+ .await?;
+
+ let suff = suff_opt.map(|r| {
+ let nbytes = match r {
+ GetRange::Suffix(n) => n,
+ _ => unreachable!(),
+ };
+ fetch_ranges.pop().unwrap();
+ let b = fetched.pop().unwrap();
+ if nbytes >= b.len() {
+ b
+ } else {
+ b.slice((b.len() - nbytes)..)
+ }
+ });
+
+ Ok(ranges
+ .iter()
+ .map(|range| {
+ match range {
+ GetRange::Suffix(n) => {
+ let b = suff.as_ref().unwrap();
+ let start = b.len().saturating_sub(*n);
+ return b.slice(start..b.len());
+ }
+ _ => (),
+ }
+ // unwrapping range.first_byte() is fine because we've early-returned suffixes
+ let idx = fetch_ranges
+ .partition_point(|v| v.first_byte().unwrap() <= range.first_byte().unwrap())
+ - 1;
+ let fetch_range = &fetch_ranges[idx];
+ let fetch_bytes = &fetched[idx];
+
+ let start = range.first_byte().unwrap() - fetch_range.first_byte().unwrap();
+ let end = range.last_byte().map_or(fetch_bytes.len(), |range_last| {
+ fetch_bytes
+ .len()
+ .max(range_last - fetch_range.first_byte().unwrap() + 1)
+ });
+ fetch_bytes.slice(start..end)
+ })
+ .collect())
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidRangeResponse {
+ #[snafu(display("Response was not PARTIAL_CONTENT; length {length:?}"))]
+ NotPartial { length: Option<usize> },
+ #[snafu(display("Content-Range header not present"))]
+ NoContentRange,
+ #[snafu(display("Content-Range header could not be parsed: {value:?}"))]
+ InvalidContentRange { value: Vec<u8> },
+}
+
+pub(crate) fn response_range(r: &Response) -> Result<Range<usize>, InvalidRangeResponse> {
+ use InvalidRangeResponse::*;
+
+ if r.status() != StatusCode::PARTIAL_CONTENT {
+ return Err(NotPartial {
+ length: r.content_length().map(|s| s as usize),
+ });
+ }
+
+ let val_bytes = r
+ .headers()
+ .get(CONTENT_RANGE)
+ .ok_or(NoContentRange)?
+ .as_bytes();
+
+ match ContentRange::parse_bytes(val_bytes) {
Review Comment:
I would prefer to explicitly enumerate the variants we support and error otherwise
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439557074
##########
object_store/src/local.rs:
##########
@@ -416,9 +419,16 @@ impl ObjectStore for LocalFileSystem {
let meta = convert_metadata(metadata, location)?;
options.check_preconditions(&meta)?;
+ let range = if let Some(r) = options.range {
+ r.as_range(meta.size)
+ .map_err(|e| as_generic_err(STORE, e))?
Review Comment:
Does adding a new variant in this way mean that every downstream user needs to handle their error differently (e.g. when matching error types) even when they're not doing something range-related?
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1441983232
##########
object_store/src/memory.rs:
##########
@@ -225,23 +216,8 @@ impl ObjectStore for InMemory {
let (range, data) = match options.range {
Some(range) => {
- let len = entry.data.len();
- match range {
- Bounded(r) => {
- ensure!(r.end <= len, OutOfRangeSnafu { range: r, len });
- ensure!(r.start <= r.end, BadRangeSnafu { range: r });
- (r.clone(), entry.data.slice(r))
- }
- Offset(o) => {
- ensure!(o < len, OutOfRangeSnafu { range: o..len, len });
- (o..len, entry.data.slice(o..len))
- }
- Suffix(n) => {
- ensure!(n < len, BadSuffixSnafu { nbytes: n });
- let start = len - n;
- (start..len, entry.data.slice(start..len))
- }
- }
+ let r = range.as_range(entry.data.len()).context(RangeSnafu)?;
Review Comment:
It seemed simpler to just have this logic in one place, as opposed to duplicating it
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "joroKr21 (via GitHub)" <gi...@apache.org>.
joroKr21 commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1571070089
##########
object_store/src/util.rs:
##########
@@ -167,6 +173,119 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// Request only a portion of an object's bytes
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use object_store::GetRange;
+/// let range1: GetRange = (50..150).into();
+/// let range2: GetRange = (50..=150).into();
+/// let range3: GetRange = (50..).into();
+/// let range4: GetRange = (..150).into();
+/// ```
+///
+/// Implementations may wish to inspect [`GetResult`] for the exact byte
+/// range returned.
+///
+/// [`GetResult`]: crate::GetResult
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// Request a specific range of bytes
+ ///
+ /// If the given range is zero-length or starts after the end of the object,
+ /// an error will be returned. Additionally, if the range ends after the end
+ /// of the object, the entire remainder of the object will be returned.
+ /// Otherwise, the exact requested range will be returned.
+ Bounded(Range<usize>),
+ /// Request all bytes starting from a given byte offset
+ Offset(usize),
+ /// Request up to the last n bytes
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub(crate) enum InvalidGetRange {
+ #[snafu(display(
+ "Wanted range starting at {requested}, but object was only {length} bytes long"
+ ))]
+ StartTooLarge { requested: usize, length: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ pub(crate) fn is_valid(&self) -> Result<(), InvalidGetRange> {
+ match self {
+ Self::Bounded(r) if r.end <= r.start => {
+ return Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ });
+ }
+ _ => (),
+ };
+ Ok(())
+ }
+
+ /// Convert to a [`Range`] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
Review Comment:
Why not make this public? It looks like this is needed by anyone implementing an `ObjectStore`
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876772516
Interestingly the [spec](https://www.rfc-editor.org/rfc/rfc9110#byte.ranges) has the following to say
> A client can limit the number of bytes requested without knowing the size of the [selected representation](https://www.rfc-editor.org/rfc/rfc9110#selected.representation). If the [last-pos](https://www.rfc-editor.org/rfc/rfc9110#rule.int-range) value is absent, or if the value is greater than or equal to the current length of the representation data, the byte range is interpreted as the remainder of the representation (i.e., the server replaces the value of [last-pos](https://www.rfc-editor.org/rfc/rfc9110#rule.int-range) with a value that is one less than the current length of the selected representation).
Which would imply that the logic in this PR is still overly strict
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876951489
The tests which were failing were all tests written as part of this effort. I think there were a couple of places where the errors' fields were ambiguously named and so some of them were the wrong way round, so I've improved the names and updated the tests for the new, stricter failure cases.
- users cannot request a 0-byte range
- `as_range` should raise an error if the first byte requested is after the end of the resource, but in practice this should never be hit because the server will return a 416 Range Not Satisfiable and that error should bubble up first.
- `as_range` raises an error if the user requests a range ending after the end of the resource, even though the server will 206 Partial Content and return the valid part of the requested range. This is to behave more like rust slicing and let the user know early if they are requesting beyond the length of the resource. The downside is that this disallows users implementing their own paginated requests, like "I want to request 100 bytes at a time and if I get < 100 back I'll stop", but I think this is a pretty rare case which should probably fetch the resource length with a HEAD first.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439568281
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
Review Comment:
I think I would definitely prefer to keep this outside of the object_store library
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876917336
Would it be possible to allow the workflows on this PR to run without approval? As the failing tests are dependent on complex and possibly secret CI setup.
> Which would imply that the logic in this PR is still overly strict
I think it's OK to be more strict than the HTTP spec on this check, because `as_range` is only used to compare that we got the range that we requested, as a user is likely to want to then act on the data as if it were the range that they requested. If they request a 100-byte range, and then the server returns a 50-byte range, the user will want to know about it up front even if it's technically permissible. If the user didn't care how long the response was, they could use an offset. We already treat `get_range` in a rust-y way rather than an HTTP-y way (using a right-exclusive range object); the rust behaviour is to error if the right bound is beyond the end of the indexed object.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874151263
> This method has a default implementation of making 2 requests: one to find the length and one to get the suffix, which is then overridden in all of the stores which directly support suffix requests
As we're now making a breaking change to support this use-case, I think I would prefer us to return a NotImplemented error in such cases. This is consistent with what we do for preconditions, etc...
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1875364449
It would appear this is falling foul of https://github.com/apache/arrow-rs/issues/5272
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877358991
I took the liberty of pushing https://github.com/apache/arrow-rs/pull/5222/commits/e20b130b3578467b696ecdc310c7e498a4257004 which makes some minor copy tweaks and also loosens the suffix restrictions. I could see this being useful for reading something like parquet, where a common pattern is to guess at the length of the metadata footer and request this to avoid additional roundtrips, and would otherwise not be able to make use of the suffix functionality.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877199137
> I think the use-case of "give me up to the first 100 bytes" is a perfectly valid use-case
Got it, I'll see if I can get that done this afternoon.
> Although one could argue the lack of support for zero-length ranges also breaks this 😅
True, although there is no way to express "return a zero-length slice of bytes" in the HTTP spec because it's right-inclusive. We could bodge it with a HEAD request to fill out the `ObjectMeta` and then an empty `Bytes` but that's highly unlikely to be what the user wanted.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877127243
It currently works in that the server returns some bytes, but it doesn't return the range of bytes which the user requested, and then the reported `GetResult::range` is incorrect. So if we want the returned bytes to match the returned `Range`, we need to change something either way. Either
- Raise an error
- Implemented here
- Pro: consistent with cases where a requested range cannot be satisfied, e.g. 416
- Pro: consistent with cases where a user uses a `Range` to slice into a resource that isn't as long as they thought it was
- Pro: matches the the `GetResult::Range` from the current implementation
- Con: maybe the user wanted whatever bytes are there up to a certain maximum, rather than exactly what they requested; throws away those bytes
- Return the bytes from the response and change `GetResult::range` to reflect the actual bytes returned
- Would be a ~1 statement change (plus test updates)
- Pro: doesn't throw away whatever bytes are actually returned
- Pro: the bytes returned matches the current implementation
- Con: users have to find the intersection between what they wanted and what they got every time they request a range
The middle ground would be to return an error, but have it contain the `GetResult` with the actual range. However, this might need to be represented at the top of the `crate::Error` so it could easily be matched on.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439560391
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
Review Comment:
This was written in anticipation of `GetRange`s being used under the hood for all of the range-fetching machinery (as I intend to do that in a separate library); it can certainly be omitted here.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877677979
Yes, this looks good to me! Thank you so much for your patience and willingness to wade into this.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877205283
Thank you, sorry to go back and forth on this, but whenever I've assumed "nobody could possibly rely on this", I'm proven wrong :sweat_smile:
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439558612
##########
object_store/src/local.rs:
##########
@@ -416,9 +419,16 @@ impl ObjectStore for LocalFileSystem {
let meta = convert_metadata(metadata, location)?;
options.check_preconditions(&meta)?;
+ let range = if let Some(r) = options.range {
+ r.as_range(meta.size)
+ .map_err(|e| as_generic_err(STORE, e))?
Review Comment:
No you add it to the modules private error enumeration, which then gets converted to the object_store::Error
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1440394289
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,167 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A bounded byte range.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Convert to a [std::ops::Range<usize>] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Review Comment:
Should this include the `bytes=` part?
##########
object_store/src/util.rs:
##########
@@ -279,66 +274,13 @@ impl<T: RangeBounds<usize>> From<T> for GetRange {
Unbounded => 0,
};
match value.end_bound() {
- Included(i) => GetRange::Bounded(first..(i + 1)),
- Excluded(i) => GetRange::Bounded(first..*i),
- Unbounded => GetRange::Offset(first),
+ Included(i) => Self::Bounded(first..(i + 1)),
+ Excluded(i) => Self::Bounded(first..*i),
+ Unbounded => Self::Offset(first),
}
}
}
-#[derive(Debug, Snafu)]
Review Comment:
This is moved into get.rs (and tweaked) to avoid feature flag issues
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876925781
> Would it be possible to allow the workflows on this PR to run without approval
It doesn't require approvals once you have had code merged, I'm not sure there is a way to turn it off
> If they request a 100-byte range, and then the server returns a 50-byte range, the user will want to know about it up front even if it's technically permissible
So long as this is what it currently does that sounds fine to me
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876749448
Yes, that all makes sense to me. The only query I had was no longer explicitly checking for `StartTooLarge` in the `GetRange::Bounded::as_range` - a correct error will still be thrown (because we check that the end is after the start and that the `end <= len`), it's just marginally less explicit. Happy with the changes either way.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876785116
> What it's running against shouldn't matter, should it?
My point was we shouldn't break things that currently work, regardless of opinions on their relative correctness :smile:
We should therefore establish what exactly the current behaviour is and look to preserve that
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1875983684
@clbarnes Ok I have now gotten this updated to the point where CI passes, could you perhaps take a look and check the changes I've made make sense
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1571091373
##########
object_store/src/util.rs:
##########
@@ -167,6 +173,119 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// Request only a portion of an object's bytes
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use object_store::GetRange;
+/// let range1: GetRange = (50..150).into();
+/// let range2: GetRange = (50..=150).into();
+/// let range3: GetRange = (50..).into();
+/// let range4: GetRange = (..150).into();
+/// ```
+///
+/// Implementations may wish to inspect [`GetResult`] for the exact byte
+/// range returned.
+///
+/// [`GetResult`]: crate::GetResult
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// Request a specific range of bytes
+ ///
+ /// If the given range is zero-length or starts after the end of the object,
+ /// an error will be returned. Additionally, if the range ends after the end
+ /// of the object, the entire remainder of the object will be returned.
+ /// Otherwise, the exact requested range will be returned.
+ Bounded(Range<usize>),
+ /// Request all bytes starting from a given byte offset
+ Offset(usize),
+ /// Request up to the last n bytes
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub(crate) enum InvalidGetRange {
+ #[snafu(display(
+ "Wanted range starting at {requested}, but object was only {length} bytes long"
+ ))]
+ StartTooLarge { requested: usize, length: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ pub(crate) fn is_valid(&self) -> Result<(), InvalidGetRange> {
+ match self {
+ Self::Bounded(r) if r.end <= r.start => {
+ return Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ });
+ }
+ _ => (),
+ };
+ Ok(())
+ }
+
+ /// Convert to a [`Range`] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
Review Comment:
In general we try to keep the public API as small as possible to allow for evolution
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874149627
> Yes, we mentioned azure's lack of support for this in the linked issue. This method has a default implementation of making 2 requests: one to find the length and one to get the suffix, which is then overridden in all of the stores which directly support suffix requests.
Great, seems fine 👍
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1870296692
A quick note: some storage services like `azblob` don't support suffix range like `bytes: -1024`, we need to return a correct error in this case.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1440473373
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,167 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A bounded byte range.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Convert to a [std::ops::Range<usize>] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Review Comment:
I think given the variants represent quantities in bytes, it would make sense to do so
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876776350
> should be a <= because otherwise you're requesting a 0-length range, which is almost certainly not what the user intends.
These were changed because they were resulting in failures of existing tests, running against actual object stores.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1440463056
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,167 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A bounded byte range.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Convert to a [std::ops::Range<usize>] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Review Comment:
Strictly speaking, bytes isn't the only unit allowed in the header, although I am not aware of any servers which implement any other units. I'm happy to include `bytes=`.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874131333
> A quick note: some storage services like `azblob` don't support suffix range like `bytes: -1024`, we need to return a correct error in this case.
Yes, we've azure's lack of support for this in the linked issue. This method has a default implementation of making 2 requests: one to find the length and one to get the suffix, which is then overridden in all of the stores which directly support suffix requests.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874332813
Implemented all requested changes. For azure, I've checked whether a suffix was requested and then failed before sending the request. It would be valid to just send the suffix request, as the response will trigger an error anyway, and then it would immediately start working if/when microsoft pulls their socks up and implements it, but the error wouldn't be as informative and it would be slower.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874202116
> I would prefer us to return a NotImplemented error
Is it also the case with preconditions that it's possible to get that information in an inefficient way, and we choose not to? I'm just thinking about the zarr use case where we want to use object_store so that we can write a library which is generic over the users' stores. If we don't build in the (inefficient) fallback behaviour, then all library authors wanting to make use of generic stores will have to do a lot of re-implementing that exact behaviour with matches on the response and make 3 requests (one to fail, one to get the length, one to actually get the range). An error is the current behaviour (but returned by the server rather than on our side).
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876765449
I think that should fail a `StartTooLarge`? Byte indices are zero-based, so trying to get the byte with index 100 from a resource which is 100 bytes long is indeed starting after the end of the resource. The other off-by-one was a hangover from when I stored the right-inclusive bounds.
I'll suggest another couple of changes along those lines.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877246431
[Relevant xkcd](https://xkcd.com/1172/), of course!
That's now implemented and tested, and the docs for `get_range` have been expanded to note the error cases and how to get the actual output range if you need it.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876953641
> but I think this is a pretty rare case
Does this case currently work, as if it doesn't that would be a pretty compelling argument for "it doesn't matter".
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1431313221
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
Review Comment:
Perhaps we could keep things simple and not include all these methods to start with? I'm not entirely sure of their utility.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1440565211
##########
object_store/src/util.rs:
##########
@@ -167,6 +173,114 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use object_store::GetRange;
+/// let range1: GetRange = (50..150).into();
+/// let range2: GetRange = (50..=150).into();
+/// let range3: GetRange = (50..).into();
+/// let range4: GetRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A bounded byte range.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+#[derive(Debug, Snafu)]
+pub(crate) enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Convert to a [`Range`] if valid.
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::Bounded(r) => f.write_fmt(format_args!("bytes={}-{}", r.start, r.end - 1)),
+ Self::Offset(o) => f.write_fmt(format_args!("bytes={o}-")),
+ Self::Suffix(n) => f.write_fmt(format_args!("bytes=-{n}")),
Review Comment:
```suggestion
Self::Bounded(r) => write!(f, "bytes={}-{}", r.start, r.end - 1),
Self::Offset(o) => write!(f, "bytes={o}-"),
Self::Suffix(n) => write!(f, "bytes=-{n}"),
```
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on code in PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#discussion_r1439521309
##########
object_store/src/util.rs:
##########
@@ -167,6 +178,501 @@ fn merge_ranges(ranges: &[std::ops::Range<usize>], coalesce: usize) -> Vec<std::
ret
}
+/// A single range in a `Range` request.
+///
+/// These can be created from [usize] ranges, like
+///
+/// ```rust
+/// # use byteranges::request::HttpRange;
+/// let range1: HttpRange = (50..150).into();
+/// let range2: HttpRange = (50..=150).into();
+/// let range3: HttpRange = (50..).into();
+/// let range4: HttpRange = (..150).into();
+/// ```
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum GetRange {
+ /// A range with a given first and last bytes.
+ Bounded(Range<usize>),
+ /// A range defined only by the first byte requested (requests all remaining bytes).
+ Offset(usize),
+ /// A range defined as the number of bytes at the end of the resource.
+ Suffix(usize),
+}
+
+impl PartialOrd for GetRange {
+ fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+ use std::cmp::Ordering::*;
+ use GetRange::*;
+ // `Suffix`es cannot be compared with `Range`s or `Offset`s.
+ // `Range`s and `Offset`s are compared by their first byte,
+ // using the last byte as a tiebreaker (`Offset`s always greater than `Range`s).
+ match self {
+ Bounded(r1) => match other {
+ Bounded(r2) => match r1.start.cmp(&r2.start) {
+ Equal => Some(r1.end.cmp(&r2.end)),
+ o => Some(o),
+ },
+ Offset(f2) => match r1.start.cmp(f2) {
+ Equal => Some(Less),
+ o => Some(o),
+ },
+ Suffix { .. } => None,
+ },
+ Offset(f1) => match other {
+ Bounded(r2) => match f1.cmp(&r2.start) {
+ Equal => Some(Greater),
+ o => Some(o),
+ },
+ Offset(f2) => Some(f1.cmp(f2)),
+ Suffix { .. } => None,
+ },
+ Suffix(b1) => match other {
+ Suffix(b2) => Some(b2.cmp(b1)),
+ _ => None,
+ },
+ }
+ }
+}
+
+#[derive(Debug, Snafu)]
+pub enum RangeMergeError {
+ #[snafu(display("Ranges could not be merged because exactly one was a suffix"))]
+ DifferentTypes,
+ #[snafu(display("Ranges could not be merged because they were too far apart"))]
+ Spread,
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidGetRange {
+ #[snafu(display("Wanted suffix with {expected}B, resource was {actual}B long"))]
+ SuffixTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range starting at {expected}, resource was {actual}B long"))]
+ StartTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Wanted range ending at {expected}, resource was {actual}B long"))]
+ EndTooLarge { expected: usize, actual: usize },
+
+ #[snafu(display("Range started at {start} and ended at {end}"))]
+ Inconsistent { start: usize, end: usize },
+}
+
+impl GetRange {
+ /// Create a range representing the whole resource.
+ pub fn new_whole() -> Self {
+ Self::Offset(0)
+ }
+
+ /// Whether this is an offset.
+ pub fn is_offset(&self) -> bool {
+ match self {
+ GetRange::Offset(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is a range.
+ pub fn is_range(&self) -> bool {
+ match self {
+ GetRange::Bounded(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether this is an offset.
+ pub fn is_suffix(&self) -> bool {
+ match self {
+ GetRange::Suffix(_) => true,
+ _ => false,
+ }
+ }
+
+ /// Whether the range has no bytes in it (i.e. the last byte is before the first).
+ ///
+ /// [None] if the range is an `Offset` or `Suffix`.
+ /// The response may still be empty.
+ pub fn is_empty(&self) -> Option<bool> {
+ match self {
+ GetRange::Bounded(r) => Some(r.is_empty()),
+ _ => None,
+ }
+ }
+
+ /// Whether the range represents the entire resource (i.e. it is an `Offset` of 0).
+ ///
+ /// [None] if the range is a `Range` or `Suffix`.
+ /// The response may still be the full resource.
+ pub fn is_whole(&self) -> Option<bool> {
+ match self {
+ GetRange::Offset(first) => Some(first == &0),
+ _ => None,
+ }
+ }
+
+ /// How many bytes the range is requesting.
+ ///
+ /// Note that the server may respond with a different number of bytes,
+ /// depending on the length of the resource and other behaviour.
+ pub fn nbytes(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.end.saturating_sub(r.start)),
+ GetRange::Offset(_) => None,
+ GetRange::Suffix(n) => Some(*n),
+ }
+ }
+
+ /// The index of the first byte requested ([None] for suffix).
+ pub fn first_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => Some(r.start),
+ GetRange::Offset(o) => Some(*o),
+ GetRange::Suffix(_) => None,
+ }
+ }
+
+ /// The index of the last byte requested ([None] for offset or suffix).
+ pub fn last_byte(&self) -> Option<usize> {
+ match self {
+ GetRange::Bounded(r) => match r.end {
+ 0 => None,
+ n => Some(n),
+ },
+ GetRange::Offset { .. } => None,
+ GetRange::Suffix { .. } => None,
+ }
+ }
+
+ pub(crate) fn as_range(&self, len: usize) -> Result<Range<usize>, InvalidGetRange> {
+ match self {
+ Self::Bounded(r) => {
+ if r.start >= len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: r.start,
+ actual: len,
+ })
+ } else if r.end <= r.start {
+ Err(InvalidGetRange::Inconsistent {
+ start: r.start,
+ end: r.end,
+ })
+ } else if r.end >= len {
+ Err(InvalidGetRange::EndTooLarge {
+ expected: r.end,
+ actual: len,
+ })
+ } else {
+ Ok(r.clone())
+ }
+ }
+ Self::Offset(o) => {
+ if o >= &len {
+ Err(InvalidGetRange::StartTooLarge {
+ expected: *o,
+ actual: len,
+ })
+ } else {
+ Ok(*o..len)
+ }
+ }
+ Self::Suffix(n) => {
+ len.checked_sub(*n)
+ .map(|start| start..len)
+ .ok_or(InvalidGetRange::SuffixTooLarge {
+ expected: *n,
+ actual: len,
+ })
+ }
+ }
+ }
+
+ /// Merge two ranges which fall within a certain distance `coalesce` of each other.
+ ///
+ /// Error if exactly one is a suffix or if the ranges are too far apart.
+ pub(crate) fn try_merge(
+ &self,
+ other: &GetRange,
+ coalesce: usize,
+ ) -> Result<Self, RangeMergeError> {
+ use GetRange::*;
+
+ let (g1, g2) = match self.partial_cmp(other) {
+ None => {
+ // One is a suffix, the other isn't.
+ // This is escapable if one represents the whole resource.
+ if let Some(whole) = self.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ if let Some(whole) = other.is_whole() {
+ if whole {
+ return Ok(GetRange::new_whole());
+ }
+ }
+ return Err(RangeMergeError::DifferentTypes);
+ }
+ Some(o) => match o {
+ std::cmp::Ordering::Greater => (other, self),
+ _ => (self, other),
+ },
+ };
+
+ match g1 {
+ Bounded(r1) => match g2 {
+ Bounded(r2) => {
+ if r2.start <= r1.end.saturating_add(coalesce) {
+ Ok(GetRange::Bounded(r1.start..r1.end.max(r2.end)))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Offset(first) => {
+ if first < &(r1.end.saturating_add(coalesce)) {
+ Ok(GetRange::Offset(r1.start))
+ } else {
+ Err(RangeMergeError::Spread)
+ }
+ }
+ Suffix { .. } => unreachable!(),
+ },
+ // Either an offset or suffix, both of which would contain the second range.
+ _ => Ok(g1.clone()),
+ }
+ }
+
+ pub fn matches_range(&self, range: Range<usize>, len: usize) -> bool {
+ match self {
+ Self::Bounded(r) => r == &range,
+ Self::Offset(o) => o == &range.start && len == range.end,
+ Self::Suffix(n) => range.end == len && range.start == len - n,
+ }
+ }
+}
+
+/// Returns a sorted [Vec] of [HttpRange::Offset] and [HttpRange::Range] that cover `ranges`,
+/// and a single [HttpRange::Suffix] if one or more are given.
+/// The suffix is also omitted if any of the ranges is the whole resource (`0-`).
+///
+/// The suffix is returned separately as it may still overlap with the other ranges,
+/// so the caller may want to handle it differently.
+pub fn merge_get_ranges<T: Into<GetRange> + Clone>(
+ ranges: &[T],
+ coalesce: usize,
+) -> (Vec<GetRange>, Option<GetRange>) {
+ if ranges.is_empty() {
+ return (vec![], None);
+ }
+ let mut v = Vec::with_capacity(ranges.len());
+ let mut o = None::<usize>;
+
+ for rng in ranges.iter().cloned().map(|r| r.into()) {
+ match rng {
+ GetRange::Suffix(n) => {
+ if let Some(suff) = o {
+ o = Some(suff.max(n));
+ } else {
+ o = Some(n);
+ }
+ }
+ _ => v.push(rng),
+ }
+ }
+
+ let mut ret = Vec::with_capacity(v.len() + 1);
+ let suff = o.map(|s| GetRange::Suffix(s));
+
+ if v.is_empty() {
+ if let Some(s) = suff.as_ref() {
+ ret.push(s.clone());
+ }
+ return (ret, suff);
+ }
+
+ // unwrap is fine because we've already filtered out the suffixes
+ v.sort_by(|a, b| a.partial_cmp(b).unwrap());
+ let mut it = v.into_iter();
+ let mut prev = it.next().unwrap();
+
+ for curr in it {
+ match prev {
+ GetRange::Offset { .. } => {
+ match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr);
+ }
+ }
+
+ let Some(s) = suff else {
+ return (ret, None);
+ };
+
+ if ret.last().unwrap().is_whole().unwrap() {
+ return (ret, None);
+ } else {
+ return (ret, Some(s));
+ }
+ }
+ GetRange::Bounded { .. } => match prev.try_merge(&curr, coalesce) {
+ Ok(r) => ret.push(r),
+ Err(_) => {
+ ret.push(prev);
+ ret.push(curr.clone());
+ }
+ },
+ GetRange::Suffix { .. } => unreachable!(),
+ }
+ prev = curr;
+ }
+
+ (ret, suff)
+}
+
+impl Display for GetRange {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ GetRange::Bounded(r) => f.write_fmt(format_args!("{}-{}", r.start, r.end - 1)),
+ GetRange::Offset(o) => f.write_fmt(format_args!("{o}-")),
+ GetRange::Suffix(n) => f.write_fmt(format_args!("-{n}")),
+ }
+ }
+}
+
+impl<T: RangeBounds<usize>> From<T> for GetRange {
+ fn from(value: T) -> Self {
+ use std::ops::Bound::*;
+ let first = match value.start_bound() {
+ Included(i) => *i,
+ Excluded(i) => i + 1,
+ Unbounded => 0,
+ };
+ match value.end_bound() {
+ Included(i) => GetRange::Bounded(first..(i + 1)),
+ Excluded(i) => GetRange::Bounded(first..*i),
+ Unbounded => GetRange::Offset(first),
+ }
+ }
+}
+
+/// Takes a function `fetch` that can fetch a range of bytes and uses this to
+/// fetch the provided byte `ranges`
+///
+/// To improve performance it will:
+///
+/// * Combine ranges less than `coalesce` bytes apart into a single call to `fetch`
+/// * Make multiple `fetch` requests in parallel (up to maximum of 10)
+pub async fn coalesce_get_ranges<F, E, Fut>(
+ ranges: &[GetRange],
+ fetch: F,
+ coalesce: usize,
+) -> Result<Vec<Bytes>, E>
+where
+ F: Send + FnMut(GetRange) -> Fut,
+ E: Send,
+ Fut: std::future::Future<Output = Result<Bytes, E>> + Send,
+{
+ let (mut fetch_ranges, suff_opt) = merge_get_ranges(ranges, coalesce);
+ if let Some(suff) = suff_opt.as_ref() {
+ fetch_ranges.push(suff.clone());
+ }
+ if fetch_ranges.is_empty() {
+ return Ok(vec![]);
+ }
+
+ let mut fetched: Vec<_> = futures::stream::iter(fetch_ranges.iter().cloned())
+ .map(fetch)
+ .buffered(OBJECT_STORE_COALESCE_PARALLEL)
+ .try_collect()
+ .await?;
+
+ let suff = suff_opt.map(|r| {
+ let nbytes = match r {
+ GetRange::Suffix(n) => n,
+ _ => unreachable!(),
+ };
+ fetch_ranges.pop().unwrap();
+ let b = fetched.pop().unwrap();
+ if nbytes >= b.len() {
+ b
+ } else {
+ b.slice((b.len() - nbytes)..)
+ }
+ });
+
+ Ok(ranges
+ .iter()
+ .map(|range| {
+ match range {
+ GetRange::Suffix(n) => {
+ let b = suff.as_ref().unwrap();
+ let start = b.len().saturating_sub(*n);
+ return b.slice(start..b.len());
+ }
+ _ => (),
+ }
+ // unwrapping range.first_byte() is fine because we've early-returned suffixes
+ let idx = fetch_ranges
+ .partition_point(|v| v.first_byte().unwrap() <= range.first_byte().unwrap())
+ - 1;
+ let fetch_range = &fetch_ranges[idx];
+ let fetch_bytes = &fetched[idx];
+
+ let start = range.first_byte().unwrap() - fetch_range.first_byte().unwrap();
+ let end = range.last_byte().map_or(fetch_bytes.len(), |range_last| {
+ fetch_bytes
+ .len()
+ .max(range_last - fetch_range.first_byte().unwrap() + 1)
+ });
+ fetch_bytes.slice(start..end)
+ })
+ .collect())
+}
+
+#[derive(Debug, Snafu)]
+pub enum InvalidRangeResponse {
+ #[snafu(display("Response was not PARTIAL_CONTENT; length {length:?}"))]
+ NotPartial { length: Option<usize> },
+ #[snafu(display("Content-Range header not present"))]
+ NoContentRange,
+ #[snafu(display("Content-Range header could not be parsed: {value:?}"))]
+ InvalidContentRange { value: Vec<u8> },
+}
+
+pub(crate) fn response_range(r: &Response) -> Result<Range<usize>, InvalidRangeResponse> {
+ use InvalidRangeResponse::*;
+
+ if r.status() != StatusCode::PARTIAL_CONTENT {
+ return Err(NotPartial {
+ length: r.content_length().map(|s| s as usize),
+ });
+ }
+
+ let val_bytes = r
+ .headers()
+ .get(CONTENT_RANGE)
+ .ok_or(NoContentRange)?
+ .as_bytes();
+
+ match ContentRange::parse_bytes(val_bytes) {
Review Comment:
A well-formed content range header is very simple. However, strictly headers can contain basically anything they want and there's no guarantee it is well-formed, so I chose to push the complexity down into the external lib. We can have the logic internally and just raise a very broad error otherwise.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874211029
> If we don't build in the (inefficient) fallback behaviour, then all library authors wanting to make use of generic stores will have to do a lot of re-implementing
The aim of this crate is to be faithful to the APIs exposed by the object stores themselves, and I therefore think returning an error for something that isn't supported by the store is therefore the correct thing to do. Yes workloads that wish to use suffix ranges against any store will have to do more work to support this, and will need to ascertain how it is they wish to achieve this, even if the solution is just to not use suffix ranges and to ensure they always have the object sizes on hand.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1874343384
> It would be valid to just send the suffix request, as the response will trigger an error anyway, and then it would immediately start working if/when microsoft pulls their socks up and implements it, but the error wouldn't be as informative and it would be slower.
Makes sense, I suspect we would need to update the API version anyway for this to work if they added support for this.
> Implemented all requested changes
It is EOD for me here, but I'll take a look first thing tomorrow
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876774694
Ah, because I'm the author it doesn't let me suggest changes, it just puts them right in.
```
Self::Bounded(r) => {
if r.end < r.start {
```
should be a `<=` because otherwise you're requesting a 0-length range, which is almost certainly not what the user intends.
```
Self::Offset(o) => {
if *o > len {
```
should also be a `>=` because the index of the last byte is `len-1`, so the byte at index `len` is also after the end of the resource.
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "clbarnes (via GitHub)" <gi...@apache.org>.
clbarnes commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1876793078
I think both should result in a 416 RangeNotSatisfiable, because in one the offset exceeds the resource length, and in the other a `std::ops::Range` of `10..10` turns into a header of `Range: bytes=10..9`, which should also not be satisfiable. I can look into this - thanks for all your help pushing this over the line!
--
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
Re: [PR] object_store: full HTTP range support [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5222:
URL: https://github.com/apache/arrow-rs/pull/5222#issuecomment-1877162891
I think the use-case of "give me up to the first 100 bytes" is a perfectly valid use-case, and is even called out in the HTTP spec. I therefore don't think we can/should break this if it is currently supported.
> consistent with cases where a user uses a Range to slice into a resource
Although one could argue the lack of support for zero-length ranges also breaks this :sweat_smile:
> users have to find the intersection between what they wanted and what they got every time they request a range
Only if they don't know the size of the object they are requesting data from, otherwise the behaviour is no different from master? Sure an API could return less, much like it could also return gibberish, at some point you just have to trust that the servers implement the spec correctly
--
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