fix: handle pandas nullable/extension dtypes in no-pyarrow DataFrame fallback#176
Conversation
…fallback
The columnar fallback in AutoSerializer._serialize_dataframe (used when pyarrow is
absent and serializer='auto') crashed on modern pandas dtypes:
- Nullable dtypes (Int64/Float64) have a capitalized dtype.name, so they missed the
`startswith("int")/("float")` numeric branch, fell to the object branch, and
msgpack.packb() raised on the pd.NA sentinels in series.tolist().
- pyarrow-backed dtypes ("int64[pyarrow]") matched startswith("int"), hit the
numeric branch, and raised AttributeError on series.values.tobytes() (the Arrow
extension array has no .tobytes()).
Fix:
- Gate the fast raw-buffer path on a PLAIN numpy numeric dtype
(`not is_extension_array_dtype(dtype) and dtype.kind in {i,u,f}`) — reliable
where the string-name check was not (and now also covers unsigned ints).
- Make the object branch NA-safe: convert scalar pandas NA sentinels (pd.NA/NaT/NaN)
to None so msgpack can pack them; array-like cells are left untouched.
Round-trip through this fallback is intentionally lossy (NA -> null; nullable dtype
not preserved) — exact dtype fidelity is the Arrow path's job. Tests assert no-crash
plus value/null preservation for Int64/Float64/object and pyarrow-backed columns.
Fixes #160
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAutoSerializer's no-pyarrow DataFrame and Series fallback now uses a strict numeric dtype classifier and an NA-safe object-list builder; numeric buffers remain raw bytes, while non-eligible dtypes convert pandas missing sentinels to Python None during serialization. ChangesNullable/extension dtype serialisation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cachekit/serializers/auto_serializer.py (1)
750-754:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_serialize_serieshas the same vulnerability that was fixed in_serialize_dataframe.The Series serialisation path still uses the old
dtype.name.startswith("int"/"float")check and does not convert NA sentinels toNone. This means:
- pyarrow-backed dtypes (e.g.,
int64[pyarrow]) will matchstartswith("int")and crash withAttributeErrorwhen calling.values.tobytes().- Nullable dtypes with NA values fall through to the
tolist()branch but without NA→None conversion, somsgpack.packb()will fail onpd.NAsentinels.Apply the same fix pattern used in
_serialize_dataframe:🐛 Proposed fix to align Series serialisation with DataFrame fix
- if series.dtype.name.startswith("int") or series.dtype.name.startswith("float"): + if not pd.api.types.is_extension_array_dtype(series.dtype) and series.dtype.kind in ("i", "u", "f"): serialized.update({"type": "numeric", "data": series.values.tobytes(), "dtype": str(series.dtype)}) # type: ignore[union-attr] else: - # Use tolist() for object types - preserves datetime objects for custom encoder - serialized.update({"type": "object", "data": series.tolist()}) + # Strings, datetimes, and nullable/extension columns. tolist() preserves + # datetime objects for our custom encoder; scalar pandas NA sentinels + # (pd.NA/NaT/NaN) are converted to None so msgpack can pack them. + values = [] + for v in series.tolist(): + try: + is_na = bool(pd.isna(v)) + except (TypeError, ValueError): + is_na = False # array-like cell (e.g. a list) is not a scalar NA + values.append(None if is_na else v) + serialized.update({"type": "object", "data": values})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cachekit/serializers/auto_serializer.py` around lines 750 - 754, The Series serialization uses dtype.name.startswith(...) and doesn't convert pandas NA sentinels to None; update _serialize_series to mirror the _serialize_dataframe fix: replace the startswith checks with pandas.api.types helpers (e.g., is_integer_dtype/is_float_dtype or is_numeric_dtype) when inspecting series.dtype, and in the non-numeric/object path convert NA sentinels to Python None (e.g., use series.where(series.notna(), None) or series.astype(object).where(pd.notna(series), None) before calling tolist()); for the numeric branch use series.to_numpy().tobytes() (keeping dtype=str(series.dtype)) instead of series.values.tobytes() and handle pyarrow-backed/extension dtypes via the same try/fallback pattern used in _serialize_dataframe so AttributeError from .tobytes() doesn't crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_auto_serializer_new_types.py`:
- Around line 697-708: Add parallel Series tests to mirror the DataFrame tests
so regressions in _serialize_series/_deserialize_series are caught: create two
new test functions in tests/unit/test_auto_serializer_new_types.py —
test_series_nullable_dtypes_roundtrip and
test_series_pyarrow_backed_dtype_does_not_crash — that import pandas (and
pyarrow for the second via pytest.importorskip), instantiate
AutoSerializer(enable_integrity_checking=False), build a nullable Int64 Series
and a pyarrow-backed int64 Series respectively, call ser._serialize_series(...)
then ser._deserialize_series(...), and assert the returned Series preserves
values and nulls (e.g. out.iloc checks and pd.isna) and that pyarrow-backed
roundtrip returns [1,2,3]; this will surface the old dtype-detection bug in
_serialize_series.
---
Outside diff comments:
In `@src/cachekit/serializers/auto_serializer.py`:
- Around line 750-754: The Series serialization uses dtype.name.startswith(...)
and doesn't convert pandas NA sentinels to None; update _serialize_series to
mirror the _serialize_dataframe fix: replace the startswith checks with
pandas.api.types helpers (e.g., is_integer_dtype/is_float_dtype or
is_numeric_dtype) when inspecting series.dtype, and in the non-numeric/object
path convert NA sentinels to Python None (e.g., use series.where(series.notna(),
None) or series.astype(object).where(pd.notna(series), None) before calling
tolist()); for the numeric branch use series.to_numpy().tobytes() (keeping
dtype=str(series.dtype)) instead of series.values.tobytes() and handle
pyarrow-backed/extension dtypes via the same try/fallback pattern used in
_serialize_dataframe so AttributeError from .tobytes() doesn't crash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85c3f152-1909-4776-8537-9710829630c3
📒 Files selected for processing (2)
src/cachekit/serializers/auto_serializer.pytests/unit/test_auto_serializer_new_types.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Addresses review on #176: _serialize_series had the identical bug as _serialize_dataframe (string startswith() dtype check + no NA conversion), so a nullable/pyarrow-backed Series hit the same crash. Extract the logic into two shared module helpers and use them from both paths (DRY): - _is_plain_numpy_numeric(dtype): True only for plain NumPy int/uint/float (excludes pandas nullable/extension dtypes via is_extension_array_dtype). - _na_safe_object_list(series): tolist() with pandas NA sentinels -> None, using the column-level isna() mask (no per-element try/except, so the branch is fully covered) to stay safe on array-like object cells. Adds Series tests (nullable Int64 + pyarrow-backed) mirroring the DataFrame tests.
Summary
Fixes #160. The columnar DataFrame fallback in
AutoSerializer._serialize_dataframe(used only when pyarrow is absent andserializer="auto") crashed on modern pandas dtypes:Int64/Float64) have a capitalizeddtype.name, so they missed thestartswith("int")/("float")numeric branch, fell to the object branch, andmsgpack.packb()raised on thepd.NAsentinels fromseries.tolist().int64[pyarrow]) matchedstartswith("int"), hit the numeric branch, and raisedAttributeErroronseries.values.tobytes()(the Arrow extension array has no.tobytes()).Fix
not pd.api.types.is_extension_array_dtype(dtype) and dtype.kind in {i,u,f}— which is reliable where the string-name check was not (and now also covers unsigned ints).pd.NA/NaT/NaN) toNoneso msgpack can pack them; array-like cells (e.g. a list value) are left untouched.Round-trip through this fallback is intentionally lossy (NA → null; nullable dtype not preserved) — exact dtype fidelity is the Arrow path's job.
Tests
TestColumnarFallbackExtensionDtypes(unit, runs on PRs):Int64/Float64/object columns with NA round-trip without crashing; non-NA values and null positions preserved.int64[pyarrow]column does not crash.Verification
ruffclean,basedpyright0 errors; 83 auto-serializer unit tests pass (no regression) + 2 new.Summary by CodeRabbit
Bug Fixes
Tests