Fix bytes codec endian for big-endian structured dtypes#1047
Open
emfdavid wants to merge 1 commit into
Open
Conversation
The byte order of the array-to-bytes codec was taken from dtype.byteorder, which numpy reports as "|" for every structured dtype no matter what its fields hold. Big-endian record data therefore fell through to the little-endian default, and zarr faithfully decoded the referenced bytes in the byte order it had been told -- the wrong one -- so virtual references to a FITS BinTableHDU (e.g. an SDSS spectrum) yielded silently wrong values. Read the byte order from the fields instead, looking through subarray fields (whose order hides behind .base) and nested structs, and ignoring fields that carry no byte order at all (single-byte numbers, strings) rather than letting them imply little-endian. A structured dtype whose fields mix byte orders now raises: a Zarr V3 array has a single bytes codec and cannot express it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
==========================================
+ Coverage 90.36% 90.41% +0.05%
==========================================
Files 37 37
Lines 2242 2254 +12
==========================================
+ Hits 2026 2038 +12
Misses 216 216
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
convert_to_codec_pipelinechose the array→bytes codec's byte order fromdtype.byteorder.numpy reports
"|"for every structured dtype regardless of what its fields hold, sobig-endian record data fell through to the little-endian
BytesCodec()default. zarr thenfaithfully decodes the referenced bytes in the byte order it was handed — the wrong one — so a
virtual reference to a FITS
BinTableHDU(e.g. an SDSS spectrum) reads back silently wrongvalues. No error, just garbage.
Simple scalar arrays were unaffected (
np.dtype('>f4').byteorder == '>'), which is why thisonly ever surfaced on structured/record dtypes.
Fix
Derive the byte order from the fields rather than from the dtype itself:
dtype.fields, looking through subarray fields (whose order hides behind.base) andnested structs (hence the recursion);
letting them imply little-endian;
ValueError: a Zarr V3 arrayhas a single bytes codec and so cannot express per-field byte order. FITS is uniformly
big-endian by standard, so this doesn't bite the motivating case; happy to downgrade to a
warning if you'd prefer.
numpy gotchas the naive
dtype.byteorder == ">"check misses, all covered by the new helper andtests:
.byteordernp.dtype([('flux','>f4')])np.dtype('<f4')'='— not'<'('>f4', (3,))subarray fieldS8,i1Companion zarr fix
This PR is one half of the story. It makes VirtualiZarr write
endian: bigfor a big-endianstruct; zarr then has to honor it when decoding the referenced bytes. That decode-side branch
for structured dtypes landed in zarr-developers/zarr-python#4142 (merged; ships in zarr
3.2.2). Before that fix, zarr ignored the codec's
endianfor structs, so even acorrectly-tagged array read back wrong. Both halves are needed for a virtual big-endian struct to
read correctly end-to-end.
Testing
test_codecs.pycases covering the dtypes above (incl. the mixed-order raise); the codecpipeline now emits
BytesCodec(endian="big")for a big-endian struct. These pass on releasedzarr 3.2.1 (what CI pins);
BinTableHDU→ kerchunk → VirtualiZarr → Icechunk → native readbyte-identical to astropy, no
.view('>f4')consumer workaround) additionally requires zarr'sdecode fix, i.e. a zarr carrying fix: byte-order handling for structured dtypes in the bytes codec zarr-python#4142 (main today, 3.2.2 on
release).
Sibling to #1037 (the list/base64 struct-ref fix) — same FITS-binary-table motivation.
Heads-up: a separate, pre-existing break (not this PR)
On zarr
main,virtualizarr/utils.py:130doesendian.value, but zarr changedBytesCodec.endianfrom anEndianenum to a plainstr, giving 4test_kerchunk.pyfailures. CI is green only because it pins released zarr; this will surface when zarr 3.2.2
releases. Flagging separately — glad to file an issue or fold a fix in if that's useful.
Acceptance criteria
virtualizarr/tests/test_codecs.pyBinTableHDUend-to-end green locally on released zarr 3.2.1; CI here is authoritative_byte_orders/_is_big_endianhelpers are annotateddocs/about/releases.mddocs/api— n/a,_byte_orders/_is_big_endianare private internal helpers🤖 Generated with Claude Code