Repository navigation
fix(csv): use following rows when the header holds two delimiters - #4896
sarankumar1325 wants to merge 2 commits into
Conversation
A header line containing more than one candidate delimiter (e.g. a semicolon file with a column named "Weight (kg, net)") is ambiguous on its own, so csv.Sniffer picked the first delimiter in its preference order and split the file on commas. Check such a line against the larger sample and prefer its dialect when it detects another delimiter seen in the header. Resolves docling-project#4895 Signed-off-by: sarankumar1325 <120080287+sarankumar1325@users.noreply.github.com>
|
✅ DCO Check Passed Thanks @sarankumar1325, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
AI triage (advisory)Checked commit The PR changes CI safety: no concern found. A maintainer must still approve CI. The |
There was a problem hiding this comment.
AI first review (advisory): no blocker found
This review covers the full diff at f2fd877a5e. It read the code and did not run it.
The fix correctly disambiguates CSV dialects when the header contains more than one candidate delimiter by re-sniffing the full sample and preferring the result when it finds a delimiter present in the header. Logic, test coverage, and scope are all sound. No blocker found; the human reviewer should confirm that csv.Sniffer reliably picks the true delimiter from a multi-row sample for the two parametrized test cases (it does for these inputs because the data rows are consistent).
A maintainer makes the merge decision. This review never approves the pull request.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
dolfim-ibm
left a comment
There was a problem hiding this comment.
Thanks for this fix, @sarankumar1325! Semicolon files with a comma in a column name are very common in European Excel exports, so this matters a lot.
One blocker: ragged comma files whose header contains another candidate delimiter are now split on that delimiter. When rows miss trailing fields, the comma count varies, so the sniffer prefers a delimiter that occurs once per row, like the : in times:
Date,Time (hh:mm),Value,Note
2024-01-01,10:30,5,a
2024-01-02,11:45,6On main this gives ['Date', 'Time (hh:mm)', 'Value', 'Note']. With this PR it gives ['Date,Time (hh', 'mm),Value,Note']. The same happens with ; in a header such as id,ratio (a;b),x,y.
See the inline suggestion for a fix. Please also add the case above to the parametrized test.
| sample_dialect = csv.Sniffer().sniff(read_sample(), _DELIMITERS) | ||
| except csv.Error: | ||
| return dialect | ||
| if sample_dialect.delimiter in head: |
There was a problem hiding this comment.
Only switch when the sample delimiter splits the header into at least as many fields. This keeps the fix for the issue case and for Name;Size (w, h);Price, and the ragged comma files get the main output again.
| if sample_dialect.delimiter in head: | |
| if head.count(sample_dialect.delimiter) >= head.count(dialect.delimiter): |
…r fields Ragged comma files whose header holds another candidate delimiter, like the `:` in `Time (hh:mm)`, made the sample favour that delimiter. Only switch when the sample delimiter splits the header into at least as many fields, and cover the ragged cases in the parametrized test. Signed-off-by: sarankumar1325 <120080287+sarankumar1325@users.noreply.github.com>
|
Thanks for catching that! I applied your suggestion, so the delimiter now only switches when the sample's delimiter splits the header into at least as many fields. I also added the ragged-comma cases with |
_sniff_dialect()sniffs the CSV dialect from the first line only. When that line holds more than one candidate delimiter, for example a semicolon-separated file with a column namedWeight (kg, net)(common for Excel exports in European locales),csv.Snifferhas no row-to-row consistency to go on and picks the first delimiter in its preference order (,), so every cell is split wrongly and silently.This change keeps the first-line sniff (and its quoted-field fallback) as is, and only when the first line contains two or more of the candidate delimiters, it also sniffs the larger sample. If the sample-based sniff succeeds and its delimiter splits the header into at least as many fields as the first-line delimiter, that dialect is used. Otherwise the first-line dialect is kept, which leaves ragged comma files with a
:or;inside a header field (e.g.Date,Time (hh:mm),Value,Notewith a short row) as onmain. Files whose header has a single candidate delimiter never read the sample, so behaviour there is unchanged.Issue resolved by this Pull Request:
Resolves #4895
Checklist:
_sniff_dialect)Review evidence:
Product;Weight (kg, net);Price\nApple;0,2;1,50\nPear;0,35;2\nconverts to['Product;Weight (kg', 'net);Price', ''],['Apple;0', '2;1', '50'], ...['Product', 'Weight (kg, net)', 'Price'],['Apple', '0,2', '1,50'],['Pear', '0,35', '2'].test_header_with_second_delimiter_uses_following_rowswith the reproducer from the issue (fails onmain, passes with the change) and the mirrored case (comma file with;in the header), which already worked and guards against regressions. Also added the ragged comma cases from review:Date,Time (hh:mm),Value,Noteandid,ratio (a;b),x,ywith short rows, which the first version of this PR split on:/;. Both fail with the earlier condition and pass now. A third ragged case covers the path where the sample can't be sniffed.uv run pytest tests/test_backend_csv.py: 61 passed (includes the CSV reference-data e2e tests; no reference data changed).ruff format/ruff checkclean on the changed files.ty check docling/backend/csv_backend.pyreports one pre-existing warning on an unchanged line (172).This fix was prepared with AI assistance; I reviewed the change and ran the checks above.