fix(fizarrita): classify codecs by size-preservation, not by compressor name - #9
Conversation
…or name `probeDecompressedSize` decided "is this chunk compressed?" with an allowlist of compressor names (gzip|zlib|blosc|zstd|lz4|bz2|lzma|snappy). Any codec outside that list took the not-compressed branch and returned `rawBytes.byteLength` — the *compressed* length — as the decompressed size, which `inferChunkShape` then takes as fact. For a JPEG 2000 / HTJ2K chunk that is wrong by an order of magnitude or more, and it fails silently: usually the bogus size fails to divide cleanly and the metadata shape is kept, but it can emit spurious `chunk_shape does not match` warnings and, worst case, adopt an inferred shape built from a nonsense number. Inverted to allowlist the codecs known to preserve byte count (`bytes`, `transpose`), so an unrecognised codec counts as size-changing. The two mistakes are not symmetric: calling a size-changing codec size-preserving yields a silently wrong number, while calling a size-preserving codec size-changing costs one decode through the existing fallback and still returns the right answer. The safe default is the one that can only cost time. This also corrects some codecs already in the registry that the old list missed: `crc32c` appends a 4-byte checksum, `scale_offset` and `cast_value` re-type the values, `vlen-utf8` and `json2` are variable-length, and `sharding_indexed` wraps an index plus inner chunks that are usually compressed themselves — all of which previously reported their raw length as their decoded length. Extracted as `hasSizeChangingCodec` and exported so it can be tested directly, following the existing pattern for `readZstdFrameContentSize` and `inferChunkShape`: exposed on `window` in the test app, exercised from `chunk-shape-inference.spec.ts`. v2-style `numcodecs.` prefixes are stripped before lookup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes chunk decompressed-size probing in @fideus-labs/fizarrita by classifying codec chains based on whether they preserve byte count, rather than matching codec names against a compressor allowlist. This makes chunk-shape inference robust to unknown/new codecs (e.g. JPEG2000/HTJ2K) by defaulting to the safe path (full decode) when size preservation can’t be proven.
Changes:
- Introduces and exports
hasSizeChangingCodec, which allowlists only size-preserving codecs (bytes,transpose) and treats all others as size-changing. - Updates
probeDecompressedSizeto use the new size-preservation check instead of a fixed compressor-name list. - Adds browser tests for the new classification behavior and exposes the helper on the test app
window.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
fizarrita/src/get-worker.ts |
Adds hasSizeChangingCodec and switches decompressed-size probing to size-preservation logic. |
fizarrita/src/index.ts |
Re-exports hasSizeChangingCodec as part of the public API surface. |
test/app/main.ts |
Exposes hasSizeChangingCodec on window so Playwright browser tests can call it. |
test/browser/chunk-shape-inference.spec.ts |
Adds Playwright tests covering size-preserving vs size-changing codec classification (including unknown codecs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 3. Check if the raw bytes could be an uncompressed chunk. | ||
| // For the bytes codec (no compression), rawBytes.byteLength IS the | ||
| // decompressed size. We check whether the codec chain is bytes-only | ||
| // (no bytes_to_bytes compression codecs). | ||
| const hasCompression = codecMeta.codecs.some((c) => { | ||
| const name = c.name.toLowerCase() | ||
| // array_to_array codecs (transpose, etc.) don't change byte size | ||
| // array_to_bytes codecs (bytes, etc.) don't compress | ||
| // bytes_to_bytes codecs are the compressors | ||
| return ( | ||
| name === "gzip" || | ||
| name === "zlib" || | ||
| name === "blosc" || | ||
| name === "zstd" || | ||
| name === "lz4" || | ||
| name === "bz2" || | ||
| name === "lzma" || | ||
| name === "snappy" | ||
| ) | ||
| }) | ||
| if (!hasCompression) { | ||
| // No compression codec — raw bytes are the decompressed data | ||
| // When every codec preserves byte count, rawBytes.byteLength IS the | ||
| // decompressed size. |
The step heading still said "uncompressed chunk", which undersells what the branch now accepts: a transpose + bytes chain preserves byte count without being uncompressed in any meaningful sense. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds codec-chain size classification, updates chunk-size probing for size-preserving chains, exports the classifier, and adds browser coverage for codec name normalization and classification. ChangesCodec size classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
fizarrita/src/get-worker.ts:327
- Using a regex in a hot-path-ish predicate creates avoidable overhead/allocations. Prefer a
startsWith('numcodecs.')check withslice(...)aftertoLowerCase()to avoid regex work while keeping the same behavior.
return codecs.some((codec) => {
const name = codec.name.toLowerCase().replace(/^numcodecs\./, "")
return !SIZE_PRESERVING_CODECS.has(name)
})
test/browser/chunk-shape-inference.spec.ts:754
- Inline
import('@playwright/test').Pagemakes the helper signature harder to read and inconsistent with typical typing patterns. Consider adding a top-levelimport type { Page } from '@playwright/test'(or reusing an existing one if present) and usingPagehere.
const classify = (page: import('@playwright/test').Page, names: string[]) =>
test/browser/chunk-shape-inference.spec.ts:780
- These assertions are awaited sequentially, which can slow the suite noticeably as the list grows (similar loops appear below as well). Consider running them concurrently (e.g., map to promises and
await Promise.all(...)) since eachclassifycall is independent.
test('flags the known compressors', async ({ page }) => {
for (const name of ['gzip', 'zlib', 'blosc', 'zstd', 'lz4']) {
expect(await classify(page, ['bytes', name])).toBe(true)
}
})
Hi — found this while wiring fizarrita's chunk cache into SpatialData.ts, where we use
getWorkeras codec offload for OME-Zarr imagery. Happy to adjust anything here to taste, including the test placement.The bug
#5
probeDecompressedSizedecides "is this chunk compressed?" by testing the codec names against a fixed list of compressors:Any codec not on that list takes the not-compressed branch and returns
rawBytes.byteLength— the compressed length — as the decompressed size.inferChunkShapethen takes that number as fact.For JPEG 2000 / HTJ2K imagery that is wrong by an order of magnitude or more, and it fails quietly. Usually the bogus size fails to divide cleanly and
inferChunkShapefalls back to the metadata shape, so nothing visibly breaks — but it can emit spuriouschunk_shape does not matchwarnings, and in the worst case adopt an inferred shape derived from a nonsense number.It is not only exotic codecs. Several already in zarrita's registry are missed by the current list, and all of them currently report their raw length as their decoded length:
crc32cscale_offset,cast_valuevlen-utf8,json2sharding_indexedThe change
Invert the test: allowlist the codecs known to preserve byte count (
bytes,transpose) and treat anything unrecognised as size-changing.The argument for the inversion rather than "add
imagecodecs_jpeg2kand HTJ2K to the list" is that the two mistakes are not symmetric:So the safe default is the one that can only ever cost time — and it stays correct for codecs that don't exist yet, rather than needing a new entry each time one appears.
The classification is extracted as
hasSizeChangingCodecand exported. v2-stylenumcodecs.prefixes are stripped before lookup, matching howreadArrayMetadatabuilds v2 codec entries fromid.Tests
Nine cases added to
chunk-shape-inference.spec.ts, following the existing pattern forreadZstdFrameContentSizeandinferChunkShape— exported, exposed onwindowin the test app, exercised viapage.evaluate. No new tooling.They cover the size-preserving chains, the
numcodecs.prefix and case-insensitivity, the known compressors, the unrecognised-codec case that motivated this (including HTJ2K), the resize-without-compressing codecs in the table above, and sharding.Full suite passes locally: 119/119.
One unrelated note in case it's useful: the suite only ran for me after pointing it at a free port.
playwright.config.tsusesreuseExistingServer: !process.env.CIwith a hardcodedlocalhost:5173, so if anything else is already serving 5173 — very common with Vite — Playwright silently reuses that server and every test fails atwaitForFunction, with no hint as to why. Not touched here since it's orthogonal, but happy to send a follow-up if you'd likestrictPortor a less-common default.🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests