Skip to content

Implement ZipStore delete via archive rewrite (fixes #828)#4085

Open
Akash-t25 wants to merge 3 commits into
zarr-developers:mainfrom
Akash-t25:fix/828-zipstore-delitem
Open

Implement ZipStore delete via archive rewrite (fixes #828)#4085
Akash-t25 wants to merge 3 commits into
zarr-developers:mainfrom
Akash-t25:fix/828-zipstore-delitem

Conversation

@Akash-t25

Copy link
Copy Markdown

Summary

Implements delete() and delete_dir() for ZipStore, resolving #828.

ZIP files don't support in-place deletion, so this PR uses an archive-rewrite strategy: copy every surviving member into a fresh temporary ZIP, drop the deleted entries, then atomically swap it in using os.replace.

Changes

src/zarr/storage/_zip.py

  • Flipped supports_deletes: bool = FalseTrue
  • Added _rewrite_without(should_delete) private helper that rewrites the archive atomically
  • Rewrote delete(key) — removes the key (missing key = no-op) instead of raising NotImplementedError
  • Rewrote delete_dir(prefix) — single archive rewrite dropping everything under the prefix

tests/test_store/test_zip.py

  • Updated test_api_integration — chunk deletion and del root["bar"] now succeed
  • Added test_store_supports_deletes, test_delete_compacts_duplicates, test_delete_then_set
  • Inherited StoreTests.test_delete, test_delete_dir, test_delete_nonexistent_key_does_not_raise now run and pass

tests/test_codecs/test_sharding.py

  • Added filterwarnings("ignore:Duplicate name") marker for the now-active zip case

changes/828.feature.md

  • Changelog entry in towncrier format

Test Results

  • test_zip.py: 74 passed, 5 skipped
  • test_sharding.py (zip): 49 passed
  • test_core.py + test_api.py (zip): 28 passed, 3 skipped

Note: *_sync delete tests are skipped — ZipStore does not implement SupportsDeleteSync, which is out of scope for this PR.

Checklist

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.md
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Closes #828

@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 34.37500% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.31%. Comparing base (4e79f1b) to head (0535ab7).

Files with missing lines Patch % Lines
src/zarr/storage/_zip.py 34.37% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4085      +/-   ##
==========================================
- Coverage   93.47%   93.31%   -0.17%     
==========================================
  Files          90       90              
  Lines       11967    11994      +27     
==========================================
+ Hits        11186    11192       +6     
- Misses        781      802      +21     
Files with missing lines Coverage Δ
src/zarr/storage/_zip.py 87.62% <34.37%> (-10.10%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…#828)

Add tests for the auto-open branches in delete()/delete_dir(), the
prefix-normalization branch in delete_dir(), and the temp-file cleanup
path in _rewrite_without() (both when the temp exists and when it is
already gone), bringing patch coverage to 100%.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@d-v-b

d-v-b commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

I worry that this will have counter-intuitive performance characteristics, especially since we don't have a transactional API that would support batching many deletes into one atomic archive resurrection. IF we want to support this delete implementation, it should probably be exposed via runtime store configuration.

cc @mkitti

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implementing ZipStore's __delitem__ via overwrite

2 participants