apache / opendal

Apache OpenDAL: access data freely.
https://opendal.apache.org
Apache License 2.0
3.23k stars 449 forks source link

idea: move logic from `fn metadata(&self) -> Arc<AccessorInfo>` to `impl<A: Access> Layer<A> for CompleteLayer` #4888

Open Lzzzzzt opened 2 months ago

Lzzzzzt commented 2 months ago

Feature Description

Since the CompleteAccessor stores the metadata, and Access::info return the Arc<AccessorInfo>, we can move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer to avoid creating a new Arc<AccessorInfo>(this is not add the reference count).

Problem and Solution

just change CompleteAccessor::metadata:

fn metadata(&self) -> Arc<AccessorInfo> {
    let mut meta = (*self.meta).clone();
    let cap = meta.full_capability_mut();
    if cap.list && cap.write_can_empty {
        cap.create_dir = true;
    }
    meta.into()
}

to

fn metadata(&self) -> Arc<AccessorInfo> {
   self.meta.clone()
}

and change CompleteLayer::layer:

fn layer(&self, inner: A) -> Self::LayeredAccess {
    CompleteAccessor {
        meta: inner.info(),
        inner: Arc::new(inner),
    }
}

to

fn layer(&self, inner: A) -> Self::LayeredAccess {
    let mut meta = inner.info().as_ref().clone();
    let cap = meta.full_capability_mut();

    if cap.list && cap.write_can_empty {
        cap.create_dir = true;
    }

    CompleteAccessor {
        meta: meta.into(),
        inner: Arc::new(inner),
    }
}

Additional Context

All the layers that store the metadata can apply this change.

Are you willing to contribute to the development of this feature?

Lzzzzzt commented 2 months ago

should these accessor like BlockingAccessor that doesn't store the metadata but change the capability add a field to store the new metadata to avoid needless memory allocation.

https://github.com/apache/opendal/blob/31d09673093d92a87e72575d1b00bdd4529dcf56/core/src/layers/blocking.rs#L183-L187

Xuanwo commented 2 months ago

Looks good to me, we can construct the info during build time.

Lzzzzzt commented 2 months ago

It seems that after apply this change, the c binding CI test will fail, but I don't know why this happens.