feat!: use `'static` lifetime for `delete_stream` by linhr · Pull Request #524 · apache/arrow-rs-object-store

linhr · 2025-10-30T10:29:26Z

Which issue does this PR close?

Closes #518.

Rationale for this change

The current delete_stream method looks like this:

    fn delete_stream<'a>(
        &'a self,
        locations: BoxStream<'a, Result<Path>>,
    ) -> BoxStream<'a, Result<Path>>;

This PR changes the delete_stream method to work with 'static lifetime. The reason for this change was discussed in the corresponding issue #518.

What changes are included in this PR?

As I work on this PR, the changes appeared to be more complex than thought. The challenge is that delete_stream has a default implementation that takes &self, so the returned stream cannot be 'static. I've considered the following options for this challenge.

Option 1: Only change the input stream to be 'static without changing the lifetime of the output stream. The method would look like this:

    fn delete_stream<'a>(
        &'a self,
        locations: BoxStream<'static, Result<Path>>,
    ) -> BoxStream<'a, Result<Path>>;

Or equivalently (with unnecessary lifetime elided):

    fn delete_stream(
        &self,
        locations: BoxStream<'static, Result<Path>>,
    ) -> BoxStream<'_, Result<Path>>;

This would result in minimum code change but the result is still not very elegant, since we're not fully migrating to BoxStream<'static, _>.

Option 2: Change both the input and output stream to be 'static and requires the implementation to define delete_stream explicitly (i.e. no more default implementation). The method would look like this:

    fn delete_stream(
        &self,
        locations: BoxStream<'static, Result<Path>>,
    ) -> BoxStream<'static, Result<Path>>;

I ended up choosing Option 2 for this PR. This is more work but gives a clean end result so that all ObjectStore methods now handle input/output BoxStream of 'static lifetime.

And fortunately, it likely won't be challenging for downstream implementations even if delete_stream is now required. Most implementations already use Arc for internal states, which can be cheaply cloned and moved into the 'static output stream. From the changes I made to various ObjectStore implementations in this PR, we can see that the logic is straightforward. And I've documented the pattern in lib.rs for downstream implementations to follow.

Are there any user-facing changes?

This is a breaking change that could be considered for 0.13 (#367).

linhr · 2025-10-30T10:36:27Z

The changes are covered by existing tests since delete_stream is used in src/integration.rs.

alamb

Thank you @linhr -- I went through this PR carefully and I think it makes sense to me. 🙏

I have some suggestions to improve some doc comments, but I don't think that is needed before merge

alamb · 2025-10-30T14:58:37Z

+    /// let client = Arc::clone(&self.client);
+    /// locations
+    ///     .map(move |location| {
+    ///         let client = Arc::clone(&client);


alamb · 2025-10-30T14:59:36Z

+    /// following implementation deletes each object with up to 10 concurrent
+    /// requests:
+    ///
+    /// ```ignore


Could you please make this an actual example that compiles (so we are sure it won't drift over time)?

Perhaps following the model f the examples below.

We can do this as a follow on PR

alamb · 2025-10-30T15:00:51Z

-    /// than one object per a request. The default implementation will call
-    /// the single object delete method for each location, but with up to 10
-    /// concurrent requests.
+    /// than one object per a request. Otherwise, the implementation may call


We should probably call this out as a behavior change too in the API docs (but I think it is better to avoid implementing concurrent requests by default)

Also I think the documentation to explain how to get the old behavior is good

Done!

I called out the behavior change in the docs and mentioned that the example can be used to get the previous behavior.

alamb · 2025-10-30T15:06:13Z

        self.inner.delete(location).await
    }

+    fn delete_stream(


I wonder if we should add a test for this new implementation, following the other tests at this file

linhr · 2025-10-31T02:31:29Z

Thanks @alamb for the review! I feel I can update the docs and tests in the same PR. I'll work on this today!

kylebarron · 2025-10-31T15:10:15Z

@@ -873,9 +873,8 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static {
    /// Delete all the objects at the specified locations
    ///
    /// When supported, this method will use bulk operations that delete more


It would be nice as an end user to know which backends (at least of those provided by object-store) support bulk delete

Filed Add documentation for what backends support bulk delete (ObjectStore::delete_stream) #527

alamb

Thank you for this very nice contrbution @linhr

alamb · 2025-10-31T20:30:51Z

+    /// that deletes each object with up to 10 concurrent requests. This default
+    /// behavior has been removed, and each implementation must now provide its
+    /// own `delete_stream` implementation explicitly. The following example
+    /// shows how to implement `delete_stream` to get the previous default


alamb · 2025-10-31T20:31:17Z

Thanks for the review @kylebarron

feat!: use 'static lifetime for delete_stream

a786249

linhr mentioned this pull request Oct 30, 2025

Use 'static lifetime for delete_stream #518

Closed

alamb approved these changes Oct 30, 2025

View reviewed changes

Address comments

5c2770e

linhr requested a review from alamb October 31, 2025 07:50

kylebarron approved these changes Oct 31, 2025

View reviewed changes

alamb mentioned this pull request Oct 31, 2025

Add documentation for what backends support bulk delete (ObjectStore::delete_stream) #527

Closed

alamb approved these changes Oct 31, 2025

View reviewed changes

alamb merged commit e0721f8 into apache:main Oct 31, 2025
8 checks passed

alamb added the api-change label Oct 31, 2025

linhr deleted the delete-stream-static branch November 2, 2025 03:46

Uh oh!

Conversation

linhr commented Oct 30, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

linhr commented Oct 30, 2025

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linhr commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb left a comment •

edited

Loading