fix(dtypes): dayfirst=True no longer corrupts ISO-8601 dates - #501
Merged
Merged
Conversation
fd.clean(df, dayfirst=True) on ["2021-01-05", "2021-02-11"] silently returned 2021-05-01 and 2021-11-02 -- month and day swapped, with no warning, no report entry and no coercion record. dayfirst exists to resolve ambiguous short dates like 05/12/2021. An ISO-8601 date is YYYY-MM-DD by definition and has no ambiguity to resolve, so this was not a defensible reading of the input. Cause: pandas 2 infers one format for a whole column from its first value, and under dayfirst=True reads an ISO date as %Y-%d-%m. _parse_datetime now passes format="mixed" in that case, so each value is read by its own shape. pandas 1.x infers per value and was never affected -- verified directly on 1.5.3 -- and has no format="mixed", hence the version guard. What made it easy to miss is that the corruption was data-dependent. It was silent only while every day was <= 12: a day >= 13 made the guessed format fail on that value, dropped the parse share below datetime_threshold, and triggered the mixed-format retry that produced the correct reading. The same column therefore read correctly or incorrectly depending on values it happened to contain, or on an unrelated threshold. A user validating on a sample containing a day >= 13 would see correct output and ship. Only the top-level dayfirst=True kwarg was affected. dayfirst="auto" and both semantic_context routes were already correct, which is why this was never caught. Verified the tests fail without the fix rather than assuming: reverting src gives 3 failed / 5 passed. With it, 8 pass on py3.12/pandas 2.3.3 and on py3.9/pandas 1.5.3. dayfirst still resolves genuinely ambiguous slash dates in both directions.
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
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.
The defect
2021-01-052021-05-012021-01-052021-02-112021-11-022021-02-112021-03-092021-09-032021-03-09Month and day swapped, with no warning, no report entry and no coercion record. Silent semantic corruption through a documented public kwarg, on the most standard date format there is.
dayfirstexists to resolve ambiguous short dates such as05/12/2021. An ISO-8601 date isYYYY-MM-DDby definition and has no ambiguity to resolve, so this was not a defensible reading of the input.Why it was easy to miss
The corruption was data-dependent. It was silent only while every day was
<= 12. A day>= 13made the guessed format fail on that value, which dropped the parse share belowdatetime_thresholdand triggered the mixed-format retry that produced the correct reading.Nineteen consecutive January dates, identical data, identical
dayfirst=True:datetime_threshold=0.52021-01-01, 2021-02-01, 2021-03-01 …+ 7NaTSo the same column read correctly or incorrectly depending on values it happened to contain, or on an unrelated knob. A user validating on a sample containing a day
>= 13would see correct output and ship — and the pipeline would then corrupt a month where every day was<= 12.Only the top-level
dayfirst=Truekwarg was affected.dayfirst="auto"and bothsemantic_contextroutes were already correct — which is why this was never caught, since those are the routes previously verified.Cause and fix
pandas 2 infers one format for a whole column from its first value, and under
dayfirst=Truereads an ISO date as%Y-%d-%m:_parse_datetimenow passesformat="mixed"whendayfirstis set, so each value is read by its own shape.pandas 1.x was never affected — verified directly on 1.5.3, which infers per value and returns
2021-01-05. It also has noformat="mixed", hence the guard onPANDAS_MAJOR >= 2. This is a pandas 2 format-inference regression that FreshData inherited.Verification
src/gives 3 failed, 5 passed.ruff check .clean.dayfirststill does its actual job:05/12/2021,06/11/2021→2021-12-05,2021-11-06withdayfirst=True, and2021-05-12,2021-06-11withdayfirst=False. A column mixing ISO and slash forms now reads each by its own shape.Compatibility impact
Output changes for ISO-8601 columns cleaned with
dayfirst=Trueon pandas 2 — from a wrong reading to the correct one.dayfirstbehaviour on genuinely ambiguous slash dates is unchanged, as are thedayfirst="auto"andsemantic_contextroutes. CHANGELOG entry added under[Unreleased] / Fixedstating this.Provenance
Found by mutation testing on
steps/dtypes.py— not as a mutant, but while constructing adversarial inputs to kill one. Searching for inputs that distinguish two near-identical implementations is also a search for inputs the implementation mishandles.