zarr-python icon indicating copy to clipboard operation
zarr-python copied to clipboard

Fix indexing bug when reading past the first chunk in a shard

Open darsnack opened this issue 1 year ago • 4 comments

Closes #2018 by passing the correct format for the byte_range argument to a StorePath byte getter when accessing chunks within a shard. Previously, the return value of get_chunk_slice was directly passed in as the byte range. This is incorrect since it is formatted as (start index, end index) when it should be (start index, total read length).

TODO:

  • [x] 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/tutorial.rst
  • [ ] Changes documented in docs/release.rst
  • [ ] GitHub Actions have all passed
  • [ ] Test coverage is 100% (Codecov passes)

darsnack avatar Jul 10 '24 12:07 darsnack

Thanks! I don't quite understand why this hasn't shown up in the tests. Would you mind adding a test for this bug to https://github.com/zarr-developers/zarr-python/blob/v3/tests/v3/test_codecs/test_sharding.py?

normanrz avatar Jul 10 '24 15:07 normanrz

Taking a quick glance at the test suite indicates that we never attempt to read an intermediate slice outside of the first chunk in a shard. Any access beyond the first chunk is always a slice without an upper bound.

I could either modify the existing tests to include accesses the fit the pattern in the bug, or I can add a separate test case. What would you prefer?

darsnack avatar Jul 10 '24 19:07 darsnack

Thanks! I think adding parameterization for multiple access patterns is the way to go. That is what your commit attempts, right?

normanrz avatar Jul 12 '24 15:07 normanrz

Yeah that's what the commit does.

The test failure of a segfault is puzzling, but I believe it is genuine. ~~I'm using my PR branch in another codebase, and I'm also seeing a segfault there occurring around the same point.~~ (ignore this data point) Any idea what it could be?

darsnack avatar Jul 12 '24 18:07 darsnack