fix(codecs): don't skip outer AA/BB codecs in FusedCodecPipeline partial IO#225
Open
d-v-b wants to merge 2 commits into
Open
fix(codecs): don't skip outer AA/BB codecs in FusedCodecPipeline partial IO#225d-v-b wants to merge 2 commits into
d-v-b wants to merge 2 commits into
Conversation
…ial IO Partial IO delegates the whole chunk to the array->bytes codec, so it is only sound when that codec is the entire pipeline. BatchedCodecPipeline has always required empty AA/BB codec lists for this reason; FusedCodecPipeline passed require_no_aa_bb=False and so took the partial path even with an outer codec wrapping a sharding codec, silently dropping that codec. With filters=[TransposeCodec(order=(1,0))] and a ShardingCodec serializer, the fused pipeline drops the transpose on both encode and decode. It is self-consistent, so a fused-only roundtrip looks correct while the stored bytes do not match the metadata: batched, and any other Zarr implementation, reads that array back transposed. Since require_no_aa_bb now has a single value at every call site, the parameter is gone rather than left as an attractive nuisance. This costs nothing for layouts create_array produces: filters, order= and compressors all nest inside the shard, leaving the outer codec lists empty, so partial IO stays enabled. Only an outer AA/BB codec around a partial-IO-capable codec falls back to the general path, and that layout was returning wrong data. Assisted-by: ClaudeCode:claude-opus-4.8
Assisted-by: ClaudeCode:claude-opus-4.8
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.
🤖 AI text below 🤖
Fixes F2 from the v3.3.0 evaluation — silent wrong data under
FusedCodecPipeline.The bug
FusedCodecPipeline.supports_partial_decode/encodepassedrequire_no_aa_bb=False;BatchedCodecPipelinepassesTrue. Partial IO hands the whole chunk to the array→bytes codec, so it is only sound when that codec is the entire pipeline. With an outer codec wrapping a sharding codec, the fused pipeline still took the partial path and the outer codec was silently skipped.Two corrections to the finding as written
1. It is reachable from
create_array. The evaluation says this "requires explicitcodecs=parameter or metadata from another implementation;create_arraynever produces this layout internally." That isn't right —filtersare applied outside the serializer, so the ordinary documented API produces it:2. It is a write bug, not just a read bug. Each pipeline is self-consistent — fused drops the transpose symmetrically on encode and decode — so a fused-only roundtrip looks perfectly fine. The corruption only surfaces when the data crosses pipelines:
So fused writes bytes that don't match their own metadata. Batched — and any other Zarr implementation — reads that array back transposed. The self-consistency is exactly what makes it silent.
The fix
Pass
require_no_aa_bb=True, matching batched. Since the parameter then has a single value at every call site, it is removed rather than left as an attractive nuisance for the same divergence to reappear. The two# Same condition and dispatch as BatchedCodecPipelinecomments at the dispatch sites become true statements.Cost: none for anything
create_arrayproducescreate_arraynests every AA/BB codec inside the shard, so the outer lists are empty and the predicate is unchanged. Measured on the fused pipeline after this change:shards=shards=+order="F"shards=+filters=shards=+gzipzarr-developers#3885's perf story is untouched. Only the layout that was returning wrong data falls back to the general path — fast-and-silently-wrong traded for correct.
Tests
test_outer_array_array_codec_around_shardingintest_pipeline_parity.py, across all four write/read pipeline pairings. The two cross-pipeline cells fail onmainand pass here; the two same-pipeline cells pass either way, which is the point.Full suite: 6731 passed, 0 failed.
mypyclean.Note on origin
Both this and #224 trace to
fe5030156(zarr-developers#3885). TheFalselooks like a refactoring artifact — the helper andFusedCodecPipelinewere created in that same commit, and the parameter appears to have been introduced to let the new pipeline's behavior coexist with the old one behind a shared helper, rather than as a deliberate relaxation. Worth a look at whether anything else from that refactor made the same assumption.