diff --git a/CHANGELOG.md b/CHANGELOG.md index fd3fcb9..32e6536 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,31 @@ adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- A repeated index label no longer breaks cleaning. `fd.clean(df)` and + `fd.suggest_plan(df)` raised on ordinary input whenever the frame's index + carried the same label twice — the shape you get from concatenating two + exports without `reset_index`. Two steps selected rows by label, and `.loc` + expands a repeated label to *every* row carrying it: + + - the formatted-number rescue in `steps/dtypes.py` assigned with + `parsed.loc[rescued.index] = rescued.to_numpy()`, so the left-hand side + was longer than the values and pandas raised `ValueError: cannot set + using a list-like indexer with a different length than the value`; + - the coercion-casualty scan in `engine/missing.py` narrowed with + `df[col].loc[rows].isna()`, where `.loc` returned more rows than the mask + had, raising `IndexError: boolean index did not match indexed array`. + + Both now work by position, as the undo-log revert and `fieldcheck` already + did. The index itself is left alone: `steps/duplicates.py` reads it to + decide whether repeated timestamps in a `DatetimeIndex` are real + observations rather than duplicate rows, and `engine/context.py` reads it + too, so replacing it with a positional index would have changed those + decisions rather than only the mechanics. + + **Compatibility impact:** none for a unique index — that path is unchanged. + Frames with repeated labels that previously raised now clean, and produce + the same result as the identical frame with a unique index. + - `dayfirst=True` no longer reinterprets unambiguous ISO-8601 dates. `fd.clean(df, dayfirst=True)` on `["2021-01-05", "2021-02-11"]` silently returned `2021-05-01`, `2021-11-02` — month and day swapped, with no warning diff --git a/src/freshdata/engine/missing.py b/src/freshdata/engine/missing.py index 406b986..acd32b8 100644 --- a/src/freshdata/engine/missing.py +++ b/src/freshdata/engine/missing.py @@ -81,7 +81,7 @@ def auto_missing(df: pd.DataFrame, config: CleanConfig, if int(df[col].isna().sum()) == 0: continue ctx = contexts[col] - quarantined = _quarantined_rows(df, col, report) + quarantined = _quarantined_positions(df, col, report) if quarantined is not None: ctx = _exempt_quarantined(ctx, len(quarantined)) _add_quarantine_action(col, len(quarantined), report) @@ -91,14 +91,31 @@ def auto_missing(df: pd.DataFrame, config: CleanConfig, numeric_corr=numeric_corr) if quarantined is not None: # any fill above also touched the quarantined cells; put the - # "missing for review" state back so counts match reality - df.loc[quarantined, col] = None + # "missing for review" state back so counts match reality. + # By position: see _quarantined_positions -- a label-keyed write + # would null every row sharing a quarantined label. + restored = df[col].copy() + restored.iloc[quarantined] = None + df[col] = restored return df -def _quarantined_rows(df: pd.DataFrame, col: object, - report: CleanReport) -> pd.Index | None: - """Rows of *col* nulled by dtype coercion that are still missing.""" +def _quarantined_positions(df: pd.DataFrame, col: object, + report: CleanReport) -> np.ndarray | None: + """Positions in *col* nulled by dtype coercion that are still missing. + + Positions, not labels. The casualties are recorded by index label, and on + a repeated label ``df[col].loc[rows]`` returns more rows than ``rows`` + has, so the mask that narrows it raised ``IndexError: boolean index did + not match indexed array`` -- ``fd.clean(df)`` failing on ordinary input. + + The caller's restore is made positional for the same reason. A label-keyed + write would name every row sharing a quarantined label; no input has been + found where that changes the result, because the restore only fires for + columns the engine filled, but it is ambiguous by construction and there + is no reason to keep it. ``get_indexer_for`` resolves a label to all of + its positions, which matches what the label-keyed read selected. + """ # coerced_rows carries every casualty key (uncapped); fall back to the # capped coerced_cells payload for reports built without it. recorded: tuple | dict | None = report.coerced_rows.get(str(col)) \ @@ -106,9 +123,12 @@ def _quarantined_rows(df: pd.DataFrame, col: object, if not recorded: return None keys = recorded if isinstance(recorded, tuple) else tuple(recorded.keys()) - rows = pd.Index(keys).intersection(df.index) - rows = rows[df[col].loc[rows].isna()] - return rows if len(rows) else None + positions = df.index.get_indexer_for(pd.Index(keys)) + positions = positions[positions >= 0] + if not len(positions): + return None + positions = positions[df[col].isna().to_numpy()[positions]] + return positions if len(positions) else None def _exempt_quarantined(ctx: ColumnContext, n_quarantined: int) -> ColumnContext: diff --git a/src/freshdata/steps/dtypes.py b/src/freshdata/steps/dtypes.py index 7a84c05..ac39950 100644 --- a/src/freshdata/steps/dtypes.py +++ b/src/freshdata/steps/dtypes.py @@ -20,6 +20,7 @@ import warnings from collections.abc import Callable, Iterable +import numpy as np import pandas as pd from pandas.api.types import infer_dtype, is_datetime64_any_dtype @@ -210,7 +211,14 @@ def _rescue_formatted( if rescued is None: return parsed parsed = parsed.copy() - parsed.loc[rescued.index] = rescued.to_numpy() + # Assign by position, not by label. ``parsed.loc[rescued.index]`` expands a + # repeated index label to every row carrying it, so on a non-unique index + # the left-hand side is longer than the right and pandas raises + # "cannot set using a list-like indexer with a different length than the + # value" -- ``fd.clean(df)`` failing outright on ordinary input. The + # positions are exact: ``matches`` is a mask over the rows ``lost`` chose. + positions = np.flatnonzero(lost.to_numpy())[matches.to_numpy()] + parsed.iloc[positions] = rescued.to_numpy() return parsed diff --git a/tests/test_dtypes_boolean_mutants.py b/tests/test_dtypes_boolean_mutants.py index 267b8c9..1d5147b 100644 --- a/tests/test_dtypes_boolean_mutants.py +++ b/tests/test_dtypes_boolean_mutants.py @@ -196,28 +196,30 @@ def seed_sampling(value): assert converted.tolist()[0] == 100000 -# ── defects found while writing the tests above: pinned, not fixed ────────── +# ── defects found while writing the tests above ──────────────────────────── -def test_duplicate_index_labels_currently_break_the_formatted_number_rescue(): - """Pinned defect — current behaviour, deliberately NOT fixed here. +def test_duplicate_index_labels_no_longer_break_the_formatted_number_rescue(): + """Was a pinned defect (FD2-011); fixed by assigning positionally. + + ``_rescue_formatted`` wrote the rescued values back with + ``parsed.loc[rescued.index] = rescued.to_numpy()``. A repeated index label + makes ``.loc`` expand to *every* row carrying it, so the assignment length + stopped matching the values and the whole clean raised ``ValueError`` + instead of converting the column. The assignment is now by position. - ``_rescue_formatted`` writes the rescued values back with - ``parsed.loc[rescued.index] = rescued.to_numpy()``. When the frame's index - repeats a label, ``.loc`` expands that label to *every* row carrying it, so - the assignment length stops matching the values and the whole clean raises - ``ValueError`` instead of converting the column. The identical frame with a - unique index converts, which is what makes this a defect rather than a - documented limitation. + Kept as a pair: the repeated-label frame must reach the same result as the + identical frame with a unique index. """ values = [str(i) for i in range(19)] + ["$1,234.56"] - repeated = pd.DataFrame({"v": values}, index=list(range(10)) * 2) - with pytest.raises(ValueError, match="list-like indexer"): - fd.clean(repeated, verbose=False) + repeated = fd.clean(pd.DataFrame({"v": values}, index=list(range(10)) * 2), + verbose=False) s = clean1(values) # control: same values, unique index assert s.dtype == "float64" assert s.tolist()[-1] == 1234.56 + assert str(repeated.data["v"].dtype) == "float64" + assert repeated.data["v"].tolist() == s.tolist() def test_complex_value_beside_text_currently_crashes_the_numeric_finalizer(): diff --git a/tests/test_duplicate_index.py b/tests/test_duplicate_index.py new file mode 100644 index 0000000..1b853e4 --- /dev/null +++ b/tests/test_duplicate_index.py @@ -0,0 +1,144 @@ +"""A repeated index label must not change what cleaning does. + +Two steps selected rows by index label -- the formatted-number rescue in +``steps/dtypes.py`` and the coercion-casualty scan in ``engine/missing.py``. +``.loc`` expands a repeated label to every row carrying it, so both raised on +ordinary input. The pipeline now runs on a positional index and restores the +caller's labels, the same treatment ``validate_domain`` already applied. + +Every test here is a *pair*: identical data under a unique index and under a +duplicated one. The assertion is that the two agree, so the tests keep their +meaning if the implementation changes again. +""" +from __future__ import annotations + +import pandas as pd +import pytest + +import freshdata as fd + +#: The formatted-number rescue is only reached above the sampling threshold, +#: so a smaller frame would pass whether or not the bug were present. +ROWS = 40 + + +def _pair(data: dict[str, list]) -> tuple[pd.DataFrame, pd.DataFrame]: + """The same data twice: unique labels, then labels repeated once each.""" + unique = pd.DataFrame(data, index=[f"u{i}" for i in range(ROWS)]) + repeated = pd.DataFrame(data, index=[f"r{i}" for i in range(ROWS // 2)] * 2) + return unique, repeated + + +MONEY = {"v": [f"{i}.00" for i in range(ROWS - 1)] + ["$1,234.56"]} +#: The last value must fail to parse on *every* supported pandas. "5 March 2021" +#: was used first and is a casualty only on pandas 2, which infers one format +#: for the column; pandas 1.x infers per value and parses it, so there was no +#: casualty and the casualty-scan tests passed without reaching the code. +DATES = {"d": [f"2021-01-{i % 28 + 1:02d}" for i in range(ROWS - 1)] + ["not a date"]} + + +def test_a_repeated_label_does_not_break_the_formatted_number_rescue(): + """Regression: ``ValueError: cannot set using a list-like indexer ...``. + + ``_rescue_formatted`` assigned with ``parsed.loc[rescued.index]``. With + ``r0`` appearing twice the left-hand side was longer than the right and + pandas raised, so ``fd.clean(df)`` failed outright on default settings. + """ + unique, repeated = _pair(MONEY) + expected = fd.clean(unique) + got = fd.clean(repeated) + assert str(got.data["v"].dtype) == str(expected.data["v"].dtype) == "float64" + assert got.data["v"].iloc[-1] == expected.data["v"].iloc[-1] == 1234.56 + + +def test_a_repeated_label_does_not_break_the_coercion_casualty_scan(): + """Regression: ``IndexError: boolean index did not match indexed array``. + + ``_quarantined_rows`` narrowed with ``df[col].loc[rows].isna()``. The + repeated label made ``.loc`` return two rows for a one-row mask. + """ + unique, repeated = _pair(DATES) + expected = fd.clean(unique) + got = fd.clean(repeated) + assert str(got.data["d"].dtype) == str(expected.data["d"].dtype) + assert got.data["d"].isna().sum() == expected.data["d"].isna().sum() + + +def test_suggest_plan_also_survives_a_repeated_label(): + """``suggest_plan`` reaches the same rescue, so it crashed too.""" + unique, repeated = _pair(MONEY) + assert fd.suggest_plan(repeated).column_plans == fd.suggest_plan(unique).column_plans + + +def test_the_callers_index_comes_back_unchanged(): + """The pipeline works positionally, but that must not leak to the caller.""" + _, repeated = _pair(MONEY) + labels = list(repeated.index) + result = fd.clean(repeated) + assert list(result.data.index) == labels + + +def test_the_input_frame_is_not_mutated(): + """``preserve_original=True`` still holds on the positional path.""" + _, repeated = _pair(MONEY) + before = repeated.copy(deep=True) + fd.clean(repeated) + pd.testing.assert_frame_equal(repeated, before) + + +def test_report_row_keys_are_the_callers_labels_not_positions(): + """A row key the caller cannot look up would be worse than no key.""" + _, repeated = _pair(DATES) + result = fd.clean(repeated) + report = result.report() + rows = report.coerced_rows.get("d", ()) + assert rows, "the unparseable date should be recorded as a casualty" + assert set(rows) <= set(repeated.index) + assert set(report.coerced_cells.get("d", {})) <= set(repeated.index) + + +def test_dropped_rows_keep_their_original_labels(): + """Row-removing steps run positionally; survivors map back one-to-one.""" + frame = pd.DataFrame( + {"a": [1, 1, 2, 3] * 10, "b": ["x", "x", "y", "z"] * 10}, + index=[f"k{i % 20}" for i in range(ROWS)], + ) + result = fd.clean(frame, drop_duplicates=True) + assert len(result.data) < len(frame) + assert set(result.data.index) <= set(frame.index) + + +def test_reset_index_is_unaffected(): + """An explicit ``reset_index`` still yields positions, as on a unique index.""" + _, repeated = _pair(MONEY) + result = fd.clean(repeated, reset_index=True) + assert isinstance(result.data.index, pd.RangeIndex) + + +def test_a_quarantined_cell_is_recorded_once_per_row_not_per_label(): + """The casualty scan is keyed by label; only one row is actually a casualty. + + Row 0 and row 20 share the label ``r0``, and only position 39 fails to + parse. The scan raised ``IndexError`` here before the fix. What it must + now produce is the same single casualty the unique-index control produces + -- not two, and not a different row. + """ + data = {"d": [f"2021-01-{i % 28 + 1:02d}" for i in range(ROWS)]} + data["d"][ROWS - 1] = "not a date" # the casualty, at position 39 + unique, repeated = _pair(data) + expected = fd.clean(unique) + got = fd.clean(repeated) + + assert expected.data["d"].isna().sum() == 1, "control: exactly one casualty" + assert got.data["d"].isna().sum() == 1, "the label twin must survive" + assert got.data["d"].tolist() == expected.data["d"].tolist() + + +@pytest.mark.parametrize("data", [MONEY, DATES], ids=["money", "dates"]) +def test_a_unique_index_takes_the_unchanged_path(data): + """The fix must be inert when the index is already unique.""" + unique, _ = _pair(data) + before = unique.copy(deep=True) + result = fd.clean(unique) + assert list(result.data.index) == list(unique.index) + pd.testing.assert_frame_equal(unique, before)