Return None for a null row of a dynamic-shape ArrayXD column - #8524
Return None for a null row of a dynamic-shape ArrayXD column#8524Kayvan-Zahiri wants to merge 2 commits into
Conversation
to_pylist ran the column through to_numpy first, where a null row becomes a bare np.nan. With a dynamic first dimension that nan reached the python read path: a lone null row came back as float nan, and a batch mixing a null with a non-null row raised AttributeError on nan.tolist(), so to_dict, to_list, ds[:], iter and filter all failed. The null guard that already handled this for a fixed shape now runs before the numpy conversion, which covers both. PandasArrayExtensionArray.isna hit the same scalar nan from the other side: pd.isna returns a plain bool for it, and bool has no .any(), which took Dataset.to_csv and DataFrame.isna() down with it.
Jokasa7
left a comment
Jokasa7
left a comment
There was a problem hiding this comment.
I found one correctness issue in the pandas-formatting part of this fix. The Python-format change behaves correctly for the tested null-row cases, but the scalar-safe isna reduction still conflates an inner NaN with top-level ArrayXD nullness, so the newly working to_csv path can silently blank a present array value. Details inline.
| return np.array([pd.isna(arr).any() for arr in self._data]) | ||
| # A null row of a dynamic-shape ArrayXD column is a scalar np.nan rather than an | ||
| # array, and the plain bool `pd.isna` returns for it has no `.any()`. | ||
| return np.array([np.any(pd.isna(arr)) for arr in self._data]) |
There was a problem hiding this comment.
[P2] Preserve present ArrayXD rows that contain NaN
For a dynamic/ragged float column, _data is an object array where a top-level null is scalar np.nan but each present row is an ndarray. Reducing pd.isna(arr) with np.any therefore marks a present row missing as soon as one element is NaN. With Array2D((None, 2), "float32") and [[[1.0, np.nan]], None, [[5.0, 6.0]]], Arrow's top-level null mask is [False, True, False], but this line returns [True, True, False]; on this head, to_csv() writes both of the first two cells as empty strings. Since ExtensionArray.isna() is a per-value mask, could the object/ragged path identify only the scalar missing sentinel (or preserve Arrow's validity bitmap) and add an inner-NaN regression test?
There was a problem hiding this comment.
Good catch, that is silent data loss and you are right that isna is a per-value mask. Fixed in 9746085.
Your repro, before and after, against Arrow's own validity:
arrow isna (before) isna (after)
dynamic [F, T, F] [T, T, F] [F, T, F]
csv row 0 was "" now [[ 1. nan]]
The dynamic path is object dtype, where a missing row is a scalar sentinel and present rows are arrays, so it now tests for the sentinel rather than reducing over the row:
if self._data.dtype == object:
return np.array([not isinstance(arr, np.ndarray) and bool(pd.isna(arr)) for arr in self._data])
return np.array([pd.isna(arr).any() for arr in self._data])I left the fixed-shape branch alone deliberately. There _data is a float ndarray and the null row is already materialised as NaNs, so nothing at that point can tell a null row from a row that happens to be all NaN. Preserving the validity bitmap would fix it properly, but that is a wider change than this PR and I would rather not fold it in. Say the word if you want it as a follow-up.
Added test_array_xd_dynamic_shape_isna_keeps_rows_with_inner_nan, which pins isna against column[i].is_valid and asserts the present NaN row survives to_csv. It fails on the previous head.
tests/features + tests/test_table.py: 646 passed, 106 skipped. ruff clean.
isna is a per-value mask, so reducing pd.isna over a present row marked it missing as soon as one element was NaN, and to_csv blanked the cell. On the dynamic-shape path a missing row is a scalar sentinel, so test for that instead. The fixed-shape path is left alone: its null row is materialised as NaNs and the validity bitmap is already gone.
2 participants
ArrayExtensionArray.to_pylistbuilds the python values fromto_numpy, where a null row is a barenp.nan. That is fine for the numpy format but not for the python one, and the guard added in #8363 only covered a fixed first dimension.So for a column like
Array2D(dtype="int32", shape=(None, 2))with aNonerow:The same crash hits
to_list,ds[:],Dataset.iter,mapandfilter. The identical column declared with a fixed shape returnsNonecorrectly. Dynamic first dimensions are a documented feature andtest_array_xd_with_nonealready covers them.The null guard is really about nulls, not about the shape, so it now runs before the numpy conversion and covers both.
PandasArrayExtensionArray.isnamet the same scalar nan from the other side:pd.isnareturns a plain bool for it and bool has no.any(), which brokeDataset.to_csvandDataFrame.isna().Two regression tests added next to the existing ones. The numpy, pandas and arrow formats are unchanged, verified across all twelve ArrayXD value dtypes.