feat(csv): probe for a dialect instead of checking for one - #666
Conversation
💡 Codex Reviewhttps://github.com/opendocument-app/OpenDocument.core/blob/528cb2f887ef63098dbf28734392ff4ff557b523/output/odr-private/output/pdf/mova-viax-500-user-manual-de.pdf/document.html#L1 This adds 624 generated artifacts under AGENTS.md reference: AGENTS.md:L91-L91 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
c462066 to
ede0212
Compare
528cb2f to
8e08e93
Compare
ede0212 to
cbc058a
Compare
8e08e93 to
e1e29b7
Compare
`check_csv_file` conflated two jobs: deciding whether a file is a csv, and deciding whether it can be parsed. It answered both by reading the *whole* file, and `open_strategy` called it speculatively for every unrecognised text file, so classifying a large one cost a full pass over it. It also hard-coded `,` and refused any file whose records disagreed on a field count. Detection and parsing split. `RecordReader` reads records out of decoded UTF-8 text for a given dialect and judges nothing: ragged records come out ragged, an unterminated quote still yields its field and sets a flag. `probe` scores a bounded sample and returns the dialect it resolved plus a verdict. Scoring tries `,`, `;`, tab and `|`, takes the field count most records carry, and picks the separator explaining the most records. Excel's `sep=` opening line wins outright. The verdict is a heuristic and says so: at least two columns, because one column is every line of prose ever written, and no dangling quote in a file we have all of. Neither is a statement about validity — a one-column csv is a legitimate csv, and once a caller declares a file to be one, it goes straight to `RecordReader`. A sample cut mid-record says nothing about the record it cut, so the last one is dropped unless the sample is the whole file. The sample is decoded first, which is what makes a UTF-16 csv detectable at all — Excel's "Unicode Text" export is UTF-16LE and tab-separated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSgWdLTSLCWDeFvbVwZDVU
e1e29b7 to
7b05289
Compare
🤖 Generated with Claude Code
Second of five stacked PRs. Based on #665 — review that first. Plan:
src/odr/internal/csv/PLAN.md.What was wrong
check_csv_fileconflated two jobs — is this a csv and can this be parsed — and answered both by reading the whole file.open_strategy.cpp:281called it speculatively for every unrecognised text file, so classifying one large.txtcost a full pass over it. It also hard-coded,and", and refused any file whose records disagreed on a field count.The split
RecordReaderreads records out of decoded UTF-8 text for a given dialect and judges nothing. Ragged records come out ragged. An unterminated quote still yields its field and sets a flag. Whether either is a reason to refuse the file is a question for detection, not parsing.probescores a bounded sample and returns the dialect it resolved plus a verdict:,,;, tab,|; takes the field count most records carry; picks the separator explaining the most recordssep=;opening line wins outrightBoth verdict rules are heuristics, not validity rules — one column is every line of prose ever written, and a dangling quote is good evidence of not-csv. Neither says a one-column csv is illegitimate. Once a caller declares a file to be csv (next PR), it goes straight to
RecordReaderand is never asked to pass this.A sample cut mid-record says nothing about the record it cut, so the last record is dropped unless the sample is the whole file. That is why
a_dangling_quote_in_a_complete_file_is_evidence_againstanda_dangling_quote_in_a_sample_is_notare separate tests.The sample is decoded before scanning, which is what makes a UTF-16 csv detectable at all — Excel's "Unicode Text" export is UTF-16LE and tab-separated, so the separator is invisible in the raw bytes.
Tests
The old
csv_file_testassertions move to the probe side, where they belong.a_quoted_field_must_be_terminatedcarried a comment recording a real misclassification; that rule survives, on the detection side only. New coverage for separator scoring, thesep=directive, the complete-vs-sample distinction, andRecordReader's new tolerance.Full suite green (559 unit, 234 reference-output, no drift).