fix(dtype): write the generic time unit in brackets in Zarr format 2 metadata (zarr-developers#4581)

* fix(dtype): write the generic time unit in brackets in Zarr format 2 metadata

The v2 spec requires the unit of a datetime64 or timedelta64 dtype in
brackets, but a generic-unit time dtype serialized as NumPy's unitless
typestr (<M8), which the spec forbids. The generic unit is now written
explicitly — <M8[generic], or <M8[2generic] with a scale, as scaled
generic units already were — which NumPy parses back to the same dtype.
The unitless spellings stay readable as the generic unit.

Fixes zarr-developers#4580

Assisted-by: ClaudeCode:claude-fable-5

* docs: changelog fragment for #421

Assisted-by: ClaudeCode:claude-fable-5

* refactor(dtype): share the v2 time typestr spelling between the two time types

The generic-unit bracketing moves to TimeDTypeBase._v2_name, which both
to_json implementations call, instead of being duplicated in each.

Assisted-by: ClaudeCode:claude-fable-5

* test(dtype): check v2 JSON alias spellings through the shared wrapper tests

The one-off unitless-time test becomes json_alias_v2_params on the
shared dtype test base: (alias, canonical) pairs asserting that reading
either spelling gives the same dtype and writing gives the canonical
one. The time types list the unitless generic spellings and NumPy's
microsecond alias; other types can list their aliases the same way.

Assisted-by: ClaudeCode:claude-fable-5

Author: d-v-b

changes/421.bugfix.md

@@ -0,0 +1 @@
+The generic time unit of a datetime64 or timedelta64 data type is now written in brackets in Zarr format 2 metadata — `<M8[generic]`, or `<M8[2generic]` with a scale — as the v2 spec requires, instead of NumPy's unitless typestr `<M8`. The unitless spellings stay readable as the generic unit.

src/zarr/core/dtype/npy/time.py

@@ -239,6 +239,16 @@ def __post_init__(self) -> None:
             # Store the NumPy spelling; "μs" stays accepted as input and in stored metadata.
             object.__setattr__(self, "unit", "us")
 
+    def _v2_name(self) -> str:
+        """The v2 typestr: NumPy's spelling, except that the generic unit, which
+        NumPy's `dtype.str` omits, is written in brackets with its scale, because
+        the v2 spec requires a bracketed unit."""
+        name: str = self.to_native_dtype().str
+        if self.unit != "generic":
+            return name
+        scale = "" if self.scale_factor == 1 else self.scale_factor
+        return f"{name}[{scale}generic]"
+
     @classmethod
     def from_native_dtype(cls, dtype: TBaseDType) -> Self:
         """
@@ -497,11 +507,7 @@ def to_json(self, zarr_format: ZarrFormat) -> TimeDelta64JSON_V2 | TimeDelta64JS
             If the zarr_format is not 2 or 3.
         """
         if zarr_format == 2:
-            name: str = self.to_native_dtype().str
-            if self.unit == "generic" and self.scale_factor != 1:
-                # NumPy omits generic scale from dtype.str; preserve it explicitly.
-                name += f"[{self.scale_factor}generic]"
-            return {"name": name, "object_codec_id": None}
+            return {"name": self._v2_name(), "object_codec_id": None}
         elif zarr_format == 3:
             return {
                 "name": self._zarr_v3_name,
@@ -785,11 +791,7 @@ def to_json(self, zarr_format: ZarrFormat) -> DateTime64JSON_V2 | DateTime64JSON
             If the zarr_format is not 2 or 3.
         """
         if zarr_format == 2:
-            name: str = self.to_native_dtype().str
-            if self.unit == "generic" and self.scale_factor != 1:
-                # NumPy omits generic scale from dtype.str; preserve it explicitly.
-                name += f"[{self.scale_factor}generic]"
-            return {"name": name, "object_codec_id": None}
+            return {"name": self._v2_name(), "object_codec_id": None}
         elif zarr_format == 3:
             return {
                 "name": self._zarr_v3_name,

tests/test_dtype/test_npy/test_time.py

@@ -33,10 +33,9 @@ def _native_dtypes(kind: str) -> tuple[np.dtype[np.generic], ...]:
 
 
 def _v2_names(kind: str) -> tuple[DTypeSpec_V2, ...]:
-    # NumPy's spelling, except that a generic scale is written explicitly.
+    # NumPy's spelling, except that the generic unit, which NumPy omits, is
+    # written explicitly: the v2 spec requires a bracketed unit.
     def suffix(unit: str, scale: int) -> str:
-        if unit == "generic" and scale == 1:
-            return ""
         return f"[{unit}]" if scale == 1 else f"[{scale}{unit}]"
 
     return tuple(
@@ -81,6 +80,23 @@ class TestDateTime64(_TestTimeBase):
         np.dtype("timedelta64[ns]"),
     )
     valid_json_v2 = _v2_names("M8")
+    # The legacy unitless spelling reads as the generic unit and writes back
+    # bracketed, as the v2 spec requires (#4580); NumPy's microsecond alias
+    # reads as NumPy's own spelling.
+    json_alias_v2_params = (
+        (
+            {"name": "<M8", "object_codec_id": None},
+            {"name": "<M8[generic]", "object_codec_id": None},
+        ),
+        (
+            {"name": ">M8", "object_codec_id": None},
+            {"name": ">M8[generic]", "object_codec_id": None},
+        ),
+        (
+            {"name": "<M8[μs]", "object_codec_id": None},
+            {"name": "<M8[us]", "object_codec_id": None},
+        ),
+    )
     valid_json_v3 = _v3_configurations("numpy.datetime64")
     invalid_json_v2 = (
         {"name": "datetime64", "object_codec_id": None},
@@ -125,6 +141,21 @@ class TestTimeDelta64(_TestTimeBase):
     )
 
     valid_json_v2 = _v2_names("m8")
+    # See TestDateTime64.json_alias_v2_params.
+    json_alias_v2_params = (
+        (
+            {"name": "<m8", "object_codec_id": None},
+            {"name": "<m8[generic]", "object_codec_id": None},
+        ),
+        (
+            {"name": ">m8", "object_codec_id": None},
+            {"name": ">m8[generic]", "object_codec_id": None},
+        ),
+        (
+            {"name": "<m8[μs]", "object_codec_id": None},
+            {"name": "<m8[us]", "object_codec_id": None},
+        ),
+    )
     valid_json_v3 = _v3_configurations("numpy.timedelta64")
     invalid_json_v2 = (
         {"name": "timedelta64", "object_codec_id": None},

tests/test_dtype/test_wrapper.py

@@ -90,6 +90,11 @@ class BaseTestZDType:
     # pairs. the first element of the pair is used to create a dtype instance, and the second
     # element is the json serialization of the scalar that we want to round-trip.
 
+    # for testing v2 JSON spellings that read as a valid dtype but are not the
+    # spelling zarr writes: (alias json, canonical json) pairs. Reading either
+    # spelling gives the same dtype, and writing it gives the canonical spelling.
+    json_alias_v2_params: ClassVar[tuple[tuple[DTypeSpec_V2, DTypeSpec_V2], ...]] = ()
+
     scalar_v2_params: ClassVar[tuple[tuple[ZDType[Any, Any], Any], ...]] = ()
     scalar_v3_params: ClassVar[tuple[tuple[Any, Any], ...]] = ()
     cast_value_params: ClassVar[tuple[tuple[ZDType[Any, Any], Any, Any], ...]] = ()
@@ -127,6 +132,14 @@ def test_from_json_roundtrip_v3(self, valid_json_v3: DTypeSpec_V3) -> None:
         zdtype = self.test_cls.from_json(valid_json_v3, zarr_format=3)
         assert zdtype.to_json(zarr_format=3) == valid_json_v3
 
+    def test_from_json_alias_v2(
+        self, json_alias_v2_params: tuple[DTypeSpec_V2, DTypeSpec_V2]
+    ) -> None:
+        alias, canonical = json_alias_v2_params
+        zdtype = self.test_cls.from_json(alias, zarr_format=2)
+        assert zdtype == self.test_cls.from_json(canonical, zarr_format=2)
+        assert zdtype.to_json(zarr_format=2) == canonical
+
     def test_from_json_invalid_v2(self, invalid_json_v2: DTypeSpec_V2) -> None:
         with pytest.raises(DataTypeValidationError):
             self.test_cls.from_json(invalid_json_v2, zarr_format=2)