GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [3.15] gh-109638: Avoid pathological backtracking in csv.Sniffer (GH-153694) by Punisheroot · Pull Request #154865 · python/cpython Conversation
|
| Version | Time |
|---|---|
| Before | 7.592759 s |
| After, median of 5 runs | 0.187145 s |
This is approximately a 40.6x speedup. Both versions detected , as the
delimiter and left doublequote false for this sample.
TestSniffer: 14 tests passedtest_csv, Debug: 135 tests passedtest_csv, Release: 135 tests passed, 4 skippedTools/patchcheck/patchcheck.py: passed and recognized the NEWS entrygit diff --check: passed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Excluding the quote character from the first run lowers the degree, but the second run can still take each quote either as its own content or as one of the matched quotes, so the search stays polynomial. Measured on the reproducer from the issue:
O(n^5) before, O(n^4) after: a 2 KB sample still takes four and a half minutes, and the test added here takes 1.05 s. #154869 proposed the same change and was closed in favour of #154868, reporting the same limitation. Excluding the quote character from both runs and matching them possessively makes it linear, at the cost of changing results on 42 of 560 files in the CSVsniffer corpora, where this PR changes none. 3.15 is still in beta, so there is also a third option: backporting the new engine from main, which fixes this issue along with seven others. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the analysis. I implemented the quote-exclusion approach suggested in the July 21 issue comment, but I have now reproduced the residual scaling locally on the current 3.15 branch: n=50 took 0.122 s, n=100 1.598 s, and n=150 7.856 s. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
7ba94ca to
a45aca2
Compare
There was an error while loading. Please reload this page.
|
Reopening with a substantially revised implementation at a45aca2. The PR body describes the superseded first implementation; this comment documents the current patch and its validation. Thanks to @serhiy-storchaka for showing that the original change only reduced the pathological search from O(n^5) to O(n^4). The previous implementation and its performance claims have been replaced. Revised designDoublequote detection now has two linear stages. 1. Possessive pre-filterThe first pattern adopts the core approach from #154868:
There are separate patterns for non-empty and empty delimiters. This is necessary because interpolating an empty delimiter into The empty-delimiter pattern instead starts only at 2. Anchored no-doublequote validationAn unanchored local match can begin at a delimiter inside one quoted field and finish in another field. A minimal example is: sample = '",","",","' This is three quoted fields whose values are After a pre-filter match, the revised code therefore checks whether the complete sample is a well-formed sequence of fields containing no doubled quote. If it is, the result is The validator is anchored at Generated-input validationI generated two independent batches of 500,000 semantically labelled samples, using seeds The generator covered comma, semicolon, tab, pipe, space, and the Sniffer's internal empty delimiter; both quote characters; LF, CRLF, and CR; quoted and unquoted fields; embedded delimiters and line breaks; empty fields; leading spaces; doubled quotes; and literal quote characters in unquoted fields. Ground truth came from the semantic values used to construct and escape each field, not from another detector.
The anchored validation is what removes the remaining 742 pre-filter false positives. Additional validation:
The arbitrary-input run is only a robustness check: malformed CSV has no unique semantic ground truth, so it is not included in the accuracy totals. Corpus comparisonI tested the 548 files available in my local CSVsniffer corpus checkout. With newline normalization there were no decision differences from #154868. With raw newlines, three decisions differed: two agreed with the annotations and one did not. I am reporting the latter conservatively as an annotated regression, even though the raw input contains doubled quote characters. PerformanceThe following are post-rebase measurements from a Windows x64 Release build. Each value is a median after warm-up. Original gh-109638 familysample = '"",' * n + '"' * n + '0' + '"' * n + '0'
Both implementations are linear and effectively equivalent on the original reproducer. Empty-delimiter familysample = '"a"\n' + ' ' * n
#154868 is approximately quadratic on this family; the revised implementation is linear. Repeated cross-field false positivesRepeating the valid
The full-sample validation is also linear. Known trade-offWhen the first real doubled quote occurs very late in a 128 KB comma-delimited sample, #154868 measured 1.870 ms and this revision measured 2.497 ms. This is the cost of the additional structural validation; both remain linear. Rejected alternatives
The final two-regexp design avoids those problems while keeping the maintenance-branch scope limited to doubled-quote detection. Post-rebase validation
The new tests cover the gh-109638 reproducer, true and false single-column detection, LF/CRLF/CR boundaries, both quote characters, comma/space/empty delimiters, cross-field false positives, and literal quotes in unquoted fields. This remains a focused 3.15 maintenance-branch fix; it does not backport the complete Sniffer rewrite from |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Spaces after a space delimiter cannot be a padding, so there is no need to skip them at all. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
91680c6 to
d3c62be
Compare
|
Thank you, @serhiy-storchaka. You were right. I applied your suggestion in d3c62be: when the delimiter is a space, the pre-filter no longer consumes a run of spaces as padding. I also added regression tests for both the 100,000-space case and a positive case with consecutive space delimiters. The reported case is now linear. While auditing related inputs, I found a separate pre-existing quadratic path in the first legacy regexp in sample = ('"a", ' * n) + '"b"' For This is not introduced by this PR and should probably be tracked separately rather than expanding the scope of this maintenance fix. I can open a separate issue for it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It's already tracked as #98820. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It's still quadratic with this space delimiter:
import csv import time for n in (10_000, 20_000, 40_000, 80_000): data = '"a" "b"\n' + ' ' * n + 'y "c""d"' t = time.perf_counter() dialect = csv.Sniffer().sniff(data) elapsed = time.perf_counter() - t print(f"n={n:>7,}: {elapsed:8.4f}s " f"delim={dialect.delimiter!r} doublequote={dialect.doublequote}")
main:
n= 10,000: 0.0080s delim=' ' doublequote=True
n= 20,000: 0.0017s delim=' ' doublequote=True
n= 40,000: 0.0033s delim=' ' doublequote=True
n= 80,000: 0.0062s delim=' ' doublequote=True
PR:
n= 10,000: 0.0462s delim=' ' doublequote=True
n= 20,000: 0.1231s delim=' ' doublequote=True
n= 40,000: 0.4762s delim=' ' doublequote=True
n= 80,000: 1.8913s delim=' ' doublequote=True
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| field. The second regexp rules out well-formed input without doubled | ||
| quotes. Both regexps use possessive repetition to avoid backtracking. | ||
| """ | ||
| import re |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This can be lazy import re at the top, along with the other in this file.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| r"[^%(quote)s]*+%(quote)s%(quote)s" | ||
| r"(?:%(quote)s%(quote)s|[^%(quote)s]++)*+" | ||
| r"%(quote)s(?=[\r\n]|\Z)" | ||
| % values, re.MULTILINE) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We can skip this because there's no ^...$?
| % values, re.MULTILINE) | |
| % values) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you for your effort. @Punisheroot, but other PR was merged instead. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.