diff --git a/changes/4510.bugfix.md b/changes/4510.bugfix.md new file mode 100644 index 0000000000..e290c62d23 --- /dev/null +++ b/changes/4510.bugfix.md @@ -0,0 +1 @@ +The `cast_value` codec now rejects a fill value that does not survive a round trip through the codec, as the [`cast_value` spec](https://github.com/zarr-developers/zarr-extensions/tree/main/codecs/cast_value#data_type) requires. Previously `out_of_range="clamp"` or `"wrap"`, or rounding, could encode the fill value to something that decodes to a different value, so the unwritten part of a partly written chunk read back as that value instead of the fill value. For example, a `float32` array with fill value `1000.0` and an `int8` target with clamping read `127.0` there. Array creation now raises a `ValueError` that names the fill value, both data types, and the value it decodes to. diff --git a/src/zarr/codecs/cast_value.py b/src/zarr/codecs/cast_value.py index 629b4e7a27..b928657067 100644 --- a/src/zarr/codecs/cast_value.py +++ b/src/zarr/codecs/cast_value.py @@ -175,6 +175,21 @@ def _check_representable( ) from e +def _same_scalar(a: np.ndarray, b: np.ndarray) -> bool: + """Return whether two one-element arrays of the same dtype hold the same value. + + NaN equals NaN, whatever its payload, and the sign of zero must match, as + the `cast_value` spec requires signed zero to be preserved. Comparing bytes + covers both, and works for every permitted dtype, including the + `ml_dtypes` floats. + """ + a_is_nan = bool(a[0] != a[0]) + b_is_nan = bool(b[0] != b[0]) + if a_is_nan or b_is_nan: + return a_is_nan and b_is_nan + return a.tobytes() == b.tobytes() + + # --------------------------------------------------------------------------- # Codec # --------------------------------------------------------------------------- @@ -364,20 +379,60 @@ def _get_scalar_map( def resolve_metadata(self, chunk_spec: ArraySpec) -> ArraySpec: """ Update the fill value of the output spec by applying casting procedure. + + The fill value must survive a round trip: encoding it and decoding the + result must give back the original fill value. Otherwise a chunk that + is only partly written would read back a different value wherever it + holds the fill value. See the ``data_type`` section of the + [`cast_value` spec](https://github.com/zarr-developers/zarr-extensions/tree/main/codecs/cast_value#data_type). + + Raises + ------ + ValueError + If the fill value cannot be cast to the target data type, cannot + be cast back, or does not decode to the original fill value. """ target_zdtype = self.dtype target_native = target_zdtype.to_native_dtype() source_native = chunk_spec.dtype.to_native_dtype() + source_name = chunk_spec.dtype.to_json(zarr_format=3) + target_name = target_zdtype.to_json(zarr_format=3) - fill = chunk_spec.fill_value - fill_arr = np.array([fill], dtype=source_native) + fill_arr = np.array([chunk_spec.fill_value], dtype=source_native) + fill = fill_arr[0] - new_fill_arr = self._do_cast( - fill_arr, target_dtype=target_native, scalar_map=self._get_scalar_map("encode") - ) - new_fill = target_native.type(new_fill_arr[0]) + try: + new_fill_arr = self._do_cast( + fill_arr, target_dtype=target_native, scalar_map=self._get_scalar_map("encode") + ) + except (ValueError, OverflowError) as e: + raise ValueError( + f"The fill value {fill} cannot be cast from {source_name} to {target_name} " + f"by the cast_value codec: {e}" + ) from e + new_fill = new_fill_arr[0] + + try: + decoded_arr = self._do_cast( + new_fill_arr, target_dtype=source_native, scalar_map=self._get_scalar_map("decode") + ) + except (ValueError, OverflowError) as e: + raise ValueError( + f"The fill value {fill} does not survive a round trip through the cast_value " + f"codec: it encodes from {source_name} to {new_fill} in {target_name}, which " + f"cannot be cast back to {source_name}: {e}" + ) from e + decoded = decoded_arr[0] + + if not _same_scalar(fill_arr, decoded_arr): + raise ValueError( + f"The fill value {fill} does not survive a round trip through the cast_value " + f"codec: it encodes from {source_name} to {new_fill} in {target_name}, which " + f"decodes back to {decoded} in {source_name}. Choose a fill value that the " + f"codec's data_type, rounding, out_of_range, and scalar_map preserve." + ) - return replace(chunk_spec, dtype=target_zdtype, fill_value=new_fill) + return replace(chunk_spec, dtype=target_zdtype, fill_value=target_native.type(new_fill)) def _encode_sync( self, diff --git a/tests/test_codecs/test_cast_value.py b/tests/test_codecs/test_cast_value.py index c2e78770d9..6ad38ad000 100644 --- a/tests/test_codecs/test_cast_value.py +++ b/tests/test_codecs/test_cast_value.py @@ -611,3 +611,170 @@ def make_array(filters: list[Any]) -> Any: array[:] = data assert_array_equal(array[:], data) + + +# --------------------------------------------------------------------------- +# Fill value round trip +# --------------------------------------------------------------------------- + + +def _create_with_fill(dtype: str, fill_value: Any, codec: CastValue) -> Any: + """Create a 4-element array with 2-element chunks, filtered through `codec`.""" + return zarr.create_array( + store=MemoryStore(), + shape=(4,), + chunks=(2,), + dtype=dtype, + fill_value=fill_value, + filters=[codec], + serializer=BytesCodec(endian="little"), + compressors=None, + ) + + +@requires_cast_value_rs +@pytest.mark.parametrize( + "case", + [ + Expect( + input=( + "float32", + np.nan, + CastValue( + data_type="int8", scalar_map={"encode": [("NaN", 0)], "decode": [(0, "NaN")]} + ), + ), + output=np.int8(0), + id="nan-through-scalar-map", + ), + Expect( + input=("float64", 5.0, CastValue(data_type="int8", out_of_range="clamp")), + output=np.int8(5), + id="clamp-in-range", + ), + Expect( + input=("float64", -3.0, CastValue(data_type="int8", out_of_range="wrap")), + output=np.int8(-3), + id="wrap-in-range", + ), + Expect( + input=("int16", 300, CastValue(data_type="float32")), + output=np.float32(300.0), + id="int-to-float", + ), + Expect( + input=("float64", 2.0, CastValue(data_type="int8", rounding="towards-zero")), + output=np.int8(2), + id="integral-float-to-int", + ), + Expect( + input=("float64", 1.5, CastValue(data_type="float32")), + output=np.float32(1.5), + id="exact-float-narrowing", + ), + Expect( + input=("float32", -0.0, CastValue(data_type="float16")), + output=np.float16(-0.0), + id="signed-zero-float-to-float", + ), + ], + ids=lambda c: c.id, +) +def test_fill_value_roundtrip_accepted(case: Expect[tuple[str, Any, CastValue], Any]) -> None: + """A fill value that survives encode then decode is accepted. The encoded + fill is stored for the unwritten part of a partly written chunk, and that + part reads back as the original fill value, bit for bit.""" + dtype, fill_value, codec = case.input + arr = _create_with_fill(dtype, fill_value, codec) + arr[0] = 1 + + stored = arr.store._store_dict["c/0"].to_bytes() + expected_encoded = np.array([case.output], dtype=case.output.dtype) + assert stored[expected_encoded.itemsize :] == expected_encoded.tobytes() + + read = arr[1:2] + assert read.tobytes() == np.array([fill_value], dtype=dtype).tobytes() + + +@requires_cast_value_rs +def test_fill_value_partial_chunk_without_check_reads_wrong_value( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression: a fill value that does not round-trip corrupts partly written chunks. + + With the round-trip check disabled, float32 fill 1000.0 encodes to int8 127 + under `out_of_range="clamp"`. The unwritten element of a partly written + chunk is stored as 127 and reads back as 127.0, not as the fill value. The + check refuses this metadata. + """ + import zarr.codecs.cast_value as cast_value_module + + codec = CastValue(data_type="int8", out_of_range="clamp") + + with monkeypatch.context() as m: + m.setattr(cast_value_module, "_same_scalar", lambda a, b: True) + arr = _create_with_fill("float32", 1000.0, codec) + arr[0] = 5.0 + assert_array_equal(arr[:], np.array([5.0, 127.0, 1000.0, 1000.0], dtype="float32")) + + with pytest.raises(ValueError, match=r"decodes back to 127\.0 in float32"): + _create_with_fill("float32", 1000.0, codec) + + +@requires_cast_value_rs +def test_fill_value_clamp_out_of_range_rejected() -> None: + with pytest.raises( + ValueError, + match=r"fill value 1000\.0 .* from float32 to 127 in int8, which decodes back to 127\.0", + ): + _create_with_fill("float32", 1000.0, CastValue(data_type="int8", out_of_range="clamp")) + + +@requires_cast_value_rs +def test_fill_value_wrap_out_of_range_rejected() -> None: + with pytest.raises( + ValueError, + match=r"fill value 300 .* from int16 to 44 in int8, which decodes back to 44 in int16", + ): + _create_with_fill("int16", 300, CastValue(data_type="int8", out_of_range="wrap")) + + +@requires_cast_value_rs +def test_fill_value_lost_to_rounding_rejected() -> None: + with pytest.raises( + ValueError, + match=r"fill value 1\.5 .* from float64 to 2 in int8, which decodes back to 2\.0", + ): + _create_with_fill("float64", 1.5, CastValue(data_type="int8")) + + +@requires_cast_value_rs +def test_fill_value_signed_zero_lost_rejected() -> None: + """The spec requires signed zero to be preserved; an integer target cannot.""" + with pytest.raises( + ValueError, + match=r"fill value -0\.0 .* from float32 to 0 in int8, which decodes back to 0\.0", + ): + _create_with_fill("float32", -0.0, CastValue(data_type="int8")) + + +@requires_cast_value_rs +def test_fill_value_nan_to_int_without_scalar_map_rejected() -> None: + with pytest.raises( + ValueError, + match=r"fill value nan cannot be cast from float32 to int8 .*without scalar_map", + ): + _create_with_fill("float32", np.nan, CastValue(data_type="int8")) + + +@requires_cast_value_rs +def test_fill_value_cannot_decode_rejected() -> None: + """The fill encodes to NaN through the encode map, but with no decode map + NaN cannot be cast back to the integer source.""" + with pytest.raises( + ValueError, + match=r"fill value 0 .* from int32 to nan in float64, which cannot be cast back to int32", + ): + _create_with_fill( + "int32", 0, CastValue(data_type="float64", scalar_map={"encode": [(0, "NaN")]}) + )