fix(contracts): route to_numeric calls through the shared crash guard - #417
Merged
Merged
Conversation
On pandas < 3, pd.to_numeric reads the exponent digits of a numeric string into a C int with no overflow check, before it rejects trailing text (pandas-dev/pandas#62617). A cell such as "81e3104049863b72" can segfault the process whatever errors= says. enterprise/contracts.py still had four direct calls. The one in _contract_values runs on any non-datetime column with min_value or max_value, so a text column holding such a token reached the parser from enforce_contract and compare_to_baseline(contract=...). The other three (_profile_column, _ks_statistic, _psi_numeric) only see int/float-family columns, but go through the guard too so the file needs no exemption. safe_to_numeric hands input with no unsafe cell to pandas untouched, so results are unchanged. This completes the migration from #409: _DEFERRED in tests/test_numeric.py is now empty and the static guard covers contracts.py. New tests run enforce_contract, build_baseline and compare_to_baseline on crash tokens in a child interpreter behind a to_numeric tripwire, and check parity with raw pd.to_numeric on ordinary frames.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This finishes the
to_numericcrash-guard migration started in #407 and #409.On pandas < 3,
pd.to_numericreads the exponent digits of a numeric string into a C int without checking for overflow, and does so before it rejects trailing text (pandas-dev/pandas#62617). A cell such as"81e3104049863b72"can therefore segfault the process, whatevererrors=is set to. #409 routed every direct call insrc/throughfreshdata._numeric.safe_to_numeric, except the four inenterprise/contracts.py, which were deferred._contract_values: it appliesmin_value/max_valueto columns of any non-datetime dtype. A text column holding such a token could reach the parser throughenforce_contractandcompare_to_baseline(contract=...). It now usessafe_to_numeric._profile_column,_ks_statisticand_psi_numeric: these only ever see int or float columns, but they also use the guard, so the file needs no exemption.safe_to_numericpasses the call to pandas unchanged, so results are identical.Tests
_DEFERREDintests/test_numeric.pyis now empty, so the static guard coversenterprise/contracts.py.test_contract_apis_survive_crash_tokens: runsbuild_baseline,enforce_contractandcompare_to_baselineon object andstringcolumns holding crash tokens. It runs in a child interpreter behind ato_numerictripwire, and checks that the numeric cells are still range-checked.test_contract_calls_match_raw_to_numeric: checks that the baseline and both reports match rawpd.to_numericon ordinary frames, and that all four call sites are exercised._CHILD_TRIPWIREpreamble.Verification
unguarded to_numeric reached '81e3104049863b72'){'enterprise/contracts.py': 4}ruff check .: clean.mypy src/freshdata: no issues.pytest -m "not online and not large": 6257 passed on Python 3.12 / pandas 2.3.3, and 6228 passed on Python 3.9 / pandas 1.5.3.