Skip to content

[3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency - #154872

Open
pikammmmm wants to merge 1 commit into
python:3.15from
pikammmmm:gh-111487-guess-delimiter-rounding
Open

[3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency#154872
pikammmmm wants to merge 1 commit into
python:3.15from
pikammmmm:gh-111487-guess-delimiter-rounding

Conversation

@pikammmmm

@pikammmmm pikammmmm commented Jul 29, 2026

Copy link
Copy Markdown

Sniffer._guess_delimiter counts a consistency threshold down from 100% and
accepts the first candidate delimiter at or above a documented minimum of 90%:

consistency = 1.0
# minimum consistency threshold
threshold = 0.9
while len(delims) == 0 and consistency >= threshold:
    ...
    consistency -= 0.01

Stepping a float down by 0.01 accumulates rounding error. The counter runs
1.0, 0.99, ... , 0.9199999999999999, 0.9099999999999999 and then reaches
0.8999999999999999, which fails >= 0.9. So the loop makes ten passes
instead 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); total no longer needs to be a float.
The if delims: break at the top preserves the original while len(delims) == 0
guard, which matters because delims persists 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:

modes[char] = (modes[char][0], modes[char][1] - sum(item[1] for item in items))

so that sample scores (9 - 1) / 10 = 80%, which is below the threshold at
every level and is unaffected by the rounding. I checked, and it still raises
Could not determine delimiter with this patch applied. Making that sample work
means 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%), which
corresponds to being modal on 95.0%–95.5% of the rows.

On the earlier objection

@rhettinger wrote on the issue:

We could use exact fractional arithmetic in this section. However, the
threshold of 90% was arbitrary so it likely doesn't make sense to make
accumulation more exact. Also, we should be cautious about making any changes
to guessing or sniffing logic. Code that is currently working but is on the
margins may stop working.

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 at
0.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 rows
with 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 delimiter to a correctly identified
delimiter. 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_consistency builds 20 rows in which ; is
modal on 19 of them, scoring (19 - 1) / 20 = exactly 90%. Every row uses a
different 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:

_csv.Error: Could not determine delimiter

Branch

_guess_delimiter was replaced by the Sniffer rewrite in gh-83273, so this
does not apply to main and is based directly on 3.15. 3.14 and 3.13 are
affected 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.

@pikammmmm
pikammmmm force-pushed the gh-111487-guess-delimiter-rounding branch from 4c342c5 to b517280 Compare July 29, 2026 12:35
@pikammmmm pikammmmm changed the title [3.15] gh-111487: Fix csv.Sniffer rejecting a delimiter at 90% consistency [3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency Jul 29, 2026
@pikammmmm

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant