diff --git a/changes/4340.bugfix.md b/changes/4340.bugfix.md new file mode 100644 index 0000000000..992cf67ff9 --- /dev/null +++ b/changes/4340.bugfix.md @@ -0,0 +1 @@ +Fix nested structured dtype round-tripping through Zarr format 2 metadata. Native NumPy dtypes with field titles or subarray fields now raise `ValueError` identifying the unsupported field during conversion, rather than being converted to duplicate fields or raw-byte fields. This reflects a limitation of the current Zarr-Python dtype implementation; the V2 format supports subarray fields, as did Zarr-Python 2.x. Padded layouts continue to be converted to packed layouts, now with a `ZarrUserWarning` explaining that field values are preserved when writing arrays but offsets and itemsize may change. These checks also apply to nested fields. Add dtype serialization and layout-conversion property tests using new `zdtypes` and `structured_dtypes` strategies. diff --git a/docs/user-guide/data_types.md b/docs/user-guide/data_types.md index 91f828a738..4f44fc9576 100644 --- a/docs/user-guide/data_types.md +++ b/docs/user-guide/data_types.md @@ -84,8 +84,18 @@ arbitrary fixed-size byte strings. The `str` attribute of a regular NumPy void data type is the same as the `str` of a NumPy structured data type. This means that the `str` attribute does not convey information about the fields contained in a structured data type. For these reasons, Zarr V2 uses a special data type encoding for structured data types. -They are stored in JSON as lists of pairs, where the first element is a string, and the second -element is a Zarr V2 data type specification. This representation supports recursion. +The [V2 specification](https://zarr-specs.readthedocs.io/en/latest/v2/v2.0.html#data-type-encoding) +represents fields as `[fieldname, datatype]` or `[fieldname, datatype, shape]`, where the +optional shape describes a subarray field. Field data types can themselves be structured. +For example, `[["position", " TypeGuard[StructuredName_V2 if isinstance(data[-1], str): return True elif isinstance(data[-1], Sequence): - return check_structured_dtype_v2_inner(data[-1]) + # A nested structured dtype's field has the form [name, [[sub_name, sub_dtype], ...]], + # i.e. the last element is itself a sequence of field pairs rather than a single + # [name, dtype] pair, so it must be validated as a list of fields, not a single field. + return check_structured_dtype_name_v2(data[-1]) return False diff --git a/src/zarr/core/dtype/npy/structured.py b/src/zarr/core/dtype/npy/structured.py index dcc523d1d2..3dddd8a7ff 100644 --- a/src/zarr/core/dtype/npy/structured.py +++ b/src/zarr/core/dtype/npy/structured.py @@ -1,10 +1,13 @@ from __future__ import annotations +import warnings from collections.abc import Sequence from dataclasses import dataclass +from pathlib import Path from typing import TYPE_CHECKING, ClassVar, Literal, Self, TypeGuard, cast, overload import numpy as np +from numpy.lib.recfunctions import repack_fields from zarr.core.common import NamedConfig from zarr.core.dtype.common import ( @@ -22,13 +25,54 @@ check_json_str, ) from zarr.core.dtype.wrapper import TBaseDType, TBaseScalar, ZDType -from zarr.errors import DataTypeValidationError +from zarr.errors import DataTypeValidationError, ZarrUserWarning if TYPE_CHECKING: from zarr.core.common import JSON, ZarrFormat StructuredScalarLike = list[object] | tuple[object, ...] | bytes | int +# The root of the zarr package, used to attribute layout warnings to the caller outside zarr. +_ZARR_PACKAGE_ROOT = str(Path(__file__).parents[3]) + + +def _unsupported_field_feature(dtype: np.dtype[np.void]) -> str | None: + """ + Check for field features unsupported by this implementation's native dtype conversion. + + `Structured.fields` stores `(name, ZDType)` pairs and `to_native_dtype` packs them + contiguously. This conversion does not preserve NumPy field titles or subarray shapes. + Reject those features, including in nested fields, rather than silently losing them: + + - field titles, e.g. `np.dtype([(("title", "name"), "i4")])` + - subarray fields, e.g. `np.dtype([("name", "i4", (2,))])` + + Returns + ------- + str | None + `None` if these field features are supported, otherwise a description of the problem. + + Notes + ----- + This is an implementation limitation, not a statement about the V2 format, which has + an encoding for subarray fields. + """ + names = dtype.names + fields = dtype.fields + if names is None or fields is None: # pragma: no cover - only called on structured dtypes + return None + for name in names: + field_dtype, _offset, *title = fields[name] + if title: + return f"field {name!r} has a title ({title[0]!r})" + if field_dtype.subdtype is not None: + return f"field {name!r} is a subarray with shape {field_dtype.shape}" + if field_dtype.names is not None: + reason = _unsupported_field_feature(field_dtype) + if reason is not None: + return f"within field {name!r}: {reason}" + return None + class StructuredJSON_V2(DTypeConfig_V2[StructuredName_V2, None]): """ @@ -175,6 +219,15 @@ def from_native_dtype(cls, dtype: TBaseDType) -> Self: DataTypeValidationError If the input data type is not an instance of np.dtypes.VoidDType with a non-null ``fields`` attribute. + ValueError + If the input has field titles or subarray fields, which this implementation's + native dtype conversion does not support. + + Warns + ----- + ZarrUserWarning + If a non-default field layout is converted to a packed layout. Field values are + preserved when writing arrays, but offsets and itemsize may change. Notes ----- @@ -185,10 +238,38 @@ def from_native_dtype(cls, dtype: TBaseDType) -> Self: fields: list[tuple[str, ZDType[TBaseDType, TBaseScalar]]] = [] if cls._check_native_dtype(dtype): - # fields of a structured numpy dtype are either 2-tuples or 3-tuples. we only - # care about the first element in either case. - for key, (dtype_instance, *_) in dtype.fields.items(): # type: ignore[union-attr] - dtype_wrapped = get_data_type_from_native_dtype(dtype_instance) + reason = _unsupported_field_feature(dtype) + if reason is not None: + # NOTE: this is a ValueError rather than a DataTypeValidationError on purpose. + # The data type registry suppresses DataTypeValidationError (treating it as + # "this dtype does not match"), but a dtype with an unsupported field feature + # *does* match this dtype class -- it simply cannot be represented faithfully -- + # so we must raise an error the registry propagates to the caller. + raise ValueError( + f"Cannot convert the structured data type {dtype}: {reason}. " + "Zarr-Python's current structured dtype conversion does not support " + "field titles or subarray fields." + ) + # Repack once, at the top level, before resolving the fields. Nested fields then + # reach the registry already packed, so a padded nested field warns exactly once, + # here, rather than once per level of nesting. + packed = cast("np.dtype[np.void]", repack_fields(dtype, recurse=True)) + if packed != dtype: + warnings.warn( + "The structured dtype is converted to a packed field layout. " + "Field values are preserved when writing arrays, but field offsets and " + "itemsize may change.", + ZarrUserWarning, + # Attribute the warning to the first frame outside the zarr package, since + # the depth of the call chain that leads here varies by entry point. This + # reaches the caller for direct uses of the dtype API; the synchronous array + # API runs on the event loop thread, where no caller frame is available. + skip_file_prefixes=(_ZARR_PACKAGE_ROOT,), + ) + # Iterate over `names` rather than `fields`: the `fields` mapping also + # contains an entry for every field title, which would duplicate titled fields. + for key in packed.names: # type: ignore[union-attr] + dtype_wrapped = get_data_type_from_native_dtype(packed.fields[key][0]) # type: ignore[index] fields.append((key, dtype_wrapped)) return cls(fields=tuple(fields)) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index db01697f1e..cd1cfba20a 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -1,3 +1,4 @@ +import dataclasses import itertools import math import sys @@ -27,7 +28,11 @@ from zarr.core.array import Array, CompressorsLike, SerializerLike from zarr.core.chunk_key_encodings import DefaultChunkKeyEncoding from zarr.core.common import JSON, AccessModeLiteral, ZarrFormat -from zarr.core.dtype import get_data_type_from_native_dtype +from zarr.core.dtype import data_type_registry, get_data_type_from_native_dtype +from zarr.core.dtype.common import HasItemSize +from zarr.core.dtype.npy.common import DATETIME_UNIT +from zarr.core.dtype.npy.structured import Struct +from zarr.core.dtype.wrapper import TBaseDType, TBaseScalar, ZDType from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata from zarr.core.metadata.v3 import RectilinearChunkGridMetadata, RegularChunkGridMetadata from zarr.core.sync import sync @@ -70,6 +75,123 @@ def dtypes() -> st.SearchStrategy[np.dtype[Any]]: ) +_field_names = st.text( + alphabet=st.characters(min_codepoint=97, max_codepoint=122), min_size=1, max_size=4 +) +_field_titles = st.text( + alphabet=st.characters(min_codepoint=65, max_codepoint=90), min_size=1, max_size=4 +) + + +def _leaf_zdtypes(cls: type[ZDType[TBaseDType, TBaseScalar]]) -> SearchStrategy[ZDType[Any, Any]]: + """ + A strategy for instances of a single non-struct `ZDType` class, drawing each constructor + parameter the class declares from its valid range. + """ + params = {f.name for f in dataclasses.fields(cls)} + kwargs: dict[str, SearchStrategy[Any]] = {} + if "endianness" in params: + kwargs["endianness"] = st.sampled_from(["little", "big"]) + if "length" in params: + kwargs["length"] = st.integers(min_value=1, max_value=16) + if "unit" in params: + # NumPy spells the microsecond unit "us", so the "μs" alias never round-trips as-is. + kwargs["unit"] = st.sampled_from([u for u in DATETIME_UNIT if u != "μs"]) + kwargs["scale_factor"] = st.integers(min_value=1, max_value=2**31 - 1) + return st.builds(cls, **kwargs).map(_normalize_generic_scale_factor) + return st.builds(cls, **kwargs) + + +def _normalize_generic_scale_factor(zdtype: Any) -> Any: + """ + NumPy retains generic scale factors internally, but its dtype string omits them. + Use `scale_factor=1` so the generated dtype survives Zarr V2 string serialization. + """ + if zdtype.unit == "generic": + return dataclasses.replace(zdtype, scale_factor=1) + return zdtype + + +def _struct_zdtypes( + children: SearchStrategy[ZDType[Any, Any]], +) -> SearchStrategy[ZDType[Any, Any]]: + """A strategy for `Struct` instances whose field data types are drawn from `children`.""" + + @st.composite + def _draw(draw: st.DrawFn) -> ZDType[Any, Any]: + num_fields = draw(st.integers(min_value=1, max_value=4)) + # suffix with the index so that names are unique without filtering + names = [f"{draw(_field_names)}{i}" for i in range(num_fields)] + return Struct(fields=tuple((name, draw(children)) for name in names)) + + return _draw() + + +def zdtypes(*, max_leaves: int = 6) -> SearchStrategy[ZDType[Any, Any]]: + """ + Generate instances of the built-in registered `ZDType` classes, including nested `Struct`. + + Struct fields are restricted to fixed-size data types, as required by the V3 `struct` + extension. This strategy samples bounded lengths and normalized datetime units/scales; + it does not cover every valid instance or arbitrary third-party dtype constructors. + """ + leaf_classes = [cls for cls in data_type_registry.contents.values() if cls is not Struct] + leaves = st.one_of([_leaf_zdtypes(cls) for cls in leaf_classes]) + fixed_size_leaves = st.one_of( + [_leaf_zdtypes(cls) for cls in leaf_classes if issubclass(cls, HasItemSize)] + ) + structs = st.recursive(fixed_size_leaves, _struct_zdtypes, max_leaves=max_leaves).filter( + lambda dt: isinstance(dt, Struct) + ) + return leaves | structs + + +@st.composite +def structured_dtypes( + draw: st.DrawFn, *, allow_extended: bool = False, max_depth: int = 3 +) -> np.dtype[np.void]: + """ + A strategy for native NumPy structured dtypes, flat or nested. + + With `allow_extended=False` (the default), generate packed fields without titles or + subarray shapes. With `allow_extended=True`, also generate field titles, subarray fields, + and `align=True` layouts, independently. The current native dtype conversion rejects + titles and subarray fields and accepts padding with a warning. These are implementation + behaviors, not restrictions imposed by the V2 format. + """ + fixed_size_leaves = st.one_of( + [ + _leaf_zdtypes(cls) + for cls in data_type_registry.contents.values() + if cls is not Struct and issubclass(cls, HasItemSize) + ] + ) + + def build(depth: int) -> np.dtype[np.void]: + num_fields = draw(st.integers(min_value=1, max_value=4)) + # suffix with the index so that names and titles are unique without filtering; titles + # draw from a different alphabet so they never collide with names either + names = [f"{draw(_field_names)}{i}" for i in range(num_fields)] + titles = [f"{draw(_field_titles)}{i}" for i in range(num_fields)] + specs: list[tuple[Any, Any]] = [] + for name, title in zip(names, titles, strict=True): + field_dtype: Any + if depth < max_depth and draw(st.booleans()): + field_dtype = build(depth + 1) + else: + field_dtype = draw(fixed_size_leaves).to_native_dtype() + key: Any = name + if allow_extended and draw(st.booleans()): + key = (title, name) + if allow_extended and draw(st.booleans()): + field_dtype = (field_dtype, draw(npst.array_shapes(max_dims=2, max_side=3))) + specs.append((key, field_dtype)) + align = allow_extended and draw(st.booleans()) + return np.dtype(specs, align=align) + + return build(0) + + def v3_dtypes() -> st.SearchStrategy[np.dtype[Any]]: return dtypes() diff --git a/tests/test_dtype/test_npy/test_structured.py b/tests/test_dtype/test_npy/test_structured.py index 554c3b4e41..7c21528099 100644 --- a/tests/test_dtype/test_npy/test_structured.py +++ b/tests/test_dtype/test_npy/test_structured.py @@ -1,10 +1,12 @@ from __future__ import annotations -from typing import Any +from typing import TYPE_CHECKING, Any import numpy as np import pytest +from numpy.lib.recfunctions import repack_fields +import zarr from tests.test_dtype.test_wrapper import BaseTestZDType from zarr.core.dtype import ( Float16, @@ -14,7 +16,12 @@ Struct, Structured, UInt8, + get_data_type_from_json, ) +from zarr.errors import ZarrUserWarning + +if TYPE_CHECKING: + from zarr.core.common import ZarrFormat class TestStruct(BaseTestZDType): @@ -260,3 +267,115 @@ def test_struct_from_native_dtype() -> None: struct = Struct.from_native_dtype(dtype) assert struct.fields[0][0] == "field1" assert struct.fields[1][0] == "field2" + + +@pytest.mark.filterwarnings("ignore::zarr.errors.UnstableSpecificationWarning") +@pytest.mark.parametrize("zarr_format", [2, 3]) +@pytest.mark.parametrize( + "dtype", + [ + # flat + np.dtype([("a", "i1"), ("b", "i8")]), + # one level of nesting + np.dtype([("a", " None: + """ + A packed (default-layout) structured dtype, flat or nested, round-trips unchanged through + both the JSON form and the native form. + + Nested dtypes are the regression case: `Struct.to_json(zarr_format=2)` emits a nested + field as `[name, [[sub, dt], ...]]`, and the inner V2 type guard used to reject that form, + so Zarr wrote V2 metadata it could not read back. + """ + zdtype = Struct.from_native_dtype(dtype) + recovered = get_data_type_from_json( + zdtype.to_json(zarr_format=zarr_format), zarr_format=zarr_format + ) + assert recovered == zdtype + assert recovered.to_native_dtype() == dtype + assert recovered.to_native_dtype().itemsize == dtype.itemsize + + +@pytest.mark.filterwarnings("ignore::zarr.errors.UnstableSpecificationWarning") +def test_nested_structured_v2_array_round_trip() -> None: + """End-to-end test: write and read a Zarr V2 array with a nested structured dtype.""" + dtype = np.dtype([("a", " None: + """ + Preserve the existing conversion of padded records to packed records, with a warning. + The layout changes, but field values must survive writing and reopening the array. + + The warning is emitted exactly once, even for padding inside a nested field. + """ + data = np.ones(3, dtype=dtype) + data["a"] = [1, 2, 3] + expected = repack_fields(data, recurse=True) + store = zarr.storage.MemoryStore() + with pytest.warns(ZarrUserWarning, match="packed.*layout") as record: + zarr.create_array(store, data=data, chunks=(2,), zarr_format=zarr_format) + layout_warnings = [w for w in record if issubclass(w.category, ZarrUserWarning)] + assert len(layout_warnings) == 1 + reopened = zarr.open_array(store) + assert reopened.dtype == expected.dtype + assert reopened.dtype.itemsize == expected.dtype.itemsize + np.testing.assert_array_equal(reopened[:], expected) + + +def test_titled_structured_dtype_raises() -> None: + """ + The current conversion rejects titles rather than treating their aliases as extra fields. + NumPy's `fields` mapping lists a string title as an extra key. + """ + dtype = np.dtype([(("title", "f0"), "i4"), ("g", "f8")]) + with pytest.raises(ValueError, match="field 'f0' has a title"): + Struct.from_native_dtype(dtype) + + +def test_subarray_structured_dtype_raises() -> None: + """ + The current conversion rejects subarrays rather than resolving them as raw bytes and + dropping their shape and element type. V2's support for subarray metadata is separate. + """ + dtype = np.dtype([("f0", "i4", (2,))]) + with pytest.raises(ValueError, match="field 'f0' is a subarray"): + Struct.from_native_dtype(dtype) diff --git a/tests/test_properties.py b/tests/test_properties.py index 2794ad3cb0..fcb1076441 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -1,11 +1,13 @@ import itertools import json import numbers +import warnings from collections.abc import Generator from typing import Any import numpy as np import pytest +from numpy.lib.recfunctions import repack_fields from numpy.testing import assert_array_equal import zarr @@ -19,8 +21,13 @@ from zarr.abc.store import Store from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZATTRS_JSON +from zarr.core.dtype import get_data_type_from_json, get_data_type_from_native_dtype +from zarr.core.dtype.common import HasItemSize +from zarr.core.dtype.npy.structured import Struct +from zarr.core.dtype.wrapper import ZDType from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata from zarr.core.sync import sync +from zarr.errors import ZarrUserWarning from zarr.testing.strategies import ( array_metadata, arrays, @@ -34,7 +41,9 @@ sharded_arrays, simple_arrays, stores, + structured_dtypes, zarr_formats, + zdtypes, ) @@ -361,6 +370,93 @@ def test_roundtrip_array_metadata_from_json(data: st.DataObject, zarr_format: in assert deep_equal(orig, rt), f"Roundtrip mismatch:\nOriginal: {orig}\nRoundtripped: {rt}" +def _struct_depth(zdtype: ZDType[Any, Any]) -> int: + if not isinstance(zdtype, Struct): + return 0 + return 1 + max(_struct_depth(field_dtype) for _, field_dtype in zdtype.fields) + + +@given(zdtype=zdtypes(), zarr_format=zarr_formats) +@pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning") +def test_zdtype_json_roundtrip(zdtype: ZDType[Any, Any], zarr_format: int) -> None: + """ + Generated built-in data types, including nested structs, round-trip through their JSON + forms for both Zarr formats within the strategy's documented parameter ranges. + + Zarr format 3 data type names do not carry endianness (the bytes codec does), so for that + format the JSON form is compared instead of the data type instance. + """ + event(f"dtype={type(zdtype).__name__}") + event(f"struct_depth={_struct_depth(zdtype)}") + as_json = zdtype.to_json(zarr_format=zarr_format) # type: ignore[arg-type] + roundtripped = get_data_type_from_json(as_json, zarr_format=zarr_format) + assert roundtripped.to_json(zarr_format=zarr_format) == as_json # type: ignore[arg-type] + if zarr_format == 2: + assert roundtripped == zdtype + + +@given(zdtype=zdtypes()) +@pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning") +def test_zdtype_native_roundtrip(zdtype: ZDType[Any, Any]) -> None: + """ + Generated built-in data types round-trip through their native NumPy dtypes within the + strategy's parameter ranges, and reported item sizes match native dtype itemsizes. + + The NumPy object dtype is shared by several Zarr data types, so resolving it is ambiguous by + design and must raise instead. + """ + event(f"dtype={type(zdtype).__name__}") + native = zdtype.to_native_dtype() + if native.kind == "O": + event("native=object") + with pytest.raises(ValueError, match="ambiguous"): + get_data_type_from_native_dtype(native) + return + roundtripped = get_data_type_from_native_dtype(native) + assert roundtripped == zdtype + assert roundtripped.to_native_dtype() == native + if isinstance(zdtype, HasItemSize): + assert zdtype.item_size == native.itemsize + + +def _extended_descr_features(descr: list[Any]) -> set[str]: + """Classify NumPy's serialized field records independently of Zarr's dtype conversion.""" + features = set() + for field in descr: + if isinstance(field[0], tuple): + features.add("title") + if len(field) == 3: + features.add("subarray") + if isinstance(field[1], list): + features.update(_extended_descr_features(field[1])) + return features + + +@given(dtype=structured_dtypes(allow_extended=True)) +@pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning") +def test_structured_dtype_never_silently_changes(dtype: np.dtype[np.void]) -> None: + """ + Generated structured dtypes with titles or subarrays are rejected by the current + conversion. Other generated dtypes preserve their fields, warning on layout changes. + """ + unsupported_features = _extended_descr_features(dtype.descr) + if unsupported_features: + with pytest.raises(ValueError, match="|".join(sorted(unsupported_features))): + get_data_type_from_native_dtype(dtype) + event("outcome=rejected") + return + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", ZarrUserWarning) + zdtype = get_data_type_from_native_dtype(dtype) + event("outcome=accepted") + native = zdtype.to_native_dtype() + assert native == repack_fields(dtype, recurse=True) + layout_warnings = [ + w for w in caught if issubclass(w.category, ZarrUserWarning) and "packed" in str(w.message) + ] + assert bool(layout_warnings) == (native != dtype) + + # @st.composite # def advanced_indices(draw, *, shape): # basic_idxr = draw(