Add AbstractChunkKeyEncoding by mkitti · Pull Request #234 · JuliaIO/Zarr.jl

mkitti · 2026-03-06T00:07:41Z

Add AbstractChunkKeyEncoding
Rename ChunkEncoding to ChunkKeyEncoding

Chunk Key Encodings are a potential extension point in Zarr:
https://zarr-specs.readthedocs.io/en/latest/v3/chunk-key-encodings/index.html

There are two chunk key encoding extensions:

I proposed a suffix chunk key encoding to add a file extension:
feat: Add suffix chunk key encoding zarr-developers/zarr-extensions#28
There is also a fanout chunk key encoding:
Fanout chunk key encoding zarr-developers/zarr-extensions#31

Thus, I think we need an AbstractChunkKeyEncoding to allow for alternate
chunk key encoding extensions.

While ChunkEncoding is a shorter name, it could be confused with the
encoding of a chunk itself rather than it's name. Including the word "Key"
would make it clearer that we are indeed referring to the Zarr v3 concept
of a "chunk key encoding".

mkitti · 2026-03-06T00:12:30Z

I'm pretty sure that we at least need an abstract type. I could be convinced to not use the word "key", but I do think this will be confusing.

asinghvi17

This seems fine to me. I think we don't yet set the API for that, but we can probably do that later. I don't think that's necessary quite yet.

mkitti · 2026-03-06T00:17:35Z

If the suffix type advances in zarr-extensions, I think we should consider integrating it into the main ChunkKeyEncoding type rather than creating another type for it.

mkitti · 2026-03-06T12:03:25Z


 """Metadata for Zarr version 3 arrays"""
-struct MetadataV3{T,N,C,F} <: AbstractMetadata{T,N,C,F}
+struct MetadataV3{T,N,C,F,E} <: AbstractMetadata{T,N,C,F,E}


@asinghvi17 Do we need a type parameter for the ChunkKeyEncoding?

Makes sense to me, yeah. I added an extra comment about constraining that at the abstract type level.

mkitti · 2026-03-06T13:38:01Z

A prefix chunk key encoding scheme has appeared:
zarr-developers/zarr-extensions#47

asinghvi17 · 2026-03-07T15:54:46Z

Nightly CI is an expected failure from CondaPkg, I would not worry about that too much. Perhaps we should not load CondaPkg on nightly...

The doctest failure is a bit odd though

mkitti · 2026-03-07T15:56:17Z

There is an available patch. I was thinking we should test that patch on nightly.

lazarusA · 2026-03-07T19:08:41Z

The doctest failure is a bit odd though

I fixed docs build on #237.

mkitti · 2026-03-09T20:20:59Z

I intend to merge this if CI passes here.

coveralls · 2026-03-09T20:26:05Z

Pull Request Test Coverage Report for Build 23070014955

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

59 of 62 (95.16%) changed or added relevant lines in 5 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage remained the same at 82.336%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Storage/Storage.jl	9	10	90.0%
src/chunkkeyencoding.jl	32	34	94.12%

Files with Coverage Reduction	New Missed Lines	%
src/metadata.jl	1	93.68%
src/Storage/Storage.jl	1	83.74%
src/metadata3.jl	4	96.83%

Totals
Change from base Build 22929818448:	0.0%
Covered Lines:	1403
Relevant Lines:	1704

💛 - Coveralls

mkitti · 2026-03-09T20:29:42Z

I will follow this up with another pull request to test adding the SuffixChunkKeyEncoding.

mkitti · 2026-03-10T06:32:33Z

Actually, I think I will wait for #232 to merge into master.

Co-authored-by: Anshul Singhvi <anshulsinghvi@gmail.com>

asinghvi17 · 2026-03-11T11:27:06Z

+        E = ChunkKeyEncoding
+    else
+        error("Unknown chunk key encoding: ", chunk_key_encoding["name"])
    end

