Repository navigation
zarr3: support the regular_bounded chunk grid - #296
rhoadesScholar wants to merge 7 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the 'offset' chunk key encoding in the Zarr3 driver, allowing arrays to grow in the negative direction of a dimension by applying a fixed per-dimension integer offset to chunk grid positions. The changes include metadata parsing, validation, key formatting/parsing logic, schema updates, and comprehensive unit tests. The review feedback highlights two issues in driver.cc: first, a potential out-of-bounds or undefined behavior when parsing keys for rank 0 arrays due to incorrect prefix removal length; second, a potential issue in MinGridIndexForLexicographicalOrder where passing a non-positive value to MinValueWithMaxBase10Digits can be avoided by returning -offset directly when the interval consists entirely of negative keys.
The `offset` encoding stores a chunk under the `default` key of its grid index plus a fixed per-dimension integer offset, so that an array grows in the negative direction of a dimension by whole chunks as a metadata-only change (zarr-extensions chunk-key-encodings/offset). Adds the kind to `ChunkKeyEncoding` with its `offset` vector (required for this kind, rejected for the others, length checked against the rank in ValidateMetadata), applies it in FormatKey and GetChunkStorageKey, and accepts a leading `-` in ParseKey. MinGridIndexForLexicographicalOrder splits off every grid index whose key is negative, since negative base-10 components do not sort numerically, and shifts the digit-count rule for the rest. Schema docs and tests, including a growth that rewrites only zarr.json and a shrink that deletes through key ranges.
e713ac2 to
92e1fec
Compare
|
Ready for review. The encoding is proposed in zarr-developers/zarr-extensions#80; this is the reader side. cc @jbms |
|
Converting to draft while the design is worked out in zarr-developers/zarr-extensions#79. This branch is a prototype and will be reworked to match. |
…y encoding Rework following zarr-extensions#79. The `offset` chunk key encoding is removed. `regular_bounded` is a regular chunk grid with an `origin` (the array coordinate where chunk 0 starts) and a `lower_bound` (the coordinate of the first element). The array's domain starts at `lower_bound`, and the internal grid space is the array coordinate minus `origin`, so chunk grid indices are the absolute ones and may be negative. The lower bound of a `regular_bounded` array is implicit, so `Resize` can move it; chunks that fall off the front are deleted through the existing box-difference path. `regular` arrays are unchanged. Key parsing accepts a leading `-` and rejects `-0`. The per-component fill value is broadcast over an unbounded domain, since chunks may now sit at negative internal coordinates. The chunk layout reports `origin` as the grid origin, and the compatibility key ignores `lower_bound`, as it ignores `shape`.
rhoadesScholar
changed the title
This branch has not been deployed
1 participant
Prototype for zarr-developers/zarr-extensions#79: the
regular_boundedchunk grid proposed in zarr-developers/zarr-extensions#81. It is a regular grid with anorigin(the array coordinate where chunk 0 starts) and alower_bound(the coordinate of the first element), following the origin-and-bounds design suggested on zarr-developers/zarr-extensions#80.lower_bound, and the internal grid space is the array coordinate minusorigin. Cell indices are therefore the absolute chunk grid indices, negative where they should be, and they stay put when the lower bound moves.regular_boundedarray is implicit, soResizecan grow or shrink it. Chunks that fall off the front are deleted through the existing box-difference path.regulararrays are unchanged.-and rejects-0. Formatting needed no change.AsyncWriteArray::Spec::GetFillValueForDomainasserts on once chunks sit at negative internal coordinates.originas the grid origin. The compatibility key ignoreslower_bound, as it ignoresshape.ParseKeyfrom review.Before and after. Before,
Openrefuses such an array because the chunk grid is unknown. After:GrowsInFrontByEditingZarrJsonAlonewrites aregulararray, rewrites onlyzarr.jsonto grow it by two elements in front (less than a chunk), and reopens it with domain[-2, 6). It reads the old data at its old coordinates, then writes the front without any existing chunk moving.ResizeMovesTheLowerBoundgrows the front throughtensorstore::Resize, shrinks it again, and checks that exactly the chunks that fell off are deleted.Also checked by hand: two stores written by zarr-python (zarr-developers/zarr-python#4516) read here with the same domains and values. One was grown in front by
prepend; the other has a non-zero origin on both axes.Tests. The metadata tests cover the round trip, members rejected under
regular, both members required with one entry per dimension, and the compatibility key. The driver tests cover the domain and grid origin, keys, reopening, growth by a metadata edit, resizing the lower bound both ways, storage statistics over negative keys, and-0.bazel test //tensorstore/driver/zarr3/...passes.Not covered here: creating a
regular_boundedarray from a schema domain with a non-zero origin. Today such a domain is refused; it could map onto this grid.