Skip to content

fix(fused codec pipeline): bulk shard decode must only engage for identity full reads - #4203

Merged
d-v-b merged 1 commit into
zarr-developers:mainfrom
d-v-b:fix/bulk-shard-decode-identity-gate
Jul 29, 2026
Merged

fix(fused codec pipeline): bulk shard decode must only engage for identity full reads#4203
d-v-b merged 1 commit into
zarr-developers:mainfrom
d-v-b:fix/bulk-shard-decode-identity-gate

Conversation

@d-v-b

@d-v-b d-v-b commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This PR ensures that we more correctly enter a bulk-decode fast path, avoiding bulk-decodes where inappropriate, which led to data corruption.

based on a claude-authored PR here: d-v-b#256

Summary

[Describe what this PR changes and why, in your own words.]

For reviewers

[What would you most value a second look at? What are you already confident in? For a refactor, say whether behavior is meant to be unchanged.]

Author attestation

  • I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

TODO

  • 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)

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.91%. Comparing base (123268a) to head (42241ba).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4203   +/-   ##
=======================================
  Coverage   93.90%   93.91%           
=======================================
  Files          91       91           
  Lines       12672    12680    +8     
=======================================
+ Hits        11900    11908    +8     
  Misses        772      772           
Files with missing lines Coverage Δ
src/zarr/codecs/sharding.py 96.17% <100.00%> (+0.04%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…x density check

The FusedCodecPipeline's vectorized whole-shard decode accepted any
indexer without `sel_shape` whose output shape matched the shard shape.
An OrthogonalIndexer from `arr[perm, :]` / `arr.oindex[...]` satisfies
that, so reordering or duplicating fancy-index reads on uncompressed,
crc-free sharded arrays silently returned the shard in natural order.
The bulk path now requires an identity full read: one whole-dimension,
step-1 slice per dimension (structural, not a BasicIndexer type check,
since `arr[:]` arrives as an OrthogonalIndexer).

Also:
- decline structured dtypes in the bulk path, which lacks the Struct
  byte-order branch of BytesCodec._decode_sync (latent until zarr-developers#3054 is
  fixed);
- `_ShardIndex.is_dense` now requires offsets to exactly tile the data
  section instead of merely being unique, so corrupt indexes with
  overlapping or out-of-range offsets (e.g. pointing into an
  index_location='start' index region) cannot be served as array data.

Assisted-by: ClaudeCode:claude-fable-5
@d-v-b
d-v-b force-pushed the fix/bulk-shard-decode-identity-gate branch from 2287a0b to 42241ba Compare July 29, 2026 14:32
@d-v-b
d-v-b marked this pull request as ready for review July 29, 2026 14:37
@d-v-b

d-v-b commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

self-merging as this is scoped to the fused codec pipeline and it's a data corruption risk

@d-v-b
d-v-b merged commit bf81831 into zarr-developers:main Jul 29, 2026
30 checks passed
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.

1 participant