Add as_any() to the ObjectStore to make it able to identify which ObjectStore is using for the related trait object

[yahoNanJing]

Which issue does this PR close?

Closes #3838.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[tustvold]

Now that rust supports trait upcasting you might be able to just make ObjectStore: Any

[alamb]

There appears to be some CI errors.

I think adding a as_any function is probably fine> it would have wider support from older rust versions, and there are many existing examples of doing so

FWIW this seems like either approach is a breaking change (and would require releasing object_store 0.6.0), which is fine, but just something to keep in mind

[alamb]

Thank you @yahoNanJing

[alamb]

👍

[tustvold]

I would like to understand the use-case more before merging this, I'm not a massive fan of breaking encapsulation in this way, it undermines the decorator pattern, and would rather avoid making a breaking change if possible

Edit: turns out trait upcasting is still unstable, the RFC was just accepted. Using direct "inheritance" might have avoided this being a breaking change, as all 'static types automatically implement Any, but I guess this is the only way to support this

[alamb]

I would like to understand the use-case more before merging this,

I wonder if it is something like flushing the cache, or being able to take some different action if the item is not in the cache 🤔

I'm not a massive fan of breaking encapsulation in this way, it undermines the decorator pattern, and would rather avoid making a breaking change if possible

I agree finding some way to avoid as_any might be preferable in most circumstances, but I don't see that as a reason to not add it. If a downstream project wants to structure their code to use as_any I think that is a legitimate choice.

In other words I see no harm in adding this API as it allows significant flexibility even if it is not a preferred pattern for some projects

[yahoNanJing]

Thanks @alamb and @tustvold for reviewing this. The main reason for adding as_any is to identify different cache path for the corresponding ObjectStore. For example, in the apache/datafusion-ballista#645, we will introduce a cache layer with a root cache directory. All of the cache data will be under that root directory with a cache path based on the original path in its original ObjectStore. The cache layer will be able to cache data from different ObjectStores. Suppose ObjectStore A has a path p, while ObjectStore B has the same path p. Then how to cache them both becomes a problem if we are not able to identify different ObjectStores.

[tustvold]

Then how to cache them both becomes a problem if we are not able to identify different ObjectStores.

How does as_any fit into this, the underlying stores don't expose or even know that information? I think you need to use the url that is used to register the store with the ObjectStoreRegistry as a cache prefix?

[yahoNanJing]

I think you need to use the url that is used to register the store with the ObjectStoreRegistry as a cache prefix?

If the ObjectStore exposes the url, then it would be better so that we can even use a different cache prefix for different ObjectStore instances instead of for different ObjectStore types. However, it's better not to expose the url with secrets, especially for S3 or other stores.

At the first stage, I think the cache prefix based on the ObjectStore types is enough since the as_any method is very common for a trait.

[tustvold]

If the object_store exposes the URL

That is my point, the URL is a datafusion concept, the stores don't know the URL

cache prefix based on store types

How would this work if there are say two different S3 buckets registered?

[alamb]

Here is a PR in DataFusion that is perhaps related: apache/datafusion#5543

That is my point, the URL is a datafusion concept, the stores don't know the URL

So perhaps you are saying the caching should be done at a higher level? Like in ObjectStoreRegistry for example? Is this something you considered @yahoNanJing ?

[yahoNanJing]

How would this work if there are say two different S3 buckets registered?

I don't know whether it's common for a cluster to access several S3 buckets. However, it's a common case for a cluster to access one S3 bucket, one HDFS cluster, or other different kinds of storages.

[yahoNanJing]

That is my point, the URL is a datafusion concept, the stores don't know the URL

Thanks for pointing this out. Previously I thought the url is a necessary part of the ObjectStore. Although it's still very awkward to me not to include the url for ObjectStore, I respect your original design.

[yahoNanJing]

So perhaps you are saying the caching should be done at a higher level? Like in ObjectStoreRegistry for example? Is this something you considered @yahoNanJing ?

Thanks @alamb. I still think it should be at the ObjectStore level. I'll change my current implementation to adjust to it. Maybe we need to add a trait to extend the ObjectStore with providing get_url() method, since it's a datafusion specific concept.

