[3.15] gh-109638: Fix excessive backtracking in csv.Sniffer doublequote detection - #154869
Closed
pikammmmm wants to merge 1 commit into
Closed
[3.15] gh-109638: Fix excessive backtracking in csv.Sniffer doublequote detection#154869pikammmmm wants to merge 1 commit into
pikammmmm wants to merge 1 commit into
Conversation
…e detection The doublequote-detection pattern used two greedy character classes that both admitted the quote character, so the engine had to try every way of splitting a run of quotes between them. Excluding the quote from the first class forces the quote that follows it to match at the first one available, which removes the ambiguity while leaving detection intact. The second class must keep admitting the quote, since that is what lets `"a""b"` match -- narrowing both classes, or making the first one atomic while it still admits the quote, silently disables the detection.
Author
|
Closing this in favour of #154868, which solves the same problem more thoroughly: the possessive-quantifier rewrite also detects a doubled quote inside a field that contains the delimiter or a line break, which this patch does not. I hadn't spotted #154865 or #154868 when I opened this — sorry for the duplicate noise. |
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.
csv.Snifferdetects a doubled-quote dialect with a pattern built from twogreedy character classes, both of which admit the quote character:
Because either class can absorb a quote, the engine has to try every way of
splitting a run of quotes between them. On quote-heavy input that blows up:
sniffing the sample from the issue with 60 iterations takes about 9.7 s on 3.15.
Excluding the quote from the first class forces the quote that follows it to
match at the first one available, which removes the ambiguity:
The second class has to keep admitting the quote — that is what lets
"a""b"match, where it consumes
"bso the third quote can land on the closing one.Narrowing both classes, or making the first one atomic while it still admits the
quote (as GH-109639 did), makes the pattern unmatchable and silently disables
detection;
test_doublequotecatches both.This is the change @serhiy-storchaka described in #109638. It is not needed on
main, where these regexes no longer exist after theSnifferrewrite ingh-83273, so this PR is based directly on
3.15. 3.14 and 3.13 are affectedtoo and will get the same fix once this is reviewed.
Measurements
csv.Sniffer().sniff()on the sample from the issue body, run on 3.15.0b1:Growth is still superlinear, as noted in the issue — this narrows the search
rather than replacing the approach — but it is far slower.
Test
test_sniff_regex_backtrackingmirrors the test added onmain, with onedifference: it uses 60 iterations rather than 100.
maincan afford 100 becausethe rewrite made the cost negligible, whereas with this targeted fix 100
iterations still takes about 1.7 s, which is too slow to put in the suite. 60 is
the size quoted in the issue and costs 0.23 s.
Whole-file
test_csv.pyon 3.15.0b1: 0.45 s with the fix, 11.6 swithout it, 135 passing either way — so the new test acts as the same kind of
canary it does on
mainrather than asserting on a timer.Checking the change is behaviour-preserving
Beyond the suite, I compared
sniff()results between patched and unpatchedcsv.pyacross 8726 cases — the delimiter/quote/doublequote/skipinitialspacecombinations,
TestSniffer's own corpus, and 4000 randomized quote-densestrings — with no divergence in the returned dialect or the exceptions raised.
Disclosure: prepared with AI assistance (Claude Code) — used to analyse the
backtracking behaviour, produce the measurements and differential check above,
and draft the patch and this description.