fix: skip exact byte comparison for gzip codec due to timestamp variance - #4270
Conversation
github-actions
Bot
added
the
needs release notes
|
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. |
|
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). |
|
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4270 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 92 92
Lines 12825 12829 +4
=======================================
+ Hits 12080 12084 +4
Misses 745 745
🚀 New features to boost your workflow:
|
|
thanks! |
… style pass - #310 (#4296) * docs: clarify gzip MTIME handling in comparison helper and fused-pipeline test docs Follow-up to #4270: document that _gzip_streams_equal_except_mtime skips bytes 4-8 (the RFC 1952 MTIME field of the standard 10-byte gzip header), and update the AsyncChunkTransform section comment and test docstring to note that byte-identity is expected except for gzip's MTIME header field. Assisted-by: ClaudeCode:claude-fable-5 * test: style pass on fused-pipeline tests Hoist the imports that nearly every test re-imported locally, add a shared _make_spec helper to replace nine copies of the ArraySpec boilerplate, and convert test_async_chunk_transform_matches_sync to Expect cases where each case declares its expected byte-comparison function (exact equality, or gzip-MTIME-tolerant), removing the isinstance branch from the test body. Also add the missing read-back assertion in test_sync_write_async_read_roundtrip, which previously read into a buffer and never compared it to the written data. Assisted-by: ClaudeCode:claude-fable-5 * test: require a ZDType in _make_spec instead of a dtype string Assisted-by: ClaudeCode:claude-fable-5
2 participants
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.