Repository navigation
fix(sharding): reject a stored shard too short to hold its index on every path - #4475
Merged
d-v-b merged 4 commits intoOct 3, 2026
Merged
Conversation
…, not truthiness `_ShardReader` defines `__len__`, so `shard_reader or create_empty(...)` replaced a loaded zero-chunk shard with an empty reader; the outcome was equivalent but the test meant "was a shard loaded". Likewise `get_chunk_slice` returns `tuple[int, int] | None`, and `_load_full_shard_maybe` relied on `Buffer.__len__` to treat a missing and a zero-length value alike; that conflation is now spelled out. No behaviour changes. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…very path A zero-length shard was read as missing by the async full-shard read and partial write, and failed the index checksum on every other path. The index decoder now checks the index length first, so every read and partial-write path, under either pipeline, raises the same ValueError. zarrs and tensorstore also reject such a shard: https://github.com/zarrs/zarrs/blob/b8ad3ab45082b7d55ac0dd653594953a9d9e6e7d/zarrs/src/array/codec/array_to_bytes/sharding/sharding_codec.rs#L1273 https://github.com/google/tensorstore/blob/f47412a006d7b3da888f3dbb7fc001d1ac6553e1/tensorstore/kvstore/zarr3_sharding_indexed/shard_format.cc#L113 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d-v-b
marked this pull request as ready for review
October 3, 2026 16:43
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4475 +/- ##
=======================================
Coverage 94.69% 94.69%
=======================================
Files 94 94
Lines 13597 13604 +7
=======================================
+ Hits 12875 12882 +7
Misses 722 722
🚀 New features to boost your workflow:
|
Contributor
Author
|
doc build failure is due to rate limiting from other docs inventories, not due to this change. self-merging |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a bugfix that ensures that the two codec pipelines do the same thing (raise an exception) when writing to a shard that's too smol. Full shard writes still clobber an existing shard, no matter its length
🤖 AI text below 🤖
A stored shard too short to hold its shard index, including a zero-length value, now raises a
ValueErrorsaying the shard is truncated or corrupt, on every read and partial-write path under either codec pipeline. Previously the asynchronous full-shard read and partial write treated a zero-length shard as missing (reading the fill value, or overwriting it), while every other path failed with a checksum mismatch or a decode error. zarrs and tensorstore also reject such a shard.Changes
refactor(sharding): tests shard readers and chunk byte slices withis Noneinstead of truthiness._ShardReaderdefines__len__, soshard_reader or _ShardReader.create_empty(...)replaced a loaded zero-chunk shard with an empty reader. The outcome was the same, but the check meant "was a shard loaded". This commit changes no behaviour.fix(sharding): adds_check_shard_index_size, which runs before the shard index is decoded. Every read and partial-write path decodes the index, so this one check covers all of them._load_full_shard_maybeno longer treats a zero-length value as missing.zarr-python never writes a shard shorter than its index: a shard with no chunks is deleted instead. A short value comes only from a truncated or interrupted write, an empty object created by some other tool, or another writer.
zarrs and tensorstore both reject a shard shorter than its index:
sharding_codec.rs#L1273shard_format.cc#L113Tests
test_sharding_truncated_shard_raiseschecks the error for both pipelines, memory and local stores, both index locations, zero-length and 3-byte shards, and full read, partial read and partial write: 48 cases. All 48 fail without the fix. With it,tests/test_codecs,tests/test_array.pyandtests/test_indexing.pypass (3857 passed, 770 skipped, 4 xfailed).Bookkeeping
Mirrored from fork PR d-v-b#388. The changelog fragment is named after this PR.
🤖 Generated with Claude Code