Skip to content

fix(excel): preserve zero padding in clean_excel, as clean_csv already does - #480

Merged
kevincostner17 merged 1 commit into
mainfrom
fix/excel-leading-zeros
Sep 16, 2026
Merged

kevincostner17 merged 1 commit into
mainfrom
fix/excel-leading-zeros

Conversation

@kevincostner17

Copy link
Copy Markdown
Contributor

Summary

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   <- option had no effect
clean_csv(...) on the same data   -> '02134'  object

The loss was silent — no warning, no report entry. It also cascades: a postcode or account column read as integers is then profiled and outlier-checked as a quantity. And a clean_csvto_excelclean_excel hand-off undid padding that clean_csv had just preserved.

Found by the Phase 17 ingestion lane (#478) and reproduced independently before fixing.

The fix

leading_zero_dtypes_excel is the read_excel counterpart, applying the same rule as the CSV helper: 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 actually 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 (no false positives).
  • Only filesystem paths are pre-scanned, matching the CSV helper, since a buffer cannot be read twice.

Verification

  • 10 new tests. 4 fail on main; the other 6 are must-not-change controls and pass on both.
  • One test asserts the fixture really stores text (data_type == "s"), so the regression cannot be blamed on how the workbook was written.
  • Full suite py3.12: 6718 passed, 22 skipped, 0 failed, coverage 93.90%.
  • ruff check . clean repo-wide.

Compatibility

Default-output change: a zero-padded numeric column in a spreadsheet now cleans as text instead of losing its padding.

…y 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.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 0b9e3beb-6c58-4720-828d-da09ed0cf17e


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

FreshData benchmark report — performance

  • freshdata: ?
  • python: ?
  • platform: ?
fixture n_rows n_cols p50 s p95 s peak MB repair % false-repair % preserve % trust monotonic export %

Authored-code reduction (Metric 6)

@kevincostner17
kevincostner17 merged commit ad4a546 into main Sep 16, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant