You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pdeva (via GitHub)" <gi...@apache.org> on 2026/01/16 19:47:25 UTC
[PR] Support for conditional deletes [arrow-rs-object-store]
pdeva opened a new pull request, #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #298 .
Supersedes the now stale PR #427
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-6225089272
superceded by https://github.com/apache/arrow-rs-object-store/pull/864
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#discussion_r2700259626
##########
src/http/mod.rs:
##########
@@ -138,6 +138,14 @@ impl ObjectStore for HttpStore {
self.client.get_opts(location, options).await
}
+ async fn delete_opts(&self, location: &Path, opts: DeleteOptions) -> Result<()> {
+ let _ = (location, opts);
+ Err(crate::Error::NotImplemented {
Review Comment:
This seems to be strictly worse than the default impl?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-3876764204
I should have considered this in #549. Reading the [Azure docs](https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch?tabs=microsoft-entra-id#request-body) though, I think etag matching is also supported by the bulk request variant. So I think we could design an API like this:
```rust
pub struct DeleteLocation {
pub path: Path,
pub if_match: Option<String>,
}
#[derive(Debug, Clone, Default)]
pub struct DeleteOptions {
pub extensions: Extensions,
}
trait ObjectStore {
fn delete_stream_opts(
&self,
locations: BoxStream<'static, Result<DeleteLocation>>,
opts: DeleteOptions,
) -> BoxStream<'static, Result<Path>>;
}
// move `delete_stream` to `ObjectStoreExt`
```
i.e. have the extensions ONCE but have per-location matchers.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#discussion_r2836047428
##########
src/lib.rs:
##########
@@ -1877,6 +1927,88 @@ pub struct PutResult {
pub version: Option<String>,
}
+/// Options for a delete request
+#[derive(Debug, Clone, Default)]
+pub struct DeleteOptions {
+ /// Delete will succeed if the `ObjectMeta::e_tag` matches
+ /// otherwise returning [`Error::Precondition`]
+ ///
+ /// See <https://datatracker.ietf.org/doc/html/rfc9110#name-if-match>
+ ///
+ /// Examples:
+ ///
+ /// ```text
+ /// If-Match: "xyzzy"
+ /// If-Match: "xyzzy", "r2d2xxxx", "c3piozzzz"
+ /// If-Match: *
+ /// ```
+ pub if_match: Option<String>,
Review Comment:
I think this will need to use something similar to UpdateVersion found in PutMode as some stores don't support If-Match on mutating methods
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-3938549554
Coming back to this, I think having a separate delete_opts makes sense and has symmetry with get_opts and get_ranges, where we have a unary method with a full set of functionality, and a more limited batch version.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tomsanbear (via GitHub)" <gi...@apache.org>.
tomsanbear commented on PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-4276938715
@pdeva — rebased onto current main and addressed tustvold's three review comments. Branch: [tomsanbear/arrow-rs-object-store:feat/conditional-deletes](https://github.com/tomsanbear/arrow-rs-object-store/tree/feat/conditional-deletes) (three commits on top of your two).
The core change swaps `DeleteOptions.if_match: Option<String>` for `condition: Option<UpdateVersion>` to match `PutMode::Update(UpdateVersion)`. I also dropped the http override (the default trait impl handles both branches strictly better for that backend) and switched gcp to `x-goog-if-generation-match` via the existing `VERSION_MATCH` constant.
Couple of calls worth flagging since they diverge from your original direction:
I dropped the `*` wildcard and multi-etag test cases. Neither maps cleanly onto `UpdateVersion`'s `{ e_tag, version }` shape without reintroducing string parsing into a structured type. If wildcard or multi-etag matters for downstream use I'd rather add them as a separate field (or a small `match_mode` enum) than overload `UpdateVersion` — but for the scope of tustvold's three comments, keeping the type tight was cleaner.
I also kept AWS and Azure on the single-object DELETE-with-`If-Match` path you originally wrote rather than routing through the batch endpoints. S3 does support per-entry `<ETag>` in the `DeleteObjects` XML body and Azure's blob batch supports per-sub-request `If-Match`, so the batch path would work — but your single-object path is the cleaner unary contract and lines up with tustvold's framing of `delete_opts` as the unary counterpart to `delete_stream`. No reason to churn it.
`delete_opts_race_condition` stays defined but is now only wired for AWS/Azure/GCS, not Memory/Local. The "exactly one winner" invariant requires server-side atomic precondition evaluation; Memory and Local do check-then-delete non-atomically, so the test was passing by luck. Making Memory atomic is a reasonable follow-up but out of scope here.
Integration tests now use `UpdateVersion::from(put_result)` so each backend picks the field it needs — GCS reads `.version`, everything else reads `.e_tag`. fmt / test / clippy green locally; integration tests still pending a CI run.
Let me know if you want to pull these onto your branch and keep driving, or if you'd rather I open a follow-up PR superseding #610 with you as co-author. Either works. Holding off on the new PR until you respond.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-3762143904
Tagging @crepererum as this sort of overlaps with https://github.com/apache/arrow-rs-object-store/pull/549
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #610:
URL: https://github.com/apache/arrow-rs-object-store/pull/610#discussion_r2836047984
##########
src/gcp/client.rs:
##########
@@ -560,8 +560,17 @@ impl GoogleCloudStorageClient {
}
/// Perform a delete request <https://cloud.google.com/storage/docs/xml-api/delete-object>
- pub(crate) async fn delete_request(&self, path: &Path) -> Result<()> {
- self.request(Method::DELETE, path).send().await?;
+ pub(crate) async fn delete_request(&self, path: &Path, opts: DeleteOptions) -> Result<()> {
+ let mut builder = self
+ .request(Method::DELETE, path)
+ .with_extensions(opts.extensions)
+ .idempotent(true);
+
+ if let Some(if_match) = &opts.if_match {
+ builder = builder.header(&HeaderName::from_static("if-match"), if_match);
Review Comment:
Have you tested this?
The docs would suggest If-Match is only supported on GET and HEAD
https://docs.cloud.google.com/storage/docs/xml-api/reference-headers#ifmatch
This is why PutMode has UpdateVersion
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for conditional deletes [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #610: Support for conditional deletes
URL: https://github.com/apache/arrow-rs-object-store/pull/610
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org