Skip to content

Commit a82bab5

Browse files
fix(excel): preserve zero padding in clean_excel, as clean_csv already does
pandas.read_excel infers types exactly as read_csv does, so a cell the workbook stored as the TEXT "02134" arrived as the integer 2134 and the padding was gone before any cleaning step ran. clean_csv avoids this with the _csv_io.leading_zero_dtypes pre-scan; clean_excel called pd.read_excel directly and had no equivalent, so preserve_leading_zeros=True -- documented as a shared option on the companion entry point -- changed nothing there. openpyxl cell: value='02134', data_type='s' (genuinely text) clean_excel(...) -> 2134 int64 clean_excel(preserve=True) -> 2134 int64 clean_csv(...) on the same data -> '02134' object The loss was silent: no warning, no report entry. It also cascades, because a postcode or account column read as integers is then profiled and outlier-checked as a quantity, and a clean_csv -> to_excel -> clean_excel hand-off undid padding that clean_csv had just preserved. leading_zero_dtypes_excel is the read_excel counterpart, applying the same rule: every non-missing sampled value must parse as a number and at least one must be zero-padded, so only that column is read as text. Deliberately narrow: - preserve_leading_zeros=False still opts out. - An explicit read_excel_kwargs={"dtype": ...} still wins; the pre-scan stands down rather than fighting the caller. - sheet_name is honoured, so the pre-scan samples the sheet that will be read. - A multi-sheet selection returns no mapping, so clean_excel's own "cleans a single sheet" TypeError still surfaces instead of being masked. - A column without padding keeps its numeric dtype. - Only filesystem paths are pre-scanned, matching the CSV helper, since a buffer cannot be read twice. 4 of the 10 new tests fail on main; the other 6 are the must-not-change controls and pass on both. One test asserts the fixture really stores text, so the regression cannot be blamed on how the workbook was written. Default-output change: a zero-padded numeric column in a spreadsheet now cleans as text instead of losing its padding. Full suite 6718 passed / 0 failed, coverage 93.90%; ruff clean repo-wide.
1 parent 598500a commit a82bab5

4 files changed

