diff --git a/changes/4141.bugfix.md b/changes/4141.bugfix.md new file mode 100644 index 0000000000..6a132da3f5 --- /dev/null +++ b/changes/4141.bugfix.md @@ -0,0 +1 @@ +Fix silent byte-order corruption for structured dtypes with the `bytes` codec: multi-byte fields are now byte-swapped to the codec's configured `endian` on write and decoded honoring it on read, so non-native-endian structured data (e.g. big-endian fields, as produced by virtual references to external data) round-trips correctly. diff --git a/src/zarr/codecs/bytes.py b/src/zarr/codecs/bytes.py index 240c077627..fae762fd08 100644 --- a/src/zarr/codecs/bytes.py +++ b/src/zarr/codecs/bytes.py @@ -100,14 +100,28 @@ def _decode_sync( chunk_spec: ArraySpec, ) -> NDBuffer: endian_str = self.endian + dtype = chunk_spec.dtype.to_native_dtype() + # The byte order of the stored data is set by this codec's `endian` + # configuration; the byte order of the decoded array is set by the array's + # data type. The two are independent: the raw bytes are viewed with a dtype + # in the stored byte order, then converted to the declared dtype if needed. if isinstance(chunk_spec.dtype, HasEndianness): - dtype = replace(chunk_spec.dtype, endianness=endian_str).to_native_dtype() # type: ignore[call-arg] + view_dtype = replace(chunk_spec.dtype, endianness=endian_str).to_native_dtype() # type: ignore[call-arg] + elif isinstance(chunk_spec.dtype, Struct) and endian_str is not None: + # Per the struct data type spec, all multi-byte fields are stored in the + # byte order configured on this codec. + view_dtype = dtype.newbyteorder(endian_str) else: - dtype = chunk_spec.dtype.to_native_dtype() + view_dtype = dtype as_array_like = chunk_bytes.as_array_like() chunk_array = chunk_spec.prototype.nd_buffer.from_ndarray_like( - as_array_like.view(dtype=dtype) # type: ignore[attr-defined] + as_array_like.view(dtype=view_dtype) # type: ignore[attr-defined] ) + if view_dtype != dtype: + # This byte-swapping conversion copies the chunk. The dtype inequality + # guard keeps the common case, where the stored and declared byte orders + # already match, on the zero-copy view path above. + chunk_array = chunk_array.astype(dtype) # ensure correct chunk shape if chunk_array.shape != chunk_spec.shape: @@ -129,13 +143,14 @@ def _encode_sync( chunk_spec: ArraySpec, ) -> Buffer | None: assert isinstance(chunk_array, NDBuffer) - if ( - chunk_array.dtype.itemsize > 1 - and self.endian is not None - and self.endian != chunk_array.byteorder - ): + if chunk_array.dtype.itemsize > 1 and self.endian is not None: + # Compare full dtypes rather than the top-level byteorder: numpy reports + # byteorder '|' for structured dtypes even when their fields are + # byte-order-sensitive, so newbyteorder is the only reliable way to + # detect (and normalize) a byte-order mismatch. new_dtype = chunk_array.dtype.newbyteorder(self.endian) - chunk_array = chunk_array.astype(new_dtype) + if new_dtype != chunk_array.dtype: + chunk_array = chunk_array.astype(new_dtype) nd_array = chunk_array.as_ndarray_like() # Flatten the nd-array (only copy if needed) and reinterpret as bytes diff --git a/tests/test_codecs/test_bytes.py b/tests/test_codecs/test_bytes.py index 03dd0b40c6..ead778f526 100644 --- a/tests/test_codecs/test_bytes.py +++ b/tests/test_codecs/test_bytes.py @@ -33,29 +33,50 @@ @pytest.mark.parametrize("store", ["local", "memory"], indirect=["store"]) -@pytest.mark.parametrize("input_dtype", [">u2", "u2", + "f4"), ("mask", ">i4")], + [("flux", "u2", " None: """ The `bytes` codec stores multi-byte data in the byte order configured on the codec, regardless of the input array's byte order, and reads it back to the - original values. The input-dtype/store-endian cross-product exercises the - encode-side byteswap (input byte order != store byte order) and the no-op - case alike. Compression is disabled so the stored chunk is the codec's raw - output and its byte layout can be asserted directly. + original values. For structured dtypes this applies to every multi-byte + field, per the `struct` data type spec; the struct cases guard against the + endianness bugs from + https://github.com/zarr-developers/zarr-python/issues/4141, where the + encode path never byte-swapped struct fields (numpy reports byteorder '|' + for void dtypes) and the decode path ignored the codec's endian entirely. + The input-dtype/store-endian cross-product exercises the encode-side + byteswap (input byte order != store byte order) and the no-op case alike. + Compression is disabled so the stored chunk is the codec's raw output and + its byte layout can be asserted directly. """ - data = np.arange(0, 256, dtype=input_dtype).reshape((16, 16)) + dtype = np.dtype(input_dtype) + if dtype.fields is None: + data = np.arange(0, 256, dtype=dtype).reshape((16, 16)) + else: + data = np.zeros((16, 16), dtype=dtype) + data["flux"] = np.arange(0, 256).reshape((16, 16)) + data["mask"] = np.arange(256, 512).reshape((16, 16)) path = "endian" spath = StorePath(store, path) a = await zarr.api.asynchronous.create_array( spath, shape=data.shape, chunks=(16, 16), - dtype="uint16", + dtype=dtype, fill_value=0, compressors=None, serializer=BytesCodec(endian=store_endian), @@ -66,8 +87,7 @@ async def test_endian( # The stored chunk is laid out in the byte order configured on the codec. stored = await store.get(f"{path}/c/0/0", prototype=default_buffer_prototype()) assert stored is not None - expected_dtype = ">u2" if store_endian == "big" else " None: assert isinstance(BytesCodec(), SupportsSyncCodec) -def test_bytes_codec_sync_roundtrip() -> None: - codec = BytesCodec() - arr = np.arange(100, dtype="float64") +@pytest.mark.parametrize("endian", ENDIAN) +@pytest.mark.parametrize( + "native_dtype", + [np.dtype("float64"), np.dtype(">u2"), np.dtype([("a", ">f4"), ("b", " None: + """ + The synchronous encode/decode path round-trips data, and the two byte + orders involved are independent: the codec's `endian` configuration governs + only the stored byte layout (every multi-byte value, including struct + fields, is laid out in the codec's byte order regardless of the input + array's byte order), while the decoded buffer's byte order is governed by + the array's data type regardless of the codec's. The mixed-endian struct + case pins that per-field byte order of the in-memory dtype survives a + roundtrip through a single stored byte order. + """ + if native_dtype.fields is None: + arr = np.arange(100, dtype=native_dtype) + else: + arr = np.array([(1.5, 2), (3.5, 4), (5.5, 6), (7.5, 8)], dtype=native_dtype) zdtype = get_data_type_from_native_dtype(arr.dtype) spec = ArraySpec( shape=arr.shape, @@ -91,11 +129,14 @@ def test_bytes_codec_sync_roundtrip() -> None: ) nd_buf: NDBuffer = default_buffer_prototype().nd_buffer.from_numpy_array(arr) - codec = codec.evolve_from_array_spec(spec) + codec = BytesCodec(endian=endian).evolve_from_array_spec(spec) encoded = codec._encode_sync(nd_buf, spec) assert encoded is not None + assert encoded.to_bytes() == arr.astype(native_dtype.newbyteorder(endian)).tobytes() + decoded = codec._decode_sync(encoded, spec) + assert decoded.dtype == zdtype.to_native_dtype() np.testing.assert_array_equal(arr, decoded.as_numpy_array())