Skip to content

fix(sharding): flatten any multi-dim value for coordinate selections in partial writes - #4316

Merged
d-v-b merged 6 commits into
zarr-developers:mainfrom
d-v-b:fix/sharding-oindex-mixed-int-arrays
Sep 4, 2026
Merged

fix(sharding): flatten any multi-dim value for coordinate selections in partial writes#4316
d-v-b merged 6 commits into
zarr-developers:mainfrom
d-v-b:fix/sharding-oindex-mixed-int-arrays

Conversation

@d-v-b

@d-v-b d-v-b commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix a dumb singleton dimension bug that breaks oindex with mixed np.array / integer components with the sharding codec.

Closes #4315

Claude wrote this, see the original PR here: d-v-b#320

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)

d-v-b and others added 3 commits September 4, 2026 11:08
…in partial writes

The guard added in zarr-developers#4284 only reshaped the value when its shape equalled
the re-derived CoordinateIndexer's sel_shape. An orthogonal selection that
mixes an integer index with two or more array indices defeats that:
OrthogonalIndexer drops the integer axis from the value but np.ix_ keeps
it as a length-1 axis in the chunk selection, so the shapes differ in rank
while agreeing in element count, the reshape was skipped, and the write
still raised the shape-mismatch ValueError.

The invariant is that a coordinate indexer addresses the value flat, so
ravel any multi-dimensional value instead. Both partial-encode paths now
share one helper for deriving the shard indexer and shaping the value, and
the check is an isinstance on CoordinateIndexer so mypy types sel_shape.

The regression test is parametrized over selections with an integer axis
in each position, three array axes, and an unsorted selection spanning two
shards.

Closes zarr-developers#4315

Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-v-b
d-v-b marked this pull request as ready for review September 4, 2026 09:31
@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.23%. Comparing base (9c29a0d) to head (23a0003).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4316      +/-   ##
==========================================
+ Coverage   94.21%   94.23%   +0.01%     
==========================================
  Files          92       92              
  Lines       12871    12880       +9     
==========================================
+ Hits        12127    12137      +10     
+ Misses        744      743       -1     
Files with missing lines Coverage Δ
src/zarr/codecs/sharding.py 96.25% <100.00%> (+0.02%) ⬆️
src/zarr/testing/strategies.py 95.78% <100.00%> (+0.35%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

d-v-b and others added 2 commits September 4, 2026 12:02
… unit axes

Ravelling every multi-dimensional value for a coordinate selection was too
lenient. A mask write with a (2, 2) value for four selected elements, or an
orthogonal write with a spurious trailing axis, raises on an unsharded array
but was silently accepted on a sharded one, because the element count
matched and the shard-level selection cannot tell orthogonal from mask
indexing.

The value shape can. An np.ix_ selection has an N-D sel_shape and the
caller's value is that shape minus the integer-indexed axes, which np.ix_
keeps as length-1 axes. Ravel exactly that shape and leave any other rank
alone, so an invalid write fails the same way it does without sharding.

Adds an error test for both leniencies and a positive case with a length-1
array axis next to an integer axis.

Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…roperty tests

The property tests could not have found the sharded orthogonal-write bugs:

- test_oindex, test_mask_indexing and test_block_indexing skipped their set
  half on sharded arrays with assume(zarray.shards is None), added in zarr-developers#2825
  when the bug was first seen and never lifted. test_vindex had its set half
  commented out.
- orthogonal_indices wrapped every bare integer as a one-element array, so
  zarr never received an integer index and OrthogonalIndexer's dropped-axis
  path was unreachable. basic_indices(min_dims=1) never yields an integer
  either, so that branch was dead.
- arrays() only drew a shard shape when every axis had a chunk strictly
  between 1 and the axis length, on top of the v3 and regular-grid draws:
  2 of 500 test_oindex examples were sharded.

Lift the skips, draw integers explicitly and give the numpy indexer the
same dropped-axis result, enable the vindex write with a duplicate-point
filter, and let any chunk that fits the array be sharded (33 of 500 now).
With these changes test_oindex fails against the code before this PR with
the mixed-integer shape mismatch, and passes with it.

Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-v-b

d-v-b commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Two follow-up commits, addressing the review finding on mask assignment and the test gap it exposed.

49f8872 narrows the flatten

The first version ravelled any multi-dimensional value once the shard-level indexer was a CoordinateIndexer. That is too broad: the shard-level selection cannot tell orthogonal from mask indexing, so a mask write with a (2, 2) value for four selected elements, or an orthogonal write with a spurious trailing axis like (3, 2, 1), was silently accepted on a sharded array while raising ValueError on an unsharded one (and in numpy).

The value shape can tell them apart. An np.ix_ selection has an N-D sel_shape, and the caller's value is that shape with only length-1 axes removed, which is exactly where np.ix_ puts integer indices. The helper now ravels only when value.shape is sel_shape minus zero or more unit axes (_drops_only_unit_axes). A mask arrives with a 1-D sel_shape, so a 2-D value no longer qualifies and fails downstream the same way it does unsharded; an extra axis fails to qualify for the same reason. test_sharding_set_rejects_value_with_wrong_rank covers both on both pipelines and fails against the previous commit.

aaf30c8 makes the property tests able to find this

The question of how this got past Hypothesis had a plain answer: the case was unreachable.

  • test_oindex, test_mask_indexing and test_block_indexing skipped their set half on sharded arrays with assume(zarray.shards is None), added in Add more setitem property tests #2825 when bug with setitem with oindex and sharding #2834 was filed and never lifted. test_vindex had its set half commented out.
  • orthogonal_indices wrapped every bare integer as a one-element array, so zarr never received an integer index and the dropped-axis path in OrthogonalIndexer was unreachable. basic_indices(min_dims=1) never yields an integer either, so that branch was dead.
  • arrays() only drew a shard shape when every axis had a chunk strictly between 1 and the axis length. With the v3 and regular-grid draws on top, 2 of 500 test_oindex examples were sharded.

The commit lifts the skips, draws integers explicitly (the numpy indexer gets a 0-d index so its result drops the axis as oindex does), enables the vindex write behind a duplicate-point filter, and lets any chunk that fits the array be sharded (33 of 500 now, with an event() so --hypothesis-show-statistics shows the split). With these changes test_oindex fails against the code before this PR with the mixed-integer shape mismatch at the default 300 examples, and passes with it. The full property module and the stateful store and hierarchy machines pass under the default and nightly profiles.

The mask leniency is still outside what the property tests express, since they only draw correctly shaped values; the unit error test carries that.

…arrays alike

Replace the sharded-only wrong-rank test in test_sharding.py and the GH2469
one-off in test_indexing.py with one parametrized error test: a coordinate
write with twice the elements, a mask write with a 2-D value, and an
orthogonal write with an extra axis each raise ValueError on chunked and
sharded arrays under both codec pipelines. The property under test is that
storage layout does not change which writes are rejected, which a
sharded-only test could not state. zarr_array_from_numpy_array grows a
shards argument for it.

Only the rejection is asserted; a write that fails inside the chunk merge
may already have touched other chunks on a chunked array.

Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@d-v-b

d-v-b commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

it's going in! this unblocks a longstanding hole in our array indexing coverage for sharded arrays.

@d-v-b
d-v-b merged commit a50d791 into zarr-developers:main Sep 4, 2026
39 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.

bug(indexing): oindex with mixed numpy array and integer indexing fails

1 participant