-    MetadataV3{TU, N, typeof(pipeline)}(
+    MetadataV3{TU, N, typeof(pipeline),ChunkKeyEncoding}(


I'm not seeing where E gets used here

Suggested change

E = ChunkKeyEncoding

else

error("Unknown chunk key encoding: ", chunk_key_encoding["name"])

end

MetadataV3{TU, N, typeof(pipeline)}(

MetadataV3{TU, N, typeof(pipeline),ChunkKeyEncoding}(

E = ChunkKeyEncoding

else

error("Unknown chunk key encoding: ", chunk_key_encoding["name"])

end

MetadataV3{TU, N, typeof(pipeline),E}(

perhaps?

This may have been caught between commits. I added a SuffixChunkKeyEncoding to see how this might generalize across multiple AbstractChunkKeyEncodings.

mkitti · 2026-03-11T16:37:56Z

I've added a SuffixChunkKeyEncoding in 4681551 mainly to assess how the current code would work with multiple AbstractChunkKeyEncoding types. However, the longer term plan if we want to keep this would be to integrate it into the main ChunkKeyEncoding class.

My current expectation is that introducing a new type here will cause some type instability.

mkitti · 2026-03-11T21:18:30Z

Thinking about this, perhaps we are being inconsistent with naming. Some of the abstract types do not start with Abstract.

asinghvi17 · 2026-03-11T21:29:20Z

True, but then they probably should start with Abstract (in my opinion, anyway :D)

* Implement a registration system for chunk key encoding * refactor: Add typed ChunkKeyEncodingEntry to chunk key encoding registration Introduce `ChunkKeyEncodingEntry` struct pairing `return_type::Type{<:AbstractChunkKeyEncoding}` with `parser::Function`, replacing the bare `Dict{String, Function}` registry. - `register_chunk_key_encoding(parser, name, ::Type{T}=AbstractChunkKeyEncoding)` now stores a `ChunkKeyEncodingEntry` with the declared return type; the type is the last (optional) argument to work naturally with do-block syntax - `parse_chunk_key_encoding` asserts the result against `entry.return_type` at runtime - All built-in registrations updated with their specific concrete types (`ChunkKeyEncoding` for "default" and "v2", `SuffixChunkKeyEncoding` for "suffix") Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

mkitti mentioned this pull request Mar 6, 2026

support v3 #215

Closed

asinghvi17 approved these changes Mar 6, 2026

View reviewed changes

mkitti commented Mar 6, 2026

View reviewed changes

mkitti marked this pull request as ready for review March 6, 2026 12:03

asinghvi17 reviewed Mar 7, 2026

View reviewed changes

Comment thread src/metadata.jl Outdated

mkitti and others added 3 commits March 10, 2026 21:09

Add AbstractChunkKeyEncoding, rename ChunkEncoding

b7f41f2

Add constraint

48bd35f

Co-authored-by: Anshul Singhvi <anshulsinghvi@gmail.com>

chore: Update to new master

5fc50ef

mkitti force-pushed the mkitti-abstract-chunk-key-encoding branch from 8970310 to 5fc50ef Compare March 11, 2026 02:52

mkitti requested review from asinghvi17 and lazarusA March 11, 2026 03:09

asinghvi17 approved these changes Mar 11, 2026

View reviewed changes

asinghvi17 reviewed Mar 11, 2026

View reviewed changes

feat: Add SuffixChunkKeyEncoding

4681551

mkitti mentioned this pull request Mar 11, 2026

Registration system for chunk key encoding #242

Merged

mkitti merged commit d13f7cf into master Mar 13, 2026
18 checks passed

Uh oh!

Conversation

mkitti commented Mar 6, 2026

Uh oh!

mkitti commented Mar 6, 2026

Uh oh!

asinghvi17 left a comment

Choose a reason for hiding this comment

Uh oh!

mkitti commented Mar 6, 2026

Uh oh!

mkitti Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

asinghvi17 Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

mkitti commented Mar 6, 2026

Uh oh!

Uh oh!

asinghvi17 commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkitti commented Mar 7, 2026

Uh oh!

lazarusA commented Mar 7, 2026

Uh oh!

mkitti commented Mar 9, 2026

Uh oh!

coveralls commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 23070014955

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

mkitti commented Mar 9, 2026

Uh oh!

mkitti commented Mar 10, 2026

Uh oh!

asinghvi17 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

asinghvi17 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

mkitti Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

mkitti commented Mar 11, 2026

Uh oh!

mkitti commented Mar 11, 2026

Uh oh!

asinghvi17 commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asinghvi17 commented Mar 7, 2026 •

edited

Loading

coveralls commented Mar 9, 2026 •

edited

Loading