Skip to content

fix: skip exact byte comparison for gzip codec due to timestamp variance - #4270

Open
sheikhayaan wants to merge 2 commits into
zarr-developers:mainfrom
sheikhayaan:fix/gzip-timestamp-byte-comparison
Open

fix: skip exact byte comparison for gzip codec due to timestamp variance#4270
sheikhayaan wants to merge 2 commits into
zarr-developers:mainfrom
sheikhayaan:fix/gzip-timestamp-byte-comparison

Conversation

@sheikhayaan

Copy link
Copy Markdown

Fixes #4254

The test_async_chunk_transform_matches_sync[bb] test was comparing
raw gzip-encoded bytes between async and sync paths. Since gzip
embeds a wall-clock timestamp in compressed output, two compressions
of the same data at different times produce different bytes even
though the data is identical.

The fix skips the raw byte comparison when a GzipCodec is present
in the codec chain. Round-trip fidelity is still verified by the
decode assertions that follow.

@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 18, 2026
@d-v-b

d-v-b commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for this fix. I think skipping the comparison entirely isn't as good as comparing everything except the timestamp. You could add a new utility routine to the gzip module for taking a gzip encoded stream and returning a timestamp, remainder pair. We would then compare the remainder. That's my first idea but I'm sure there's an even better one.

@d-v-b

d-v-b commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

a marginally better idea: add a private boolean function to the gzip module that compares two gzip streams for "everything but timestamp" equality. There's probably a name for this kind of comparison (comparing two outputs only on the basis of the parameters under control of the producers). internally, we can just compare bytestream a and bytestream b on everything except the four bytes starting at byte 4, which is the mtime field (if I am reading the gzip spec correctly).

@sheikhayaan

Copy link
Copy Markdown
Author

Thanks for the suggestion. I updated the test to compare the gzip streams while ignoring only the 4-byte MTIME field, and added a focused test for the comparison.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.12%. Comparing base (ce10c0b) to head (5fafac8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4270   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files          92       92           
  Lines       12830    12834    +4     
=======================================
+ Hits        12076    12080    +4     
  Misses        754      754           
Files with missing lines Coverage Δ
src/zarr/codecs/gzip.py 95.55% <100.00%> (+0.43%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs release notes Automatically applied to PRs which haven't added release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

avoid comparing exact output of gzip encoding in codec pipeline tests

2 participants