[3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency - #154872
[3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency#154872pikammmmm wants to merge 1 commit into
Conversation
4c342c5 to
b517280
Compare
|
I've corrected this PR: the earlier description and NEWS entry said it fixed a delimiter "occurring on exactly 90% of the rows", and that was wrong. The score has the non-matching rows subtracted from the mode's count, so 90% of rows scores 80%, and a 90% score needs 95% of rows. The code was right; my description of it was not. I also quoted @rhettinger's "we could use exact fractional arithmetic" without his following sentence, which recommended against making the accumulation more exact. That was careless of me and gave a misleading impression of support. His full comment is quoted in the updated description, and I've tried to answer it directly. The scope section is new and is the important part: this does not make the sample from the issue report work, and I've said so explicitly rather than letting the PR imply otherwise. |
Sniffer._guess_delimitercounts a consistency threshold down from 100% andaccepts the first candidate delimiter at or above a documented minimum of 90%:
Stepping a float down by
0.01accumulates rounding error. The counter runs1.0, 0.99, ... , 0.9199999999999999, 0.9099999999999999and then reaches0.8999999999999999, which fails>= 0.9. So the loop makes ten passesinstead of eleven, and the 90% threshold it documents is never actually
tested — the lowest consistency it really accepts is about 91%.
This counts down in whole percent and compares using exact integer arithmetic
(
v[1] * 100 >= consistency * total);totalno longer needs to be a float.The
if delims: breakat the top preserves the originalwhile len(delims) == 0guard, which matters because
delimspersists across chunks.Scope — worth reading before reviewing
This does not fix the sample in the issue report. That sample is 10 rows in
which the delimiter occurs 30 times on 9 of them and 29 times on the 10th. The
score is not the fraction of matching rows — the mode's count has the
non-matching rows subtracted from it:
so that sample scores
(9 - 1) / 10= 80%, which is below the threshold atevery level and is unaffected by the rounding. I checked, and it still raises
Could not determine delimiterwith this patch applied. Making that sample workmeans changing the scoring or lowering the threshold itself, which is a much
larger behaviour change than I think belongs on a maintenance branch.
What this fixes is narrower: the pass at exactly the documented threshold now
runs. Concretely, it newly accepts delimiters scoring in
[90%, 91%), whichcorresponds to being modal on 95.0%–95.5% of the rows.
On the earlier objection
@rhettinger wrote on the issue:
That caution is fair and I would rather address it than talk past it. The
argument for changing it is that the threshold being arbitrary is what makes the
current state confusing: the constant says
0.9, the code effectively stops at0.91, and the two differ for no stated reason rather than by choice.On the risk to code that currently works: the change is provably
one-directional. Every threshold the old loop tested is strictly greater than
0.90, so anything it accepted the new loop also accepts, at the same level; the
only new behaviour is the extra final pass at exactly 90%. Nothing that
previously matched can stop matching. I also compared patched against unpatched
sniff()results across 12274 samples — threshold sweeps from 10 to 40 rowswith 0–5 inconsistent rows over four delimiters, ordinary well-formed CSV,
TestSniffer's own corpus, and 6000 randomized strings. 16 results changed,every one of them from
Could not determine delimiterto a correctly identifieddelimiter. No sample that already returned a dialect returned a different one,
and none began failing.
If you would still rather not touch the sniffing heuristics on a maintenance
branch at all, I am happy to close this.
Test
test_guess_delimiter_at_minimum_consistencybuilds 20 rows in which;ismodal on 19 of them, scoring
(19 - 1) / 20= exactly 90%. Every row uses adifferent letter so no other character is consistent, and the odd row sits in
the first chunk, which keeps that chunk below the threshold and defers the
decision to the full 20-row chunk. Without the fix it fails with:
Branch
_guess_delimiterwas replaced by theSnifferrewrite in gh-83273, so thisdoes not apply to
mainand is based directly on3.15. 3.14 and 3.13 areaffected as well; I will open those if this one is accepted.
Disclosure: prepared with AI assistance (Claude Code) — used to analyse the
rounding behaviour, construct the threshold test case, run the differential
check above, and draft the patch and this description.