crepererum commented on code in PR #7160:
URL: https://github.com/apache/arrow-rs/pull/7160#discussion_r1965230168
##########
object_store/src/extensions.rs:
##########
@@ -0,0 +1,257 @@
+use std::{
+ any::{Any, TypeId},
+ collections::HashMap,
+ sync::Arc,
+};
+
+/// Trait that must be implemented by extensions.
+pub trait Extension: std::fmt::Debug + Send + Sync {
Review Comment:
> Hi, I'm concerned about adding such an API in `object_store`, especially a
model tailored for specific use cases like metrics and tracing.
What exactly does concern you?
> Have we considered adding a `TracingObjectStore` that wraps another object
store and adds traces for every API call? I think this approach is more
extensible and can help avoid making changes for every action.
That assumes that you have one single `ObjectStore`. Truth is, `ObjectStore`
is an abstract interface that lends itself to dynamic composition, e.g. to add
caching as wrappers.
##########
object_store/src/extensions.rs:
##########
@@ -0,0 +1,252 @@
+use std::{
+ any::{Any, TypeId},
+ collections::HashMap,
+ sync::Arc,
+};
+
+/// Trait that must be implemented by extensions.
+pub trait Extension: std::fmt::Debug + Send + Sync {
+ /// Return a &Any for this type.
+ fn as_any(self: Arc<Self>) -> Ext;
+
+ /// Ensure that [`Extensions`] can implement [`std::cmp::PartialEq`] by
requiring [`Extension`]
+ /// implementors to implement a dyn-compatible partial equality operation.
+ ///
+ /// This is necessary because [`std::cmp::PartialEq`] uses a `Self` type
parameter, which
+ /// violates dyn-compatibility rules:
+ ///
https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing
+ fn partial_eq(self: Arc<Self>, other: Ext) -> bool;
+}
+
+type ExtensionMap = HashMap<TypeId, Arc<dyn Extension>>;
+type Ext = Arc<dyn Any + Send + Sync + 'static>;
+
+/// Type that holds opaque instances of types referenced by their
[`std::any::TypeId`].
+///
+/// Types are stored as instances of the [`Extension`] trait in order to
enable implementation of
+/// [`std::fmt::Debug`] and [`std::cmp::PartialEq`].
+#[derive(Debug, Default, Clone)]
+pub struct Extensions {
+ inner: ExtensionMap,
+}
+
+impl PartialEq for Extensions {
+ fn eq(&self, other: &Self) -> bool {
+ for (k, v) in &self.inner {
+ if let Some(ov) = other.inner.get(&k) {
+ if !v.clone().partial_eq(ov.clone().as_any()) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ true
+ }
+}
+
+impl Extensions {
+ pub(crate) fn set_ext<T: Extension + 'static>(&mut self, t: T) {
+ let id = t.type_id();
+ let a = Arc::new(t);
+ self.inner.insert(id, a);
+ }
+
+ pub(crate) fn get_ext<T: Any + Send + Sync + 'static>(&self) ->
Option<Arc<T>> {
+ let id = TypeId::of::<T>();
+ self.inner.get(&id).map(|e| {
+ let v = Arc::clone(&e).as_any();
+ Arc::downcast::<T>(v).unwrap()
+ })
+ }
+}
+
+impl From<HashMap<TypeId, Ext>> for Extensions {
+ fn from(other: HashMap<TypeId, Ext>) -> Self {
+ let mut s = Self::default();
+ for (k, v) in other {
+ let v = Arc::new(ExtWrapper { inner: v });
+ s.inner.insert(k, v);
+ }
+ s
+ }
+}
+
+/// Type that implements [`Extension`] for the sake of converting to
[`Extensions`] from external
+/// instances of `HashMap<TypeId, Ext>'.
+///
+/// NOTE: Different instances of the same type held by ExtWrapper are
considered equal for the sake
+/// of the `Extensions.partial_eq` trait since its intended use case is for
wrapping dyn-compatible
+/// trait objects; because [`std::cmp::PartialEq`] has `Self` as a generic
type parameter, the type
+/// system won't let us set it as a trait bound on incoming trait objects when
constructing
+/// ExtWrapper.
+struct ExtWrapper {
+ inner: Arc<dyn Any + Send + Sync + 'static>,
+}
+
+impl std::fmt::Debug for ExtWrapper {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "ExtWrapper2")
+ }
+}
+
+impl Extension for ExtWrapper {
+ fn as_any(self: Arc<Self>) -> Ext {
+ self.inner.clone()
+ }
+
+ fn partial_eq(self: Arc<Self>, _: Ext) -> bool {
+ // this is necessary because ExtWrapper is a generic impl of
Extensions necessary for
+ // converting from external Arc<dyn Any ...> types where none of the
trait bounts can
+ // implement PartialEq due to dyn-compatibility requirements.
+ true
Review Comment:
I think this wrapper is flawed. People should implement the trait themselves
or -- if they cannot due to the orphan rule -- create a dedicated wrapper type
for whatever they need.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
