kevinjqliu commented on code in PR #732: URL: https://github.com/apache/arrow-rs-object-store/pull/732#discussion_r3391675126
########## src/capabilities.rs: ########## @@ -0,0 +1,174 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Capability advertisement for [`ObjectStore`](crate::ObjectStore) implementations. +//! +//! See [`Capabilities`] and [`Capability`] for details. +use crate::Error; +use std::collections::HashSet; + +const ORDERED_LISTING: &str = "ordered-listing"; + +/// An individual capability that an [`ObjectStore`] implementation may support. +/// +/// Used together with [`Capabilities`] to advertise optional backend features. +/// Get the set of supported capabilities via [`ObjectStore::capabilities`]. +#[derive(Hash, Eq, PartialEq, Copy, Clone, Debug)] +#[non_exhaustive] +pub enum Capability { + /// List results from [`ObjectStore::list`] and + /// [`ObjectStore::list_with_offset`] are returned in ascending + /// lexicographic order by [`Path`]. + /// + /// When this capability is present, callers may rely on the ordering and + /// avoid buffering all results solely for sorting purposes. + OrderedListing, +} + +impl std::str::FromStr for Capability { + type Err = Error; + + /// Parses a capability from its kebab-case string representation. Review Comment: nit: thoughts on kebab-case vs snake_case? Most of the repo’s user-facing config strings appear to use `snake_case`, including `aws_capabilities`. Since `Capabilities` is now parsed from config values as well, `aws_capabilities=ordered-listing` mixes a `snake_case` config key with a `kebab-case` capability value. I think `aws_capabilities=ordered_listing` would be more consistent with the rest of the config surface. If we prefer `kebab-case`, maybe we should document that capability names intentionally use kebab-case and possibly accept both `ordered-listing` and `ordered_listing` to avoid bikeshedding / future compatibility. ########## src/capabilities.rs: ########## @@ -0,0 +1,174 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Capability advertisement for [`ObjectStore`](crate::ObjectStore) implementations. +//! +//! See [`Capabilities`] and [`Capability`] for details. +use crate::Error; +use std::collections::HashSet; + +const ORDERED_LISTING: &str = "ordered-listing"; + +/// An individual capability that an [`ObjectStore`] implementation may support. +/// +/// Used together with [`Capabilities`] to advertise optional backend features. +/// Get the set of supported capabilities via [`ObjectStore::capabilities`]. +#[derive(Hash, Eq, PartialEq, Copy, Clone, Debug)] +#[non_exhaustive] +pub enum Capability { + /// List results from [`ObjectStore::list`] and + /// [`ObjectStore::list_with_offset`] are returned in ascending + /// lexicographic order by [`Path`]. + /// + /// When this capability is present, callers may rely on the ordering and + /// avoid buffering all results solely for sorting purposes. + OrderedListing, +} + +impl std::str::FromStr for Capability { + type Err = Error; + + /// Parses a capability from its kebab-case string representation. + /// + /// Returns `None` if `s` does not correspond to any known capability. + fn from_str(s: &str) -> Result<Self, Self::Err> { + match s { + ORDERED_LISTING => Ok(Self::OrderedListing), + cap => Err(Error::Generic { + store: "Config", + source: format!("invalid capability: {cap}").into(), + }), + } Review Comment: nit: align behavior with doc string or vice versa, one says returns None the other throws ########## src/capabilities.rs: ########## @@ -0,0 +1,174 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Capability advertisement for [`ObjectStore`](crate::ObjectStore) implementations. +//! +//! See [`Capabilities`] and [`Capability`] for details. +use crate::Error; +use std::collections::HashSet; + +const ORDERED_LISTING: &str = "ordered-listing"; + +/// An individual capability that an [`ObjectStore`] implementation may support. +/// +/// Used together with [`Capabilities`] to advertise optional backend features. +/// Get the set of supported capabilities via [`ObjectStore::capabilities`]. +#[derive(Hash, Eq, PartialEq, Copy, Clone, Debug)] +#[non_exhaustive] +pub enum Capability { + /// List results from [`ObjectStore::list`] and + /// [`ObjectStore::list_with_offset`] are returned in ascending + /// lexicographic order by [`Path`]. + /// + /// When this capability is present, callers may rely on the ordering and + /// avoid buffering all results solely for sorting purposes. + OrderedListing, +} + +impl std::str::FromStr for Capability { + type Err = Error; + + /// Parses a capability from its kebab-case string representation. + /// + /// Returns `None` if `s` does not correspond to any known capability. + fn from_str(s: &str) -> Result<Self, Self::Err> { + match s { + ORDERED_LISTING => Ok(Self::OrderedListing), + cap => Err(Error::Generic { + store: "Config", + source: format!("invalid capability: {cap}").into(), + }), + } + } +} + +impl std::fmt::Display for Capability { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::OrderedListing => write!(f, "{}", ORDERED_LISTING), + } + } +} + +/// Optional features supported by an [`ObjectStore`] implementation. +/// +/// Get the capabilities of a store by calling [`ObjectStore::capabilities`]. +/// Check whether [`Capability`] is supported by calling [`Capabilities::has`] method. +/// +/// The struct is `#[non_exhaustive]` so that new capability flags can be added +/// in future versions without breaking existing code. +/// +/// # Example +/// +/// ``` +/// # use object_store::{ObjectStore, memory::InMemory, Capability}; +/// let store = InMemory::new(); +/// if store.capabilities().has(Capability::OrderedListing) { +/// println!("list() results are in lexicographic order — no need to sort"); +/// } +/// ``` +#[derive(Debug, PartialEq, Clone, Default)] +pub struct Capabilities { + supported: HashSet<Capability>, +} + +impl Capabilities { + /// Create a [`Capabilities`] from an explicit list of supported [`Capability`] values. + /// + /// Any capability not included in `capabilities` is considered unsupported. + /// + /// # Example + /// + /// ``` + /// # use object_store::{Capabilities, Capability}; + /// let caps = Capabilities::new([Capability::OrderedListing]); + /// assert!(caps.has(Capability::OrderedListing)); + /// ``` + pub fn new(capabilities: impl IntoIterator<Item = Capability>) -> Self { + Self { + supported: capabilities.into_iter().collect(), + } + } + + /// Returns `true` if the given [`Capability`] is supported by this store. + pub fn has(&self, capability: Capability) -> bool { + self.supported.contains(&capability) + } +} + +impl std::str::FromStr for Capabilities { + type Err = Error; + + /// Parses a comma-separated list of capability names into a [`Capabilities`]. + fn from_str(s: &str) -> crate::Result<Self> { + let mut capabilities: Vec<Capability> = Vec::new(); + for mut cap in s.split(',') { + cap = cap.trim(); + if cap.is_empty() { + continue; + } + capabilities.push(cap.parse::<Capability>()?); + } + Ok(Self::new(capabilities)) + } +} + +impl std::fmt::Display for Capabilities { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut iter = self.supported.iter(); + if let Some(cap) = iter.next() { + write!(f, "{}", cap)?; + } + for cap in iter { + write!(f, ", {}", cap)?; + } + Ok(()) + } +} Review Comment: nit: Capabilities stores capabilities in a HashSet, and Display iterates over self.supported.iter(). That is fine with one capability today, but once more capabilities are added, the string form could become non-deterministic. Since this is now exposed through config round-tripping like get_config_value, I’d make the output stable now. ``` let mut caps: Vec<_> = self.supported.iter().collect(); caps.sort_by_key(|cap| cap.to_string()); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
