Conversation
…licated columnar decode (LAB-2736) ByteStorage.retrieve() flattened every corruption case (checksum mismatch, decompression bomb/failure, size mismatch) into the same PyValueError as "not a ByteStorage envelope at all", so AutoSerializer.deserialize() could not tell a genuinely corrupted cache entry from bytes that were legitimately written without an envelope (integrity checking off) - a checksum mismatch either fell through to a confusing "not decodable" error or, on the DataFrame/Series path, was duplicated into two near-identical retrieve+decode blocks with their own ad hoc error text. Add EnvelopeIntegrityError (rust/src/python_bindings.rs), a ValueError subclass raised for every ByteStorageError variant except DeserializationFailed (which stays a plain ValueError - the fall-through signal deserialize() depends on). AutoSerializer.deserialize() catches it specifically and re-raises as SerializationError without falling through. Collapse the DataFrame/Series metadata pre-branch and the verified-envelope branch onto the single _decode_columnar path, and drop the dead isinstance(data, dict) branches in _deserialize_dataframe/_deserialize_series now that no caller passes raw bytes. Expert-panel review caught a regression the collapse introduced: an entry written with integrity off and read by an integrity-on reader (same metadata routing to dataframe/series) fell through to the generic msgpack fallback and returned the raw wire dict instead of failing closed or reconstructing - fixed by routing that fallback through _decode_columnar too, with a regression test.
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 86 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Comment |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@kody start-review |
Resolve the LAB-3131 (#289) overlap in the columnar decode path: - src/cachekit/serializers/auto_serializer.py: both sides retired the dead isinstance(data, dict) preamble in _deserialize_dataframe/_deserialize_series; keep main's parameter naming and docstrings. The PR's deserialize() collapse and EnvelopeIntegrityError handling are unchanged. - tests/unit/test_auto_serializer_new_types.py: both sides adapted the four TestColumnarFallbackExtensionDtypes tests to the decoded-document contract; keep the PR's public serialize()/deserialize() round-trip, which routes through main's _decode_columnar.
|
Merged |
1 participant
Summary
This PR introduces a distinct exception type for
ByteStorage.retrieve()verification failures and consolidates duplicated columnar (DataFrame/Series) decode logic in theAutoSerializer.Changes
New
EnvelopeIntegrityErrorexception (Rust layer)EnvelopeIntegrityErrorexception (subclass ofValueError) in the Rust bindings, registered on the Python module with a corrected__module__so it remains picklable across processes.retrieve_error_to_pymapping function that translatesByteStorageErrorvariants into the Python exception taxonomy:DeserializationFailed(bytes that were never a ByteStorage envelope) → plainValueError, preserving the fall-through signal that callers rely on.EnvelopeIntegrityError, which must fail closed.Previously, both "not an envelope" and genuine corruption raised an indistinguishable
ValueError, so the deserializer could not reliably tell them apart.AutoSerializer.deserializebehaviorEnvelopeIntegrityErrorspecifically and re-raises it asSerializationError(fail-closed), rather than falling through to a plain-msgpack/NumPy re-parse of corrupt bytes.Collapsed duplicated columnar decode paths (LAB-2736)
dataframeandseriesbranches so that, when integrity checking is on, they share a single Rust-enveloperetrieve+_decode_columnarpath instead of a duplicated copy.dataframe/seriestype but no envelope was used (integrity off) or the envelope wasn't recognized (cross-config read), the bytes are still routed through_decode_columnarto reconstruct the proper DataFrame/Series — preventing a regression where the raw wire dict would be returned instead._deserialize_dataframeand_deserialize_seriesto accept already-unpacked documents, removing redundant internal msgpack unpacking.Tests
SerializationError, while plain msgpack written with integrity off still falls through and decodes correctly.serialize/deserializeround-trip (forcing the msgpack-columnar path) instead of calling internal helpers directly.