Skip to content

Conversation

@d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jan 15, 2026

Fixes #3652 by properly allowing byte range requests in the partial decoding pathway of the sharding codec.

The failure was caused by a certain store-like class (defined just for sharding) failing to handle byte range requests.

There are 2 ways to fix this. One leaves a lot of the cruft in the sharding codec intact and just adds modifies some existing methods. But the other approach removes cruft, and re-uses stuff we have already defined elsewhere in the codebase: the Store API. That's what this PR does. Edit -- this PR now keeps the cruft, with the goal of keeping things small and simple.

In a later PR..., ShardingByteGetter and ShardingByteSetter can be removed, and instead the sharding codec creates Store classes as part of its decoding process. When reading a full shard, a _ShardReaderStore is created, which maps string keys (the names of chunks) onto contiguous ranges in a buffer (the shard bytes). When reading a partial shard, the separate chunk byte regions are used to create a MemoryStore.

Using the Store API here means nested sharding just works, and we get an easy way to add things like caching later.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 15, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 15, 2026

Merging this PR will not alter performance

✅ 30 untouched benchmarks


Comparing d-v-b:fix/nested-shard-reads (19e0e0c) with main (4cddcf5)

Open in CodSpeed

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 15, 2026
"array_fixture",
[
ArrayRequest(shape=(128,) * 3, dtype="uint16", order="F"),
ArrayRequest(shape=(127, 128, 129), dtype="uint16", order="F"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by making the shape of the array irregular w.r.t to the chunk shape, we hit the partial decode path, which required for evoking the bug reported in #3652

spath,
data=data,
chunks=(64,) * data.ndim,
compressors=None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting compressors to None here is also required to trigger the partial decode path.

@codecov
Copy link

codecov bot commented Jan 15, 2026

Codecov Report

❌ Patch coverage is 34.61538% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.55%. Comparing base (7eba7f1) to head (e6784d7).

Files with missing lines Patch % Lines
src/zarr/codecs/sharding.py 34.61% 51 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3655      +/-   ##
==========================================
- Coverage   60.88%   60.55%   -0.34%     
==========================================
  Files          86       86              
  Lines       10182    10226      +44     
==========================================
- Hits         6199     6192       -7     
- Misses       3983     4034      +51     
Files with missing lines Coverage Δ
src/zarr/codecs/sharding.py 51.67% <34.61%> (-10.44%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b d-v-b force-pushed the fix/nested-shard-reads branch from 2cfd6d5 to 4379a66 Compare January 15, 2026 22:05
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 15, 2026
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 16, 2026
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.

Nested shading can be generated, but not read back

1 participant