Lines changed: 191 additions & 3 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,20 @@ adheres to [Semantic Versioning](https://semver.org/).
77
## [Unreleased]
88

99
### Fixed
10+
- `fd.clean_excel` now preserves zero padding, as `fd.clean_csv` already did.
11+
`pandas.read_excel` infers types exactly as `read_csv` does, so a cell that
12+
the workbook stored as the **text** `"02134"` arrived as the integer `2134`
13+
and the padding was gone before any cleaning step ran. `clean_csv` avoids
14+
this with a bounded pre-scan; `clean_excel` had no equivalent, so
15+
`preserve_leading_zeros=True` — documented as a shared option — changed
16+
nothing there, and a postcode or account column was silently read as a
17+
quantity and then profiled and outlier-checked as one. A `read_excel`
18+
counterpart of the pre-scan now reads only the zero-padded numeric columns as
19+
text. `preserve_leading_zeros=False` still opts out, an explicit
20+
`read_excel_kwargs={"dtype": ...}` still wins, `sheet_name` is honoured, and
21+
columns without padding keep their numeric dtype. **Default-output change:** a
22+
zero-padded numeric column in a spreadsheet now cleans as text rather than
23+
losing its padding.
1024
- An unrecognised `semantic_type` no longer receives *more* text cleaning than
1125
a recognised one. `textclean.config_for_field` matched the declared type
1226
exactly — case-sensitively and untrimmed — and fell through to the caller's

‎src/freshdata/_csv_io.py‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
"""CSV read helpers shared by the CLI and :func:`freshdata.clean_csv`. Internal.
1+
"""Spreadsheet read helpers shared by the CLI, :func:`freshdata.clean_csv` and
2+
:func:`freshdata.clean_excel`. Internal.
23
34
``pandas.read_csv`` infers ``"02134"`` as the integer ``2134`` before any cleaning
45
step runs, so ``CleanConfig.preserve_leading_zeros`` never gets a chance to keep
@@ -67,3 +68,50 @@ def leading_zero_dtypes(
6768
if safe_to_numeric(values, errors="coerce").notna().all():
6869
padded[column] = str
6970
return padded
71+
72+
73+
def leading_zero_dtypes_excel(
74+
path: object,
75+
*,
76+
read_excel_kwargs: Mapping[str, Any] | None = None,
77+
nrows: int = LEADING_ZERO_SCAN_ROWS,
78+
) -> dict[Hashable, type[str]]:
79+
"""``{column: str}`` for numeric-looking spreadsheet columns with zero padding.
80+
81+
``pandas.read_excel`` infers types exactly as ``read_csv`` does, so a cell
82+
that openpyxl stored as the *text* ``"02134"`` still arrives as the integer
83+
``2134`` and the padding is gone before any cleaning step runs. This is the
84+
``read_excel`` counterpart of :func:`leading_zero_dtypes`, and it applies the
85+
same rule: all non-missing sampled values must parse as numbers, and at least
86+
one must be zero-padded.
87+
88+
Returns ``{}`` when the caller already decides types via ``dtype`` or
89+
``converters``, when *path* is not a filesystem path, when the workbook
90+
selects several sheets (there is no single column set to map), or when the
91+
sample cannot be read — the real read then reports that error itself.
92+
"""
93+
kwargs = dict(read_excel_kwargs or {})
94+
if any(kwargs.get(key) is not None for key in _TYPE_OPTIONS):
95+
return {}
96+
if not isinstance(path, (str, os.PathLike)):
97+
return {}
98+
sheet = kwargs.get("sheet_name", 0)
99+
if sheet is None or isinstance(sheet, (list, tuple)):
100+
return {} # several sheets: clean_excel rejects this case anyway
101+
limit = kwargs.get("nrows")
102+
kwargs["nrows"] = nrows if limit is None else min(int(limit), nrows)
103+
try:
104+
sample = pd.read_excel(path, dtype=str, **kwargs)
105+
except (OSError, ValueError, KeyError, ImportError):
106+
return {}
107+
if isinstance(sample, dict): # defensive: sheet_name resolved to many
108+
return {}
109+
110+
padded: dict[Hashable, type[str]] = {}
111+
for position, column in enumerate(sample.columns):
112+
values = sample.iloc[:, position].dropna()
113+
if values.empty or not _has_leading_zero_ids(values):
114+
continue
115+
if safe_to_numeric(values, errors="coerce").notna().all():
116+
padded[column] = str
117+
return padded

‎src/freshdata/api.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import pandas as pd
1111

12-
from ._csv_io import leading_zero_dtypes
12+
from ._csv_io import leading_zero_dtypes, leading_zero_dtypes_excel
1313
from ._reportframe import ReportFrame
1414
from ._util import require_unique_labels, sanitize_csv_formulas
1515
from .adapters.polars import from_pandas, to_pandas
@@ -620,7 +620,12 @@ def clean_excel(
620620
"""
621621
if "report" in options:
622622
return_report = bool(options.pop("report"))
623-
df = pd.read_excel(path, **(read_excel_kwargs or {}))
623+
excel_kwargs = dict(read_excel_kwargs or {})
624+
if _preserve_leading_zeros(config, options):
625+
padded = leading_zero_dtypes_excel(path, read_excel_kwargs=excel_kwargs)
626+
if padded:
627+
excel_kwargs["dtype"] = padded
628+
df = pd.read_excel(path, **excel_kwargs)
624629
if isinstance(df, dict):
625630
raise TypeError(
626631
"clean_excel cleans a single sheet; pass "

‎tests/test_excel_leading_zeros.py‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
"""``clean_excel`` must preserve zero padding, as ``clean_csv`` already does.
2+
3+
``pandas.read_excel`` infers types exactly as ``read_csv`` does, so a cell that
4+
openpyxl stored as the *text* ``"02134"`` arrived as the integer ``2134`` and
5+
the padding was gone before any cleaning step ran. ``clean_csv`` avoids this
6+
with the ``_csv_io.leading_zero_dtypes`` pre-scan; ``clean_excel`` called
7+
``pd.read_excel`` directly and had no equivalent, so
8+
``preserve_leading_zeros=True`` -- documented as a shared option -- changed
9+
nothing there.
10+
11+
The loss was silent: no warning, no report entry, and a postcode column read as
12+
integers then goes on to be profiled and outlier-checked as a quantity.
13+
"""
14+
15+
from __future__ import annotations
16+
17+
import openpyxl
18+
import pandas as pd
19+
import pytest
20+
21+
import freshdata as fd
22+
23+
ZIPS = ["02134", "10001", "94105", "00501", "07030"]
24+
25+
26+
def _workbook(tmp_path, zips=ZIPS, *, sheet="Sheet1"):
27+
"""Write genuine TEXT cells, so the defect cannot be blamed on the file."""
28+
path = tmp_path / "zips.xlsx"
29+
book = openpyxl.Workbook()
30+
sheet_obj = book.active
31+
sheet_obj.title = sheet
32+
sheet_obj.append(["cust", "zip", "qty"])
33+
for row, zip_code in enumerate(zips, start=1):
34+
sheet_obj.cell(row=row + 1, column=1, value=f"c{row}")
35+
cell = sheet_obj.cell(row=row + 1, column=2)
36+
cell.value = zip_code
37+
cell.data_type = "s"
38+
sheet_obj.cell(row=row + 1, column=3, value=row * 10)
39+
book.save(path)
40+
return path
41+
42+
43+
def test_the_workbook_really_stores_text(tmp_path):
44+
"""Guard the guard: if the fixture stored numbers, the rest proves nothing."""
45+
loaded = openpyxl.load_workbook(_workbook(tmp_path))
46+
assert loaded.active["B2"].value == "02134"
47+
assert loaded.active["B2"].data_type == "s"
48+
49+
50+
def test_leading_zeros_survive_clean_excel(tmp_path):
51+
out = fd.clean_excel(_workbook(tmp_path), verbose=False)
52+
assert out["zip"].tolist() == ZIPS
53+
assert out["zip"].dtype == object
54+
55+
56+
def test_only_the_padded_column_is_forced_to_text(tmp_path):
57+
"""A genuine quantity column must keep its numeric dtype."""
58+
out = fd.clean_excel(_workbook(tmp_path), verbose=False)
59+
assert pd.api.types.is_integer_dtype(out["qty"])
60+
61+
62+
def test_the_csv_and_excel_paths_now_agree(tmp_path):
63+
"""The two entry points are documented as companions; they must match."""
64+
csv_path = tmp_path / "zips.csv"
65+
pd.DataFrame({"cust": [f"c{i}" for i in range(1, 6)], "zip": ZIPS}).to_csv(
66+
csv_path, index=False
67+
)
68+
from_csv = fd.clean_csv(csv_path, verbose=False)
69+
from_excel = fd.clean_excel(_workbook(tmp_path), verbose=False)
70+
assert from_csv["zip"].tolist() == from_excel["zip"].tolist() == ZIPS
71+
72+
73+
def test_a_csv_to_excel_hand_off_keeps_the_padding(tmp_path):
74+
"""The end-to-end shape that lost data: clean CSV, store as xlsx, clean again."""
75+
csv_path = tmp_path / "zips.csv"
76+
xlsx_path = tmp_path / "mid.xlsx"
77+
pd.DataFrame({"cust": [f"c{i}" for i in range(1, 6)], "zip": ZIPS}).to_csv(
78+
csv_path, index=False
79+
)
80+
cleaned = fd.clean_csv(csv_path, verbose=False)
81+
pd.DataFrame(cleaned).to_excel(xlsx_path, index=False)
82+
assert fd.clean_excel(xlsx_path, verbose=False)["zip"].tolist() == ZIPS
83+
84+
85+
def test_preserve_leading_zeros_false_still_opts_out(tmp_path):
86+
"""The option must remain an option, not become unconditional behaviour."""
87+
out = fd.clean_excel(_workbook(tmp_path), verbose=False, preserve_leading_zeros=False)
88+
assert pd.api.types.is_integer_dtype(out["zip"])
89+
assert out["zip"].iloc[0] == 2134
90+
91+
92+
def test_an_explicit_dtype_still_wins(tmp_path):
93+
"""The caller decides types when they say so; the pre-scan must stand down."""
94+
out = fd.clean_excel(
95+
_workbook(tmp_path), verbose=False, read_excel_kwargs={"dtype": {"zip": str}}
96+
)
97+
assert out["zip"].tolist() == ZIPS
98+
99+
100+
def test_a_column_without_padding_is_untouched(tmp_path):
101+
"""No false positives: ordinary numbers must not be turned into text."""
102+
out = fd.clean_excel(
103+
_workbook(tmp_path, zips=["12345", "23456", "34567", "45678", "56789"]),
104+
verbose=False,
105+
)
106+
assert pd.api.types.is_integer_dtype(out["zip"])
107+
108+
109+
def test_a_named_sheet_is_pre_scanned_too(tmp_path):
110+
"""The pre-scan must follow sheet_name, or it samples the wrong sheet."""
111+
path = _workbook(tmp_path, sheet="Q3")
112+
out = fd.clean_excel(path, verbose=False, read_excel_kwargs={"sheet_name": "Q3"})
113+
assert out["zip"].tolist() == ZIPS
114+
115+
116+
def test_selecting_several_sheets_still_raises_the_documented_error(tmp_path):
117+
"""The pre-scan must not mask clean_excel's own multi-sheet rejection."""
118+
with pytest.raises(TypeError, match="cleans a single sheet"):
119+
fd.clean_excel(
120+
_workbook(tmp_path), verbose=False, read_excel_kwargs={"sheet_name": None}
121+
)

0 commit comments

Comments
 (0)