Skip to content

fix(zarr-metadata): v2 array document is open and filters may be empty - #4365

Merged
d-v-b merged 5 commits into
zarr-developers:mainfrom
d-v-b:fix/zarr-metadata-v2-open-array
Sep 16, 2026
Merged

d-v-b merged 5 commits into
zarr-developers:mainfrom
d-v-b:fix/zarr-metadata-v2-open-array

Conversation

@d-v-b

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

Copy link
Copy Markdown
Contributor

This AI-authored PR brings zarr-metadata closer to the spec by allowing filters: [] and extra keys in .zarray (I didn't know the spec allowed this -- extra keys are forbidden from .zgroup)

🤖 AI text below 🤖

Two structural rules in zarr_metadata's v2 array validator are stricter than the v2 spec. The shared conformance corpus was updated first (d-v-b/zarr-metadata.js#2, conformance/v2_array.json cases 5 and 8), which turns this package's conformance run red (65/67) until it follows — this PR brings it back to 67/67.

Rule Spec (docs/v2/v2.0.rst) Was Now
Extra members in .zarray line 91: "Other keys SHOULD NOT be present within the metadata object and SHOULD be ignored by implementations" — vs .zgroup line 313: "Other keys MUST NOT be present" invalid_value at the key tolerated by validate_array_metadata_v2, dropped by the model, allowed by the Pydantic schema
filters: [] line 77: "A list of JSON objects providing codec configurations, or null" — no minimum invalid_value "expected at least one filter" accepted (min_length=1 dropped from the schema too)

Changes

  • model/_validation.py: the v2 array validator no longer calls _unexpected_keys (the group validator still does) and no longer rejects an empty filter list.
  • _pydantic_schema.py: ZarrV2ArrayMetadataJSON becomes extra_items=JSONValue like the v3 TypedDict; ZarrV2FilterPipelineJSON loses Field(min_length=1). ZarrV2GroupMetadataJSON stays closed=True.
  • Unchanged: the on-disk .zarray rule in from_key_value that attributes must not appear there (it lives in .zattrs) — that's a different rule, and the test for it is now separate from the "other extras are ignored" test.
  • Tests inverted accordingly: test_v2_filters_may_be_empty, test_array_v2_ignores_unknown_document_member, test_v2_from_key_value_ignores_zarray_extra_members, test_v2_array_schema_allows_empty_filters, test_v2_array_schema_allows_unknown_document_members.

The TypeScript port keeps the SHOULD NOT visible: its (TS-only) v2 semantic layer reports extra .zarray members as advisories so consumers can surface them as warnings.

Test plan

  • just test in packages/zarr-metadata: 595 passed
  • just lint
  • zarr-metadata.js just conformance <this checkout>: 67/67 (was 65/67 against the released package)

🤖 Generated with Claude Code

d-v-b and others added 2 commits September 16, 2026 14:32
Two v2 structural rules were stricter than the spec, and the shared
conformance corpus (zarr-metadata.js, conformance/v2_array.json cases 5
and 8) has been updated first; this brings the reference implementation
back into agreement.

- Members outside the .zarray definition were rejected. The spec: "Other
  keys SHOULD NOT be present within the metadata object and SHOULD be
  ignored by implementations" — a recommendation, unlike .zgroup's
  "Other keys MUST NOT be present". Extras are now tolerated by the
  validator, dropped by the model, and permitted by the Pydantic schema
  (the TypedDict is open, like the v3 one). The on-disk `.zarray` rule
  that `attributes` belongs in `.zattrs` is unchanged.
- filters: [] was rejected ("expected at least one filter"). The spec:
  "A list of JSON objects providing codec configurations, or null" — an
  empty list is a list. The Pydantic schema's min_length is dropped.

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>
@github-actions github-actions Bot added needs release notes Automatically applied to PRs which haven't added release notes zarr-metadata Specific to the zarr-metadata sub-package labels Sep 16, 2026
@read-the-docs-community

Copy link
Copy Markdown

@read-the-docs-community

read-the-docs-community Bot commented Sep 16, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.22%. Comparing base (dfa18e8) to head (2e6b275).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4365   +/-   ##
=======================================
  Coverage   94.22%   94.22%           
=======================================
  Files          92       92           
  Lines       12942    12942           
=======================================
  Hits        12195    12195           
  Misses        747      747           
🚀 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 3 commits September 16, 2026 15:43
Assisted-by: ClaudeCode:claude-fable-5-1
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every docstring or comment that cites the Zarr spec or a zarr-extensions
README now carries a commit-pinned permalink with a line range
(zarr-specs fc7dd9c; zarr-extensions 4da7b37, the registry commit the
TypeScript port vendors). Unpinned zarr-extensions `tree/main` page links
are pinned the same way.

Three statements were wrong or stale and are corrected:

- zstd: `checksum` was typed required "per the proposed specification"
  (zarr-specs PR #256, never merged). The published zarr-extensions entry
  makes it optional ("Should be omitted if false"; schema requires only
  `level`), so it is now `NotRequired[bool]`.
- v3 consolidated metadata was described as "not a spec artifact"; since
  zarr-specs zarr-developers#373 the core spec names the field and fixes its envelope
  (core/index.rst L802-L816); the entry format remains a convention.
- The v2 array `*Partial` docstring spoke of a "closed shape"; the array
  document is open (other keys SHOULD be ignored), unlike `.zgroup`.

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 16, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Added a docs commit that links every spec statement in packages/zarr-metadata (source and tests) to the exact text — commit-pinned permalinks with line ranges into zarr-specs fc7dd9c and zarr-extensions 4da7b37. Page-level tree/main links to zarr-extensions are pinned the same way.

While doing that, three statements turned out to be wrong or stale, corrected in the same commit — reviewers may want to look at these specifically:

  1. v3/codec/zstd.py typed checksum as required, citing zarr-specs PR Surprising behaviour of append on object arrays #256 ("unmerged at time of writing" — it never merged). The published zarr-extensions entry says checksum is optional ("Should be omitted if false") and its schema.json requires only level. ZstdCodecConfiguration.checksum is now NotRequired[bool]. Typing-only; no runtime validator inspects zstd configuration.
  2. v3/consolidated.py said the v3 core spec "does not define consolidated metadata" / "not a spec artifact". Since zarr-specs HTTP Store #373 the core spec's Additional fields section names consolidated_metadata and fixes its envelope (core/index.rst L802-L816); the entry format is still a reference-implementation convention. Reworded accordingly.
  3. v2/array.py ZarrV2ArrayMetadataJSONPartial docstring referred to the array document's "closed shape", which this PR makes untrue; it now says other .zarray keys are ignored per the spec (the .zgroup counterpart keeps "closed", with its MUST NOT linked).

just test 595 passed; just lint and just typecheck clean.

@d-v-b
d-v-b merged commit b3c5972 into zarr-developers:main Sep 16, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs release notes Automatically applied to PRs which haven't added release notes zarr-metadata Specific to the zarr-metadata sub-package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant