Return BoxStream with 'static lifetime from ObjectStore::list
#6619
Conversation
|
In developmentseed/obstore#35 I verified that this does enable streaming Screen.Recording.2024-10-23.at.12.25.19.PM.mov |
tustvold
added
api-change
|
Looks like @tustvold had a conversation on #6587 so I defer to him. However, I had a quick look at the code and it looks pretty good to me. Thank you @kylebarron |
| delimiter: bool, | ||
| offset: Option<&Path>, | ||
| ) -> BoxStream<'_, Result<ListResult>>; | ||
| ) -> BoxStream<'static, Result<ListResult>>; |
There was a problem hiding this comment.
what is the implications of this stream having static lifetime? I don't understand the implications on downstream crates
There was a problem hiding this comment.
For my use case, Python bindings with pyo3 don't allow you to have any lifetime parameters shorter than 'static: https://pyo3.rs/v0.22.5/class.html#no-lifetime-parameters
There was a problem hiding this comment.
Right -- I guess I am wondering does this effectively mean that the returned streams can't be borrowed (aka they effectively have to be Arc'd or something like that where the stream is owned 🤔 )
Basically I am trying to think about what usecases, if any, that this additional constrait would be break (I realize some people may need to update the type signatures, I am thinking of more fundamental changes)
One thing maybe we could do is try to update DataFusion (and our internal INfluxDB code) with this change and see what, if anything, breaks for us 🤔
There was a problem hiding this comment.
This would only effect implementers of the trait, its a stronger contract on what the implementation chooses to return. The result is consumers of object_store shouldn't notice much
|
This change makes sense to me, and is definitely something we should consider when looking to make a breaking releas. However, I think we want to get a few more such changes lined up before taking the plunge, object_store breaking changes are quite disruptive |
|
For whatever it is worth, I was fighting with the lifetimes of
|
|
main is open for breaking changes for inclusion in 0.12.0 I merged up from main to resolve a conflict |
|
For anyone else following along, the practical impact of this change is that implementations of The rationale is that anything with references can't be sent safely across threads, etc |
…pache#6619) Co-authored-by: Andrew Lamb <[email protected]>
…pache#6619) Co-authored-by: Andrew Lamb <[email protected]>
…pache#6619) Co-authored-by: Andrew Lamb <[email protected]>
…pache#6619) Co-authored-by: Andrew Lamb <[email protected]>
3 participants
To preface: not expecting this to be merged imminently, as it may be a while before object-store's next breaking release, and I figure there may be a cleaner way to change this. But it was useful to learn a bit more about streams, and I'll probably use this patch in my Python-facing object-store bindings.
Which issue does this PR close?
Closes #6587.
Rationale for this change
Easier wrapping of
ObjectStore::listin bindings, e.g. to Python, that don't allow lifetime references.What changes are included in this PR?
Change the return type of
ObjectStore::listandObjectStore::list_with_offsettoAre there any user-facing changes?
Yes.