[alamb]

Is there some reason we should hold off on merging this PR ?

[tustvold]

I'll change my current implementation to adjust to it. Maybe we need to add a trait to extend the ObjectStore with providing get_url() method, since it's a datafusion specific concept.

I was waiting based on this feedback, I would rather not merge something, especially a breaking change, without a motivating use-case. I had understood apache/datafusion#5543 as removing the need for this

[yahoNanJing]

Now that rust supports trait upcasting you might be able to just make ObjectStore: Any

Hi @tustvold, I'm afraid we still need to add the as_any() interface to the trait. Based on apache/datafusion#5543, I factored previous code. However, it doesn't work as expected. See the UT fails in https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230322/ballista/core/src/cache_layer/object_store/mod.rs#L169-L173

Or maybe there are other correct way to downcast an Arc<dyn ObjectStore>.
https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230322/ballista/core/src/cache_layer/object_store/mod.rs#L31-L39

[tustvold]

However, it doesn't work as expected

I'm having a somewhat hard time understanding the code you've linked but please bear with me:

CachedBasedObjectStoreRegistry:

• Creates a FileCacheObjectStore combining a normal ObjectStore with a FileCacheLayer,
• Computes a cache key from the URL
• Creates a ObjectStoreWithKey to extend the FileCacheObjectStore with this key to wrap this construction

Calling get on this object store then:

• Calls through ObjectStoreWithKey to FileCacheObjectStore
• Looks up the value in the cache, passing the inner store as the GetExtra argument
• This propagates through to FileCacheLoader::load
• Which then calls CacheMedium::get_mapping_location
• Which finally calls through to get_key which attempts to downcast the provided ObjectStore to ObjectStoreWithKey
• This will always fail as the ObjectStoreWithKey is decorating the FileCacheObjectStore not the other way round

A simple fix might be to swap the decoration order, but taking a step back, the cache key is known at the time FileCacheObjectStore is constructed by CachedBasedObjectStoreRegistry. I don't see a reason to not just store this key inside the FileCacheObjectStore and pass it along with the inner ObjectStore as the GetExtra argument to the cache. For example, by storing ObjectStoreWithKey inside FileCacheObjectStore instead of Arc<dyn ObjectStore>, and passing this as the GetExtra argument. This would not only be simpler (and work) but would also avoid any need for downcasting?

FWIW this highlights my major reservation about as_any(), it makes for very hard to follow code, there is a reason Rust discourages its usage by not having first-class dynamic casting support, the friction is a feature 😄. I think if downstreams want to use this pattern they can derive a trait object with as_any and as_object_store, but I would prefer not to "bless" this as a first-party pattern without a really compelling use-case

[yahoNanJing]

Thanks @tustvold for reading the proto branch for cache layer. The process details you described above are exactly correct.

However, it doesn't work as expected

I'm sorry I didn't explain it very clearly. Actually the process works well. And it's totally transparent to the Datafusion. The only unexpected thing is the function get_key in https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230322/ballista/core/src/cache_layer/object_store/mod.rs#L31-L39. It seems it will always return "none". The unit test https://github.com/yahoNanJing/arrow-ballista/blob/dev-20230322/ballista/core/src/cache_layer/object_store/mod.rs#L169-L173 verifies that.

[yahoNanJing]

Calling get on this object store then:
Calls through ObjectStoreWithKey to FileCacheObjectStore
Looks up the value in the cache, passing the inner store as the GetExtra argument
This propagates through to FileCacheLoader::load
Which then calls CacheMedium::get_mapping_location
Which finally calls through to get_key which attempts to downcast the provided ObjectStore to ObjectStoreWithKey
This will always fail as the ObjectStoreWithKey is decorating the FileCacheObjectStore not the other way round

Thanks @tustvold for providing this genius idea. It works now without introducing as_any to the ObjectStore. Related commit is here. yahoNanJing/arrow-ballista@84a98d7

[tustvold]

I am going to close this for now, feel free to reopen should the need arise