Repository navigation
Conversation
_generate_choice_re() joins the locale's month names, day names and meridians with "|" and compiles the result, so the data is treated as a pattern rather than as text. Two things go wrong because of that. Several locales abbreviate with a trailing ".", which matches any character. Thai day abbreviations are "จ.", "อ.", "พ.", "พฤ.", "ศ.", "ส." and "อา.", so "พ." matches the "พฤ" of "พฤ.", the capture group comes back as "พฤ", and the name lookup then raises ValueError. Thai cannot parse its own Thursday or Sunday, and a bogus character in that position is accepted by the regex and fails the same way instead of raising ParserError. The alternation also takes the first branch that matches, and some choices are prefixes of others. Vietnamese October, "Tháng Mười", is a prefix of November and December, so where nothing follows the token to force the regex to backtrack, both parse as October and the rest of the input is left unconsumed. Escape each choice and order them longest first.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1379 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 2315 2315
Branches 358 359 +1
=========================================
Hits 2315 2315 ☔ View full report in Codecov by Harness. |
Wkhanex
left a comment
There was a problem hiding this comment.
I checked all 12 full month names and abbreviations in 12 locales (en, fr, de, es, vi, th, ca, hy, lv, mt, ru and hu): 288 public-parser checks resolved the intended month/date. This includes the Vietnamese prefix overlap at the end of the input, where no following token forces a longer alternative.
Fourteen additional choice-pattern checks covered literal regex metacharacters and a shorter-prefix/longer-choice pair; all matched the complete literal spelling. In total, 302 standalone cases passed. I did not run the full upstream suite.
I used Python 3.12.14 against commit 6e7408b. These were focused checks, not a full-suite run.
|
Thanks for checking it so thoroughly. One note for whoever picks this up: the red The same job is the only failure on master at |
Pull Request Checklist
Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:
toxormake testto find out!).tox -e lintormake lintto find out!).masterbranch.If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!
Description of Changes
_generate_choice_re()builds theMMMM,MMM,dddd,ddd,aandApatterns by joining the locale's names with
|and compiling the result:The choices are locale text, but they end up being treated as a pattern. Two
separate things go wrong because of that.
A trailing "." matches any character
Thai day abbreviations are
"จ.","อ.","พ.","พฤ.","ศ.","ส."and
"อา.". In the compiled alternation"พ."matches the"พฤ"of"พฤ.", so the capture group comes back as"พฤ", which is not inday_abbreviations, andparser.pyline 643 raisesValueErrorout oflist.index(). Thai cannot parse its own Thursday or Sunday:The same wildcard also means a character that is not part of any abbreviation
is accepted by the regex and then fails the lookup, so callers get
ValueErrorwhere
ParserErroris the documented failure:Five locales carry a
.in data that reaches this function:caandthinboth
day_abbreviationsandmonth_abbreviations,hyinday_abbreviationsandmeridians,msinmonth_abbreviations, andisinmeridians. The others happen to survive because the wildcard lands on thereal
.and the captured text is still a valid name, so they work by luckrather than by construction.
The alternation takes the first branch that matches
Vietnamese October is
"Tháng Mười", which is a prefix of November"Tháng Mười Một"and December"Tháng Mười Hai". Where something follows thetoken the regex is forced to backtrack and finds the longer name, but where
nothing does, the short branch wins and the remaining input is simply left
unconsumed:
No error, just the wrong month.
"MMMM YYYY"and"YYYY-MMMM-DD"happen tobe correct today, which is why this has not shown up: the trailing literal
forces the backtrack.
The change
Escape each choice, and sort longest first so a prefix cannot shadow a longer
name regardless of what follows the token.
sortedis stable, so choices ofequal length keep their original order and the pattern stays deterministic.
ordinal_day_reis a deliberate pattern and does not go through thisfunction, so it is untouched.
Verification
I formatted and parsed back every locale-text token for all 81 locales:
MMMMandMMMfor all twelve months,ddddanddddfor all sevenweekdays, and
A, which is 3199 round trips. Before the change 7 fail, afterit 3 fail:
and the three
ValueErrors from bogus input becomeParserError.The three that remain are a different defect and I have deliberately left them
alone:
lv,mtandsieach list the same abbreviation for two differentweekdays, so
list.index()can only ever return the first.lvhas"pi"forboth Monday (pirmdiena) and Friday (piektdiena);
mthas"Ħ"for bothThursday (Il-Ħamis) and Sunday (Il-Ħadd);
sihas"බදා"for both Tuesdayand Wednesday, which #1336 already fixes. Those are locale data corrections
rather than a parser bug, so they want their own change — happy to open one for
lvandmtif you would like, and once all three are in it would be worth atest asserting no locale repeats a day abbreviation.
pytest tests/goes from 1901 to 1904 passing, 1 skipped, coverage 99.93%,with no test failing before or after. The three new tests all fail on
masterand pass with the change. Worth noting that my first draft of the Vietnamese
test used
"YYYY-MMMM-DD"and passed onmaster, because the trailing-01forces the backtrack; the test now uses a format where the token is last,
which is what actually reproduces.
On the two unchecked boxes: I could not run
make lint, because it drivespre-commitand neither that norisortinstalls in this environment. I ranblack --check(clean),flake8(clean) andmypy arrow/parser.py, whichreports no new errors — 17 with the change against 20 on
master, allpre-existing. The change touches no imports, so
isorthas nothing to do. Idid not add documentation because
_generate_choice_reis internal and nodocumented behaviour changes; the fix makes the parser match what the docs
already promise. Say the word if you would like a changelog entry.