From 4ef426e48edd8a4087efbb93ba0c01aecc0cccf4 Mon Sep 17 00:00:00 2001 From: Vincent La Date: Sat, 10 Oct 2026 12:17:22 -0700 Subject: [PATCH 1/5] Add implementation plan to BOM stripping refactor doc Co-Authored-By: Claude Opus 5.5 --- include/internal/BOMStrippingRefactor.md | 200 ++++++++++++++++++++++- 1 file changed, 192 insertions(+), 8 deletions(-) diff --git a/include/internal/BOMStrippingRefactor.md b/include/internal/BOMStrippingRefactor.md index 5a38ad8c..0b2a9778 100644 --- a/include/internal/BOMStrippingRefactor.md +++ b/include/internal/BOMStrippingRefactor.md @@ -15,6 +15,43 @@ detect a Unicode BOM once, reject unsupported encodings early, strip a UTF-8 BOM before parsing, and feed BOM-free byte views into both serial and speculative parsers. +## Current Behavior + +These results were checked on 2026-10-10 against `master` at `eee1732` with a +probe built from `include/`, on both constructor paths: + +| Scenario | mmap | stream | +|---|---|---| +| UTF-8 BOM, serial parse | stripped, `utf8_bom()` true | stripped, `utf8_bom()` true | +| UTF-8 BOM, speculative parse (4 workers) | stripped, `utf8_bom()` true | stripped, **`utf8_bom()` false** | +| No-header BOM file, first row `raw_str()` | **includes `EF BB BF`** | **includes `EF BB BF`** | +| No-header BOM file, first row `byte_offset()` | 0 | 0 | +| BOM-only file | 0 rows, `utf8_bom()` true | 0 rows, `utf8_bom()` true | + +Where the BOM bytes leak or get misreported: + +- **Stream + speculative `utf8_bom()` bug.** The first stream window is the + 500KB head plus a full read window, so it is larger than `serial_chunk_size` + and goes to `parse_speculative_window()`. A speculative worker core strips the + BOM and records `utf8_bom_` on itself, but `CSVParseOrchestrator::utf8_bom()` + reads `serial_parser_.utf8_bom()`, which never saw the BOM. mmap is not + affected because its first window is the head buffer, which is never larger + than one serial chunk. +- **First row includes the BOM.** `CSVParserCore::parse()` sets + `current_row_start() = 0` *before* `strip_unicode_bom()` advances `data_pos_`. + So the first row's raw span still covers the BOM. Field values are unaffected, + because `field_start_` is computed from `data_pos_`. +- **Core-level scan state is per core instance.** `unicode_bom_scan_` lives on + each `CSVParserCore`: the serial parser, every speculative worker parser, the + repair parser, and each guessing parser. The orchestrator also re-derives "is + this the first window" from `base_offset == 0`. Today that's correct only + because a window that completes no rows makes `CSVReader` throw + "row larger than chunk size" before any rewind to offset 0 can re-parse. + +Format guessing (`parser/guessing.cpp`) builds its own `CSVParserCore` per +candidate delimiter. With the default `scan_bom = true` it strips the BOM, and +it throws on UTF-16/UTF-32 during `CSVReader` construction. + ## Goals - Keep `MmapParser` and `StreamParser` behavior identical. @@ -77,6 +114,17 @@ Add focused tests for both constructor paths using Catch2 `SECTION`s: - Empty file, BOM-only file, and BOM followed by a single row behave consistently. - A large UTF-8 BOM file crossing the 10MB chunk boundary preserves row data and chunk remainder behavior. +- The same large file with speculative parsing forced on + (`speculative_parallel_threads(4)`, `speculative_parallel_min_bytes(0)`) + reports `utf8_bom()` on both paths. This fails on stream input today. +- A no-header BOM file's first row has `raw_str() == "1,2,3"` and + `byte_offset() == 3`. Both fail today. +- A double BOM (`EF BB BF EF BB BF`) strips exactly one BOM, and the second + stays in the first field. This guards against double stripping by the + orchestrator and the core. +- UTF-32 LE (`FF FE 00 00`) reports "UTF-32", not "UTF-16", on both paths. +- `csv::guess_format()` on a UTF-8 BOM file still guesses the delimiter and + header, and still throws on UTF-16. ## Risks @@ -88,12 +136,148 @@ Add focused tests for both constructor paths using Catch2 `SECTION`s: - Tests that assert exact exception strings should use the shared exception message constants/helpers to avoid drift. -## Suggested Implementation Order +## Implementation Plan + +The work splits into two PRs. PR 1 moves ownership and fixes the bugs described +above, while the old core plumbing stays in place but goes unused. PR 2 deletes +that plumbing. Splitting it this way means a regression in PR 1 can be bisected +without the noise from deleting code. + +### PR 1: Orchestrator owns the BOM + +**Step 1: Characterization tests** (`tests/test_read_csv.cpp`, `[read_unicode_bom]`) + +Extend `TEST_CASE("Unicode BOM handling")` with one `SECTION` per constructor +path for every case in the Test Plan above. Use `FileGuard` temp files for mmap +and `std::istringstream` for stream. Build the large-file cases with ≥500K rows +and distinct per-column values, as `tests/AGENTS.md` requires. Check row +contiguity, not just row count. + +Three of these tests fail on `master`: stream + speculative `utf8_bom()`, first +row `raw_str()`, and first row `byte_offset()`. Commit them in the same commit +as Step 2 so every commit stays green. + +**Step 2: First-window BOM scan in `CSVParseOrchestrator`** (`parser/orchestrator.hpp`) + +- Add private state `bool bom_scanned_ = false;` and `bool utf8_bom_ = false;`. +- At the top of `parse_window()`, before the serial/speculative dispatch: + + ```cpp + size_t bom_skip = 0; + if (!this->bom_scanned_) { + bom_skip = get_bom_skip_or_throw(chunk, this->utf8_bom_); + this->bom_scanned_ = true; + } + chunk = chunk.substr(bom_skip); + base_offset += bom_skip; + // ... dispatch exactly as today, using the BOM-free view ... + result.complete_prefix_length += bom_skip; + ``` + +- `utf8_bom()` returns `this->utf8_bom_` instead of `serial_parser_.utf8_bom()`. + This fixes the stream + speculative bug. +- Use a flag rather than `base_offset == 0` for the "first window" test, so a + future rewind to offset 0 can't scan twice. +- Throwing here happens before `parse_speculative_window()` dispatches any + worker tasks, which satisfies the "reject before workers" invariant. +- The scan runs on whatever the first window contains. Both adapters guarantee + that the first window is the head buffer, which holds min(source size, 500KB) + bytes. So a window shorter than 4 bytes only happens when the source is + exhausted. Write this assumption down in a comment and don't add deferral logic. + +Offsets after this change: + +- `RawCSVData::source_start` for the first block becomes `base_offset + 3`. + `CSVRow::byte_offset()` of the first row becomes 3 and `raw_str()` excludes + the BOM. Rows after the first are unchanged, because `source_start` and the + view origin move together. +- `complete_prefix_length` stays in source-window bytes, so `MmapParser`'s + `mmap_pos -= (length - cpl)` and `StreamParser`'s + `leftover_ = chunk.substr(cpl)` / `stream_pos_ += cpl` need no changes. +- In a BOM-only window the parser sees an empty view: no row is produced, + `cpl` is 3, and on exhaustion `end_feed()` has no partial row to flush. + Before relying on this, confirm that `parse_prepared_chunk()` tolerates an + empty `chunk`, which means calling `reserve_for_source_size(0)` and entering + the `parse()` loop with `in.size() == 0`. + +**Step 3: Stop core-level scanning on orchestrated calls** + +- `parse_serial_window()` currently calls the + `parse_chunk(chunk, owner, output, size_t source_start)` overload, which + hard-codes `ParserChunkOptions(initial_state_, true, source_start)`. The core + has no public getter for `initial_state_`, so add + `ParserDFAState initial_state() const noexcept` next to `ending_state()`. + Then call the options overload with + `ParserChunkOptions(serial_parser_.initial_state(), false, base_offset)`. + In PR 2 the `source_start` overload loses its `true` argument along with the + rest of the plumbing. +- `parse_speculative_window()`: pass `false` for `scan_bom_for_first_chunk` + instead of `base_offset == 0`. +- The double-BOM test is what proves this step works: if the core still scans, + it strips the second BOM. + +**Step 4: Format guessing** (`parser/guessing.cpp`) + +In `guess_format(head, delims)`, strip once before the per-delimiter loop: + +```cpp +bool ignored = false; +head = head.substr(get_bom_skip_or_throw(head, ignored)); +``` + +This keeps UTF-16/UTF-32 rejection in the `CSVReader` constructor even after +the core loses its scan. It also scans once per guess instead of once per +candidate delimiter. Until PR 2 the guessing core still scans too, which is +harmless because the head no longer starts with a BOM. + +**Validation for PR 1** + +- Full `ctest` with the default configuration (threads on). +- `-DCSV_ENABLE_THREADS=OFF`, which compiles out the `#if CSV_ENABLE_THREADS` + branches in the orchestrator. +- The generated single header (`generate_single_header` target), compiled + directly. Don't compile `single_include/csv.hpp`. +- C++11 build. `csv::string_view::substr` and the new members must not need + C++14 or later. + +### PR 2: Remove core BOM plumbing + +Delete the following, then `grep -rn "scan_bom\|unicode_bom\|strip_unicode_bom" include tests` +and confirm nothing remains: + +| File | Remove | +|---|---| +| `parser/core.hpp` | `ParserChunkOptions::scan_bom` and its constructor parameter; `scan_bom_for_current_chunk_`, `unicode_bom_scan_`, `utf8_bom_`, `strip_unicode_bom()`, `utf8_bom()`; the reset in `finish_parse()`; the `scan_bom` branch in `parse()`; the `true` argument in the `parse_chunk` overloads | +| `parser/core.hpp` | Move the `get_bom_skip_or_throw` declaration to `parser/driver.hpp`, since the core no longer uses it. The definition stays in `driver.cpp`, and the function stays in `csv::internals` because tests call it there. | +| `parser/driver.hpp` | The `CSVParserCore<>::utf8_bom()` fallback in `CSVParserDriverBase::utf8_bom()`. Return `false` when there is no orchestrator. | +| `speculative/parallel_parser.hpp` | `SpeculativeParseChunk::scan_bom`, the `scan_bom_for_first_chunk` parameter, `result.scan_bom = ...` | +| `speculative/chunks.hpp` | `ParsedChunkRows::scan_bom` and its assignment in `split_parsed_chunk_rows()`; the `scan_bom` arguments at both `ParserChunkOptions` call sites | +| `tests/test_speculative_parser.cpp` | `first.scan_bom` / `second.scan_bom` assignments (two tests) | + +**Hazard:** shrinking `ParserChunkOptions(state, bool scan_bom, size_t source_start)` +to `(state, size_t source_start)` lets a missed two-argument call like +`ParserChunkOptions(state, true)` compile silently as `source_start = 1`. Before +deleting the parameter, temporarily mark the old 3-argument constructor +`= delete`, or rename it, and let the compiler list every call site. + +### Documentation (in PR 2) + +- `include/internal/ARCHITECTURE.md`: remove "BOM handling" from the + `CSVParserCore` bullet and add it to the orchestrator's responsibilities. +- `include/internal/JOURNEY_OF_A_CSV_FIELD.md`: remove "UTF-8 BOM skip" from the + core's list and the `scan_bom` lines from the speculative chunk diagram. Add a + "first window: BOM scan/strip" step before dispatch. +- This page: replace "Current Behavior" with a short "Resolved" note. Keep + Offset Invariants as durable reference material. +- The PR description must call out the user-visible changes: the first row's + `raw_str()` and `byte_offset()` move past the BOM, and `utf8_bom()` is now + correct for speculative stream input. `README.md` needs no change. + +### Out of Scope -1. Add tests that document the current expected behavior. -2. Add orchestrator-level BOM scan state and result adjustment. -3. Disable parser-core BOM scanning from orchestrated parse calls. -4. Remove obsolete `scan_bom` plumbing only after tests prove it is no longer - needed by serial or speculative paths. -5. Update `include/internal/ARCHITECTURE.md` if ownership of BOM handling moves - from `CSVParserCore` to the orchestrator. +- An mmap source whose first row is longer than the 500KB head buffer, with a + larger `chunk_size()`, silently yields zero rows and no column names. This + happens with or without a BOM, and stream input handles the same file + correctly. Track it separately. Once it's fixed, the + `complete_prefix_length += bom_skip` adjustment from Step 2 is what keeps the + re-read window from parsing the BOM as data. From 58469b4acee9764db7058f4c070f03d740de2515 Mon Sep 17 00:00:00 2001 From: Vincent La Date: Sat, 10 Oct 2026 12:28:36 -0700 Subject: [PATCH 2/5] Move BOM detection and stripping into CSVParseOrchestrator CSVParseOrchestrator now scans the first source window for a Unicode BOM, rejects UTF-16/UTF-32 before any serial or speculative parser runs, and hands parsers a BOM-free view. The skipped bytes are reported back through CSVParseWindowResult so source adapters keep speaking in source offsets. Format guessing strips the BOM itself once per guess. This fixes: - utf8_bom() returning false for stream input whose first window went through the speculative parser (only a worker core saw the BOM). - The first row's raw_str() including the BOM bytes and byte_offset() pointing at the BOM instead of the row. - The BOM leaking into the first column name when the mmap head window completes no row and is re-read from offset 0 (exposed by #337). MmapParser now asks the parse result whether the head completed a row instead of comparing positions, since a skipped BOM advances mmap_pos. Co-Authored-By: Claude Opus 5.5 --- include/internal/parser/core.hpp | 11 +- include/internal/parser/guessing.cpp | 6 + include/internal/parser/mmap.cpp | 12 +- include/internal/parser/mmap.hpp | 5 +- include/internal/parser/orchestrator.hpp | 49 +++- tests/test_edge_cases_large_rows.cpp | 40 ++++ tests/test_read_csv.cpp | 270 +++++++++++++++++++++++ 7 files changed, 383 insertions(+), 10 deletions(-) diff --git a/include/internal/parser/core.hpp b/include/internal/parser/core.hpp index 1a7c298b..fe3f8e63 100644 --- a/include/internal/parser/core.hpp +++ b/include/internal/parser/core.hpp @@ -161,7 +161,16 @@ namespace csv { }; struct CSVParseWindowResult { + /** Source bytes fully consumed by this window, including skipped_prefix_length. */ size_t complete_prefix_length = 0; + + /** Leading source bytes consumed without being parsed (a UTF-8 BOM). */ + size_t skipped_prefix_length = 0; + + /** Whether this window produced at least one complete row. */ + bool completed_row() const noexcept { + return this->complete_prefix_length > this->skipped_prefix_length; + } }; } @@ -485,7 +494,7 @@ namespace csv { chunk, std::move(owner), output, - ParserChunkOptions(this->initial_state_, true, source_start) + ParserChunkOptions(this->initial_state_, false, source_start) ); } diff --git a/include/internal/parser/guessing.cpp b/include/internal/parser/guessing.cpp index 96001f7b..1d0441b8 100644 --- a/include/internal/parser/guessing.cpp +++ b/include/internal/parser/guessing.cpp @@ -76,6 +76,12 @@ namespace csv { * Header detection: If first row has >= columns than mode, use row 0. * Otherwise use the first row with the mode length. */ + // The parser core does not handle BOMs, so strip one here (once, not + // per candidate). This also rejects UTF-16/UTF-32 before guessing + // scores mis-decoded bytes. + bool utf8_bom = false; + head = head.substr(get_bom_skip_or_throw(head, utf8_bom)); + CSVFormat format; size_t max_score = 0; size_t header = 0; diff --git a/include/internal/parser/mmap.cpp b/include/internal/parser/mmap.cpp index feb14ce5..66095818 100644 --- a/include/internal/parser/mmap.cpp +++ b/include/internal/parser/mmap.cpp @@ -28,7 +28,7 @@ namespace csv { CSV_INLINE MmapParser::~MmapParser() = default; - CSV_INLINE void MmapParser::finalize_loaded_chunk( + CSV_INLINE bool MmapParser::finalize_loaded_chunk( csv::string_view chunk, std::shared_ptr owner, size_t length, @@ -50,6 +50,7 @@ namespace csv { } this->mmap_pos -= (length - result.complete_prefix_length); + return result.completed_row(); } CSV_INLINE size_t MmapParser::read_window_size(size_t chunk_size) const noexcept { @@ -72,19 +73,20 @@ namespace csv { // for delimiter/header guessing. if (!head_.empty()) { auto head_owner = std::make_shared(std::move(head_)); - const size_t head_start = this->mmap_pos; const size_t length = head_owner->size(); this->mmap_pos += length; - this->finalize_loaded_chunk(*head_owner, head_owner, length, bytes); + const bool completed_row = this->finalize_loaded_chunk(*head_owner, head_owner, length, bytes); // Issue #337: The head is capped at 500KB regardless of chunk_size, // so it may end before the first row does. Only in that case fall // through and map a full read window from the rewound position; // otherwise return so construction doesn't parse an extra window // (CSVReader::initial_read() keeps reading if the header row is - // still pending). - if (this->eof_ || this->mmap_pos != head_start) { + // still pending). Ask the parse result rather than comparing + // positions: a skipped UTF-8 BOM advances mmap_pos even when no + // row completed. + if (this->eof_ || completed_row) { return; } } diff --git a/include/internal/parser/mmap.hpp b/include/internal/parser/mmap.hpp index cda089a2..9779ba45 100644 --- a/include/internal/parser/mmap.hpp +++ b/include/internal/parser/mmap.hpp @@ -38,7 +38,10 @@ namespace csv { void next(size_t bytes) override; private: - void finalize_loaded_chunk( + /** Parse a loaded window and rewind mmap_pos to its incomplete tail. + * Returns whether the window completed at least one row. + */ + bool finalize_loaded_chunk( csv::string_view chunk, std::shared_ptr owner, size_t length, diff --git a/include/internal/parser/orchestrator.hpp b/include/internal/parser/orchestrator.hpp index 110dc961..33c30f36 100644 --- a/include/internal/parser/orchestrator.hpp +++ b/include/internal/parser/orchestrator.hpp @@ -93,7 +93,7 @@ namespace csv { } bool utf8_bom() const noexcept override { - return this->serial_parser_.utf8_bom(); + return this->utf8_bom_; } void reset_with_initial_state(ParserDFAState state) noexcept override { @@ -112,6 +112,46 @@ namespace csv { bool source_exhausted, RowCollection& output ) override { + // BOM handling is a source-window concern: detect it once on the + // first window, reject unsupported encodings before any parser + // (serial or speculative) sees the bytes, and hand parsers a + // BOM-free view. Source adapters speak in original source-byte + // offsets, so the skipped bytes are added back to the completed + // prefix. That also keeps a re-read of the first window (e.g. an + // mmap head that completed no row, issue #337) from starting at + // the BOM again. + // + // Both adapters make the first window their head buffer, which + // holds min(source size, 500KB) bytes, so a first window too + // short to classify a BOM only happens at end of input. + size_t bom_skip = 0; + if (!this->bom_scanned_) { + bom_skip = get_bom_skip_or_throw(chunk, this->utf8_bom_); + this->bom_scanned_ = true; + } + + CSVParseWindowResult result = this->dispatch_window( + chunk.substr(bom_skip), + std::move(owner), + base_offset + bom_skip, + serial_chunk_size, + source_exhausted, + output + ); + result.complete_prefix_length += bom_skip; + result.skipped_prefix_length = bom_skip; + return result; + } + + private: + CSVParseWindowResult dispatch_window( + csv::string_view chunk, + std::shared_ptr owner, + size_t base_offset, + size_t serial_chunk_size, + bool source_exhausted, + RowCollection& output + ) { #if CSV_ENABLE_THREADS if (this->use_speculative_parallel_ && this->worker_count_ > 1 @@ -139,7 +179,6 @@ namespace csv { ); } - private: CSVParseWindowResult parse_serial_window( csv::string_view chunk, std::shared_ptr owner, @@ -178,7 +217,7 @@ namespace csv { this->scanner_, base_offset, 0, - base_offset == 0 + false ); const speculative::ParallelCSVParseResult parse_result = this->speculative_parser_->parse_chunks( @@ -199,6 +238,10 @@ namespace csv { CSVRowFieldPolicy, CSVRowRowPolicy> serial_parser_; SpeculativeParseDiagnostics speculative_diagnostics_; + + /** Whether the first source window has been checked for a Unicode BOM. */ + bool bom_scanned_ = false; + bool utf8_bom_ = false; #if CSV_ENABLE_THREADS ParseFlagMap parse_flags_; WhitespaceMap ws_flags_; diff --git a/tests/test_edge_cases_large_rows.cpp b/tests/test_edge_cases_large_rows.cpp index e4191d63..3ea79c17 100644 --- a/tests/test_edge_cases_large_rows.cpp +++ b/tests/test_edge_cases_large_rows.cpp @@ -381,6 +381,46 @@ TEST_CASE("Issue #337 - First row longer than the mmap head buffer", "[issue_337 } } + SECTION("UTF-8 BOM before a header row longer than the head buffer") { + // The mmap head window completes no row, so MmapParser rewinds and + // re-reads from the start of the source. The BOM must still be + // skipped on that second read instead of becoming part of "A...". + const std::string long_name = "A" + std::string(600 * 1024, 'x'); + const std::string data = "\xEF\xBB\xBF" + long_name + ",B,C\n" + body; + + CSVFormat format; + format.delimiter(',').header_row(0).chunk_size(2 * 1024 * 1024).speculative_parallel_threads(1); + + auto validate_reader = [&](CSVReader& reader) { + REQUIRE(reader.utf8_bom()); + require_col_names(reader, long_name); + + size_t i = 0; + for (auto& row : reader) { + REQUIRE(row.size() == 3); + REQUIRE(row[0].get() == i * 3); + ++i; + } + REQUIRE(i == n_rows); + }; + + SECTION("stream path") { + std::istringstream in(data); + CSVReader reader(in, format); + validate_reader(reader); + } + + SECTION("mmap path") { + FileGuard cleanup("./tests/data/tmp_issue_337_bom_long_header.csv"); + { + std::ofstream out(cleanup.filename, std::ios::binary); + out << data; + } + CSVReader reader(cleanup.filename, format); + validate_reader(reader); + } + } + SECTION("Header row larger than chunk_size still throws") { const std::string data = "A" + std::string(1200 * 1024, 'x') + ",B,C\n" + body; diff --git a/tests/test_read_csv.cpp b/tests/test_read_csv.cpp index f9f6ba36..0cfecd13 100644 --- a/tests/test_read_csv.cpp +++ b/tests/test_read_csv.cpp @@ -7,11 +7,72 @@ #include #include "csv.hpp" #include "shared/file_guard.hpp" +#include "shared/generated_file.hpp" using namespace csv; using std::vector; using std::string; +namespace { + const std::string UTF8_BOM = "\xEF\xBB\xBF"; + + void write_binary_file(const std::string& filename, const std::string& data) { + std::ofstream out(filename, std::ios::binary); + out.write(data.data(), static_cast(data.size())); + } + + /** Run `validate` against the same bytes through the mmap and stream constructors. */ + template + void validate_both_paths( + const std::string& data, + const CSVFormat& format, + const std::string& filename, + Validate validate + ) { + SECTION("mmap path") { + FileGuard cleanup(filename); + write_binary_file(filename, data); + CSVReader reader(filename, format); + validate(reader); + } + + SECTION("stream path") { + std::istringstream input(data); + CSVReader reader(input, format); + validate(reader); + } + } + + void require_encoding_rejected_both_paths( + const std::string& data, + const CSVFormat& format, + const std::string& filename, + const std::string& encoding + ) { + SECTION("mmap path") { + FileGuard cleanup(filename); + write_binary_file(filename, data); + REQUIRE_THROWS_WITH(CSVReader(filename, format), Catch::Matchers::ContainsSubstring(encoding)); + } + + SECTION("stream path") { + std::istringstream input(data); + REQUIRE_THROWS_WITH(CSVReader(input, format), Catch::Matchers::ContainsSubstring(encoding)); + } + } + + const std::string& large_utf8_bom_filename() { + static csv_test::GeneratedFile file("./tests/data/tmp_large_utf8_bom.csv"); + + return file.path([](std::ofstream& out) { + out << UTF8_BOM << "A,B,C\n"; + for (size_t i = 0; i < 600000; ++i) { + out << i * 3 << ',' << i * 3 + 1 << ',' << i * 3 + 2 << '\n'; + } + }); + } +} + TEST_CASE( "Test Parse Flags", "[test_parse_flags]" ) { REQUIRE(internals::make_parse_flags(',', '"')[162] == internals::ParseFlags::QUOTE); } @@ -114,6 +175,215 @@ TEST_CASE("Unicode BOM handling", "[read_unicode_bom]") { } } +TEST_CASE("UTF-8 BOM is stripped on both parser paths", "[read_unicode_bom]") { + CSVFormat guessed = CSVFormat::guess_csv(); + CSVFormat explicit_format; + explicit_format.delimiter(',').header_row(0); + + auto validate_rows = [](CSVReader& reader) { + REQUIRE(reader.utf8_bom()); + REQUIRE(reader.get_col_names() == vector({ "A", "B", "C" })); + + vector> rows; + for (auto& row : reader) { + rows.push_back(vector(row)); + } + REQUIRE(rows == vector>({ { "1", "2", "3" }, { "4", "5", "6" } })); + }; + + const std::string data = UTF8_BOM + "A,B,C\n1,2,3\n4,5,6\n"; + + SECTION("Guessed format") { + validate_both_paths(data, guessed, "./tests/data/tmp_utf8_bom_guessed.csv", validate_rows); + } + + SECTION("Explicit format skips guessing") { + validate_both_paths(data, explicit_format, "./tests/data/tmp_utf8_bom_explicit.csv", validate_rows); + } + + SECTION("Only one leading BOM is stripped") { + const std::string double_bom = UTF8_BOM + UTF8_BOM + "A,B\n1,2\n"; + validate_both_paths(double_bom, explicit_format, "./tests/data/tmp_utf8_double_bom.csv", [](CSVReader& reader) { + REQUIRE(reader.utf8_bom()); + REQUIRE(reader.get_col_names() == vector({ UTF8_BOM + "A", "B" })); + }); + } +} + +TEST_CASE("UTF-16 and UTF-32 BOMs are rejected on both parser paths", "[read_unicode_bom]") { + CSVFormat guessed = CSVFormat::guess_csv(); + CSVFormat explicit_format; + explicit_format.delimiter(',').header_row(0); + + const std::string utf16_le("\xFF\xFE" "A\0,\0B\0\n\0", 10); + const std::string utf16_be("\xFE\xFF" "\0A\0,\0B\0\n", 10); + const std::string utf32_le("\xFF\xFE\0\0" "A\0\0\0,\0\0\0B\0\0\0\n\0\0\0", 20); + const std::string utf32_be("\0\0\xFE\xFF" "\0\0\0A\0\0\0,\0\0\0B\0\0\0\n", 20); + const std::string filename = "./tests/data/tmp_rejected_unicode_bom.csv"; + + SECTION("UTF-16 LE, guessed format") { + require_encoding_rejected_both_paths(utf16_le, guessed, filename, "UTF-16"); + } + + SECTION("UTF-16 BE, explicit format") { + require_encoding_rejected_both_paths(utf16_be, explicit_format, filename, "UTF-16"); + } + + SECTION("UTF-16 LE, explicit format") { + require_encoding_rejected_both_paths(utf16_le, explicit_format, filename, "UTF-16"); + } + + SECTION("UTF-32 LE is not mistaken for UTF-16 LE") { + require_encoding_rejected_both_paths(utf32_le, guessed, filename, "UTF-32"); + } + + SECTION("UTF-32 BE, explicit format") { + require_encoding_rejected_both_paths(utf32_be, explicit_format, filename, "UTF-32"); + } +} + +TEST_CASE("Tiny UTF-8 BOM inputs behave consistently", "[read_unicode_bom]") { + CSVFormat format; + format.delimiter(',').no_header(); + + SECTION("Empty input") { + // Stream only: a zero-byte file opened by filename is reported as an + // open failure (see "Empty CSV does not crash parser entry points"). + std::istringstream input(""); + CSVReader reader(input, format); + size_t n_rows = 0; + for (auto& row : reader) { (void)row; ++n_rows; } + REQUIRE(n_rows == 0); + REQUIRE_FALSE(reader.utf8_bom()); + } + + SECTION("BOM-only file") { + validate_both_paths(UTF8_BOM, format, "./tests/data/tmp_bom_only.csv", [](CSVReader& reader) { + size_t n_rows = 0; + for (auto& row : reader) { (void)row; ++n_rows; } + REQUIRE(n_rows == 0); + REQUIRE(reader.utf8_bom()); + }); + } + + SECTION("BOM followed by one row without a trailing newline") { + validate_both_paths(UTF8_BOM + "1,2,3", format, "./tests/data/tmp_bom_one_row.csv", [](CSVReader& reader) { + vector> rows; + for (auto& row : reader) { + rows.push_back(vector(row)); + } + REQUIRE(rows == vector>({ { "1", "2", "3" } })); + REQUIRE(reader.utf8_bom()); + }); + } +} + +TEST_CASE("UTF-8 BOM is not part of the first row", "[read_unicode_bom]") { + // The BOM precedes the first row rather than belonging to it, so the + // row's raw text and source offset both start after the three BOM bytes. + CSVFormat format; + format.delimiter(',').no_header(); + + const std::string data = UTF8_BOM + "1,2,3\n4,5,6\n"; + validate_both_paths(data, format, "./tests/data/tmp_bom_first_row.csv", [](CSVReader& reader) { + CSVRow row; + REQUIRE(reader.read_row(row)); + REQUIRE(row.raw_str() == "1,2,3"); + REQUIRE(row.byte_offset() == 3); + REQUIRE(row[0] == "1"); + + REQUIRE(reader.read_row(row)); + REQUIRE(row.raw_str() == "4,5,6"); + REQUIRE(row.byte_offset() == 9); + }); +} + +TEST_CASE("UTF-8 BOM with a file crossing chunk boundaries", "[read_unicode_bom]") { + const std::string& filename = large_utf8_bom_filename(); + + auto validate_reader = [](CSVReader& reader) { + REQUIRE(reader.utf8_bom()); + REQUIRE(reader.get_col_names() == vector({ "A", "B", "C" })); + + size_t i = 0; + for (auto& row : reader) { + REQUIRE(row.size() == 3); + REQUIRE(row[0].get() == i * 3); + REQUIRE(row[1].get() == i * 3 + 1); + REQUIRE(row[2].get() == i * 3 + 2); + ++i; + } + REQUIRE(i == 600000); + }; + + CSVFormat serial; + serial.delimiter(',').header_row(0).speculative_parallel_threads(1); + + // Small chunks make the first stream window larger than one serial chunk, + // which sends it through the speculative parser. + CSVFormat speculative; + speculative.delimiter(',').header_row(0) + .chunk_size(internals::CSV_CHUNK_SIZE_FLOOR) + .speculative_parallel_threads(4) + .speculative_parallel_min_bytes(0); + + auto check_worker_count = [](CSVReader& reader, size_t expected) { +#if CSV_ENABLE_THREADS + REQUIRE(reader.parse_worker_count() == expected); +#else + (void)reader; + (void)expected; +#endif + }; + + SECTION("Serial, mmap path") { + CSVReader reader(filename, serial); + validate_reader(reader); + } + + SECTION("Serial, stream path") { + std::ifstream input(filename, std::ios::binary); + CSVReader reader(input, serial); + validate_reader(reader); + } + + SECTION("Speculative, mmap path") { + CSVReader reader(filename, speculative); + check_worker_count(reader, 4); + validate_reader(reader); + } + + SECTION("Speculative, stream path") { + std::ifstream input(filename, std::ios::binary); + CSVReader reader(input, speculative); + check_worker_count(reader, 4); + validate_reader(reader); + } +} + +#ifndef __EMSCRIPTEN__ +TEST_CASE("guess_format() sees through a UTF-8 BOM", "[read_unicode_bom]") { + SECTION("UTF-8 BOM") { + const std::string filename = "./tests/data/tmp_guess_format_bom.csv"; + FileGuard cleanup(filename); + write_binary_file(filename, UTF8_BOM + "A|B|C\n1|2|3\n4|5|6\n"); + + const auto guessed = guess_format(filename); + REQUIRE(guessed.delim == '|'); + REQUIRE(guessed.header_row == 0); + REQUIRE(guessed.n_cols == 3); + } + + SECTION("UTF-16 is rejected") { + const std::string filename = "./tests/data/tmp_guess_format_utf16.csv"; + FileGuard cleanup(filename); + write_binary_file(filename, std::string("\xFF\xFE" "A\0,\0B\0\n\0", 10)); + + REQUIRE_THROWS_WITH(guess_format(filename), Catch::Matchers::ContainsSubstring("UTF-16")); + } +} +#endif + //! [Escaped Comma] TEST_CASE( "Test Escaped Comma", "[read_csv_comma]" ) { auto rows = "A,B,C\r\n" // Header row From e605792bd2a75f814a5a5b3e6bd9f54930a1c41d Mon Sep 17 00:00:00 2001 From: Vincent La Date: Sat, 10 Oct 2026 12:35:25 -0700 Subject: [PATCH 3/5] Remove BOM scanning plumbing from the parser core CSVParseOrchestrator and format guessing now own BOM handling, so the core no longer needs per-instance scan state. Remove ParserChunkOptions::scan_bom, the core's utf8_bom()/strip_unicode_bom() and their state, and the scan_bom flags on speculative chunks. ParserChunkOptions is now (initial_state, source_start). Stale two-argument ParserChunkOptions(state, false) calls in tests would otherwise have compiled silently as source_start = 0; they were found by temporarily deleting the bool overloads. Update ARCHITECTURE.md, JOURNEY_OF_A_CSV_FIELD.md, and the refactor page to reflect that the orchestrator owns BOM handling. Co-Authored-By: Claude Opus 5.5 --- include/internal/ARCHITECTURE.md | 6 +- include/internal/BOMStrippingRefactor.md | 362 +++++------------- include/internal/JOURNEY_OF_A_CSV_FIELD.md | 9 +- include/internal/parser/core.hpp | 31 +- include/internal/parser/driver.hpp | 9 +- include/internal/parser/orchestrator.hpp | 4 +- include/internal/speculative/chunks.hpp | 6 +- .../internal/speculative/parallel_parser.hpp | 8 +- tests/test_speculative_parser.cpp | 18 +- 9 files changed, 120 insertions(+), 333 deletions(-) diff --git a/include/internal/ARCHITECTURE.md b/include/internal/ARCHITECTURE.md index b745c58d..07e2c236 100644 --- a/include/internal/ARCHITECTURE.md +++ b/include/internal/ARCHITECTURE.md @@ -71,7 +71,8 @@ Two independent parser paths exist and must be kept behaviorally aligned: - CSVParserCore - Templated, non-virtual byte parser core in parser/core.hpp. - - Owns DFA state, BOM handling, field/row construction, and concrete row-sink emission. + - Owns DFA state, field/row construction, and concrete row-sink emission. + - Only ever sees BOM-free bytes; Unicode BOM handling belongs to the orchestrator. - Source adapters feed byte windows into it; it does not own file, mmap, or stream source mechanics. - PermissiveParsePolicy @@ -97,6 +98,9 @@ Two independent parser paths exist and must be kept behaviorally aligned: - parser/orchestrator.hpp - Chooses serial CSVParserCore parsing or speculative parallel parsing for a byte window. + - Owns Unicode BOM handling: scans the first source window once, rejects UTF-16/UTF-32, + and hands parsers a BOM-free view while reporting completed prefixes in source bytes. + See @ref bom_stripping_refactor. - MmapParser - Reads chunks from memory maps and handles chunk-transition remainder. diff --git a/include/internal/BOMStrippingRefactor.md b/include/internal/BOMStrippingRefactor.md index 0b2a9778..a9b2a83d 100644 --- a/include/internal/BOMStrippingRefactor.md +++ b/include/internal/BOMStrippingRefactor.md @@ -2,282 +2,100 @@ # BOM Stripping Refactor -## Context - -`CSVParserCore` currently owns BOM detection through `ParserChunkOptions::scan_bom`. -That works, but it means BOM handling is still part of the lower-level byte parser -instead of the source/window orchestration layer. `MmapParser` and `StreamParser` -now share `CSVParserDriverBase::utf8_bom()`, but the actual scan/strip behavior -still happens inside the parser core used by serial and speculative parsing. - -The next architectural cleanup is to make BOM handling a window-boundary concern: -detect a Unicode BOM once, reject unsupported encodings early, strip a UTF-8 BOM -before parsing, and feed BOM-free byte views into both serial and speculative -parsers. - -## Current Behavior - -These results were checked on 2026-10-10 against `master` at `eee1732` with a -probe built from `include/`, on both constructor paths: - -| Scenario | mmap | stream | -|---|---|---| -| UTF-8 BOM, serial parse | stripped, `utf8_bom()` true | stripped, `utf8_bom()` true | -| UTF-8 BOM, speculative parse (4 workers) | stripped, `utf8_bom()` true | stripped, **`utf8_bom()` false** | -| No-header BOM file, first row `raw_str()` | **includes `EF BB BF`** | **includes `EF BB BF`** | -| No-header BOM file, first row `byte_offset()` | 0 | 0 | -| BOM-only file | 0 rows, `utf8_bom()` true | 0 rows, `utf8_bom()` true | - -Where the BOM bytes leak or get misreported: - -- **Stream + speculative `utf8_bom()` bug.** The first stream window is the - 500KB head plus a full read window, so it is larger than `serial_chunk_size` - and goes to `parse_speculative_window()`. A speculative worker core strips the - BOM and records `utf8_bom_` on itself, but `CSVParseOrchestrator::utf8_bom()` - reads `serial_parser_.utf8_bom()`, which never saw the BOM. mmap is not - affected because its first window is the head buffer, which is never larger - than one serial chunk. -- **First row includes the BOM.** `CSVParserCore::parse()` sets - `current_row_start() = 0` *before* `strip_unicode_bom()` advances `data_pos_`. - So the first row's raw span still covers the BOM. Field values are unaffected, - because `field_start_` is computed from `data_pos_`. -- **Core-level scan state is per core instance.** `unicode_bom_scan_` lives on - each `CSVParserCore`: the serial parser, every speculative worker parser, the - repair parser, and each guessing parser. The orchestrator also re-derives "is - this the first window" from `base_offset == 0`. Today that's correct only - because a window that completes no rows makes `CSVReader` throw - "row larger than chunk size" before any rewind to offset 0 can re-parse. - -Format guessing (`parser/guessing.cpp`) builds its own `CSVParserCore` per -candidate delimiter. With the default `scan_bom = true` it strips the BOM, and -it throws on UTF-16/UTF-32 during `CSVReader` construction. - -## Goals - -- Keep `MmapParser` and `StreamParser` behavior identical. -- Detect UTF-8, UTF-16, and UTF-32 BOMs before CSV parsing begins. -- Strip UTF-8 BOM bytes before parser-core field/row offsets are produced. -- Reject UTF-16/UTF-32 with the existing "use a transcoder first" style error. -- Keep `CSVReader::utf8_bom()` reporting behavior. -- Preserve bounded-memory stream parsing and mmap chunk-remainder semantics. - -## Non-goals +**Status:** implemented in v5.5.0. -- Do not parse UTF-16 or UTF-32 directly. -- Do not add automatic transcoding. -- Do not change delimiter handling, quoting behavior, or row materialization. -- Do not make `CSVReader::iterator` multi-pass or cache source chunks. - -## Proposed Shape - -Move BOM scan state into the common parse orchestration path, likely -`CSVParseOrchestrator`, or a small helper owned by it. - -For the first source window only: - -1. Inspect the leading bytes with the shared BOM utility. -2. Throw on UTF-16/UTF-32 BOMs. -3. Record whether a UTF-8 BOM was present. -4. Pass `chunk.substr(skip)` to the serial or speculative parser. -5. Adjust returned `CSVParseWindowResult::complete_prefix_length` by the skipped - byte count so source adapters still advance by source-byte offsets. +## Context -After that first scan, all parser-core invocations should receive -`ParserChunkOptions(..., false)` or an equivalent path that disables core-level -BOM scanning. +`CSVParserCore` used to own BOM detection through `ParserChunkOptions::scan_bom`. +Every core instance (the serial parser, each speculative worker, the repair +parser, and each format-guessing parser) kept its own "already scanned" flag and +stripped the BOM by advancing its read position inside the chunk. + +That put a source-window concern inside the byte parser, and it caused three bugs: + +- **`utf8_bom()` was wrong for speculative stream input.** The first stream + window is the 500KB head plus a full read window, so it can be routed to + `parse_speculative_window()`. A speculative worker stripped the BOM and + recorded it on itself, but `CSVParseOrchestrator::utf8_bom()` read the serial + parser's flag, which never saw the BOM. +- **The first row included the BOM.** `CSVParserCore::parse()` set the row start + to 0 *before* stripping advanced `data_pos_`. So the first row's `raw_str()` + began with `EF BB BF` and its `byte_offset()` was 0. Field values were correct, + because field starts were computed from `data_pos_`. +- **A re-read first window kept the BOM.** When the mmap head window completed + no row (issue #337), `MmapParser` rewound to offset 0 and parsed a full window. + The serial core had already marked its scan as done, so the BOM bytes became + part of the first column name. + +## Design + +BOM handling is a window-boundary concern owned by `CSVParseOrchestrator` +(`parser/orchestrator.hpp`): + +1. On the first `parse_window()` call only, `get_bom_skip_or_throw()` inspects + the leading bytes. A flag records that the scan happened; there is no + inference from `base_offset == 0`. +2. UTF-16 and UTF-32 BOMs throw before any serial or speculative parser sees the + window. +3. A UTF-8 BOM sets the orchestrator's `utf8_bom_` and is skipped. The parser + receives `chunk.substr(skip)` and `base_offset + skip`. +4. The returned `CSVParseWindowResult` adds the skipped bytes back into + `complete_prefix_length` and also reports them as `skipped_prefix_length`. + +`CSVParserCore` and the speculative chunk types no longer carry any BOM state. +`ParserChunkOptions` is just `(initial_state, source_start)`. + +Format guessing (`parser/guessing.cpp`) strips the BOM itself, once per +`guess_format()` call rather than once per candidate delimiter. It therefore +still rejects UTF-16/UTF-32 during `CSVReader` construction. + +The first window is always the adapter's head buffer, which holds +min(source size, 500KB) bytes. So a first window too short to classify a BOM +happens only at end of input, and the scan does not need to defer. ## Offset Invariants -The key invariant is that source adapters speak in original source-byte offsets, -while parser cores may see a BOM-free view. - -- `base_offset` passed into parsing should still describe the original source. -- Field offsets stored in `RawCSVData` must remain correct relative to the - backing chunk view they reference. -- `complete_prefix_length` returned to source adapters must include any stripped - BOM bytes, otherwise mmap/stream remainder handling will re-read or retain the - wrong prefix. -- If the first window is only a UTF-8 BOM, the parser should not manufacture a - row and should still advance past the BOM. -- BOM rejection must happen before speculative workers are launched. - -## Test Plan - -Add focused tests for both constructor paths using Catch2 `SECTION`s: - -- UTF-8 BOM is stripped for mmap input. -- UTF-8 BOM is stripped for stream input. -- `CSVReader::utf8_bom()` is true after reading UTF-8 BOM input. -- UTF-16 LE/BE BOMs throw for mmap input. -- UTF-16 LE/BE BOMs throw for stream input. -- UTF-32 LE/BE BOMs throw if supported by the BOM utility. -- Empty file, BOM-only file, and BOM followed by a single row behave consistently. -- A large UTF-8 BOM file crossing the 10MB chunk boundary preserves row data and - chunk remainder behavior. -- The same large file with speculative parsing forced on - (`speculative_parallel_threads(4)`, `speculative_parallel_min_bytes(0)`) - reports `utf8_bom()` on both paths. This fails on stream input today. -- A no-header BOM file's first row has `raw_str() == "1,2,3"` and - `byte_offset() == 3`. Both fail today. -- A double BOM (`EF BB BF EF BB BF`) strips exactly one BOM, and the second - stays in the first field. This guards against double stripping by the - orchestrator and the core. -- UTF-32 LE (`FF FE 00 00`) reports "UTF-32", not "UTF-16", on both paths. -- `csv::guess_format()` on a UTF-8 BOM file still guesses the delimiter and - header, and still throws on UTF-16. - -## Risks - -- Off-by-skip errors can corrupt `stream_pos_`, `mmap_pos`, or `leftover_`. -- Speculative parsing may double-adjust offsets if both orchestrator and core - scan BOMs. -- Empty or BOM-only files can expose EOF differences between mmap and stream - paths. -- Tests that assert exact exception strings should use the shared exception - message constants/helpers to avoid drift. - -## Implementation Plan - -The work splits into two PRs. PR 1 moves ownership and fixes the bugs described -above, while the old core plumbing stays in place but goes unused. PR 2 deletes -that plumbing. Splitting it this way means a regression in PR 1 can be bisected -without the noise from deleting code. +Source adapters speak in original source-byte offsets. Parser cores see a +BOM-free view. + +- `RawCSVData::source_start` describes the original source position of the view + the core parsed. For the first window of a BOM file this is `base_offset + 3`, + so the first row's `byte_offset()` is 3 and `raw_str()` excludes the BOM. +- Field offsets stored in `RawCSVData` are relative to the backing view they + reference, which is unchanged by the skip. +- `complete_prefix_length` includes skipped BOM bytes. `MmapParser`'s + `mmap_pos -= (length - complete_prefix_length)` and `StreamParser`'s + `leftover_` / `stream_pos_` handling therefore advance past the BOM even when + no row completed, so a re-read never starts at the BOM. +- Code that needs to know whether a window completed a row must use + `CSVParseWindowResult::completed_row()`, not position arithmetic. A skipped + BOM advances the source position without completing a row. `MmapParser::next()` + relies on this to decide whether the head buffer needs a follow-up read + (issue #337). +- A BOM-only first window produces no row and still advances past the BOM. -### PR 1: Orchestrator owns the BOM - -**Step 1: Characterization tests** (`tests/test_read_csv.cpp`, `[read_unicode_bom]`) - -Extend `TEST_CASE("Unicode BOM handling")` with one `SECTION` per constructor -path for every case in the Test Plan above. Use `FileGuard` temp files for mmap -and `std::istringstream` for stream. Build the large-file cases with ≥500K rows -and distinct per-column values, as `tests/AGENTS.md` requires. Check row -contiguity, not just row count. - -Three of these tests fail on `master`: stream + speculative `utf8_bom()`, first -row `raw_str()`, and first row `byte_offset()`. Commit them in the same commit -as Step 2 so every commit stays green. - -**Step 2: First-window BOM scan in `CSVParseOrchestrator`** (`parser/orchestrator.hpp`) - -- Add private state `bool bom_scanned_ = false;` and `bool utf8_bom_ = false;`. -- At the top of `parse_window()`, before the serial/speculative dispatch: - - ```cpp - size_t bom_skip = 0; - if (!this->bom_scanned_) { - bom_skip = get_bom_skip_or_throw(chunk, this->utf8_bom_); - this->bom_scanned_ = true; - } - chunk = chunk.substr(bom_skip); - base_offset += bom_skip; - // ... dispatch exactly as today, using the BOM-free view ... - result.complete_prefix_length += bom_skip; - ``` - -- `utf8_bom()` returns `this->utf8_bom_` instead of `serial_parser_.utf8_bom()`. - This fixes the stream + speculative bug. -- Use a flag rather than `base_offset == 0` for the "first window" test, so a - future rewind to offset 0 can't scan twice. -- Throwing here happens before `parse_speculative_window()` dispatches any - worker tasks, which satisfies the "reject before workers" invariant. -- The scan runs on whatever the first window contains. Both adapters guarantee - that the first window is the head buffer, which holds min(source size, 500KB) - bytes. So a window shorter than 4 bytes only happens when the source is - exhausted. Write this assumption down in a comment and don't add deferral logic. - -Offsets after this change: - -- `RawCSVData::source_start` for the first block becomes `base_offset + 3`. - `CSVRow::byte_offset()` of the first row becomes 3 and `raw_str()` excludes - the BOM. Rows after the first are unchanged, because `source_start` and the - view origin move together. -- `complete_prefix_length` stays in source-window bytes, so `MmapParser`'s - `mmap_pos -= (length - cpl)` and `StreamParser`'s - `leftover_ = chunk.substr(cpl)` / `stream_pos_ += cpl` need no changes. -- In a BOM-only window the parser sees an empty view: no row is produced, - `cpl` is 3, and on exhaustion `end_feed()` has no partial row to flush. - Before relying on this, confirm that `parse_prepared_chunk()` tolerates an - empty `chunk`, which means calling `reserve_for_source_size(0)` and entering - the `parse()` loop with `in.size() == 0`. - -**Step 3: Stop core-level scanning on orchestrated calls** - -- `parse_serial_window()` currently calls the - `parse_chunk(chunk, owner, output, size_t source_start)` overload, which - hard-codes `ParserChunkOptions(initial_state_, true, source_start)`. The core - has no public getter for `initial_state_`, so add - `ParserDFAState initial_state() const noexcept` next to `ending_state()`. - Then call the options overload with - `ParserChunkOptions(serial_parser_.initial_state(), false, base_offset)`. - In PR 2 the `source_start` overload loses its `true` argument along with the - rest of the plumbing. -- `parse_speculative_window()`: pass `false` for `scan_bom_for_first_chunk` - instead of `base_offset == 0`. -- The double-BOM test is what proves this step works: if the core still scans, - it strips the second BOM. - -**Step 4: Format guessing** (`parser/guessing.cpp`) - -In `guess_format(head, delims)`, strip once before the per-delimiter loop: - -```cpp -bool ignored = false; -head = head.substr(get_bom_skip_or_throw(head, ignored)); -``` - -This keeps UTF-16/UTF-32 rejection in the `CSVReader` constructor even after -the core loses its scan. It also scans once per guess instead of once per -candidate delimiter. Until PR 2 the guessing core still scans too, which is -harmless because the head no longer starts with a BOM. - -**Validation for PR 1** - -- Full `ctest` with the default configuration (threads on). -- `-DCSV_ENABLE_THREADS=OFF`, which compiles out the `#if CSV_ENABLE_THREADS` - branches in the orchestrator. -- The generated single header (`generate_single_header` target), compiled - directly. Don't compile `single_include/csv.hpp`. -- C++11 build. `csv::string_view::substr` and the new members must not need - C++14 or later. - -### PR 2: Remove core BOM plumbing - -Delete the following, then `grep -rn "scan_bom\|unicode_bom\|strip_unicode_bom" include tests` -and confirm nothing remains: - -| File | Remove | -|---|---| -| `parser/core.hpp` | `ParserChunkOptions::scan_bom` and its constructor parameter; `scan_bom_for_current_chunk_`, `unicode_bom_scan_`, `utf8_bom_`, `strip_unicode_bom()`, `utf8_bom()`; the reset in `finish_parse()`; the `scan_bom` branch in `parse()`; the `true` argument in the `parse_chunk` overloads | -| `parser/core.hpp` | Move the `get_bom_skip_or_throw` declaration to `parser/driver.hpp`, since the core no longer uses it. The definition stays in `driver.cpp`, and the function stays in `csv::internals` because tests call it there. | -| `parser/driver.hpp` | The `CSVParserCore<>::utf8_bom()` fallback in `CSVParserDriverBase::utf8_bom()`. Return `false` when there is no orchestrator. | -| `speculative/parallel_parser.hpp` | `SpeculativeParseChunk::scan_bom`, the `scan_bom_for_first_chunk` parameter, `result.scan_bom = ...` | -| `speculative/chunks.hpp` | `ParsedChunkRows::scan_bom` and its assignment in `split_parsed_chunk_rows()`; the `scan_bom` arguments at both `ParserChunkOptions` call sites | -| `tests/test_speculative_parser.cpp` | `first.scan_bom` / `second.scan_bom` assignments (two tests) | - -**Hazard:** shrinking `ParserChunkOptions(state, bool scan_bom, size_t source_start)` -to `(state, size_t source_start)` lets a missed two-argument call like -`ParserChunkOptions(state, true)` compile silently as `source_start = 1`. Before -deleting the parameter, temporarily mark the old 3-argument constructor -`= delete`, or rename it, and let the compiler list every call site. - -### Documentation (in PR 2) - -- `include/internal/ARCHITECTURE.md`: remove "BOM handling" from the - `CSVParserCore` bullet and add it to the orchestrator's responsibilities. -- `include/internal/JOURNEY_OF_A_CSV_FIELD.md`: remove "UTF-8 BOM skip" from the - core's list and the `scan_bom` lines from the speculative chunk diagram. Add a - "first window: BOM scan/strip" step before dispatch. -- This page: replace "Current Behavior" with a short "Resolved" note. Keep - Offset Invariants as durable reference material. -- The PR description must call out the user-visible changes: the first row's - `raw_str()` and `byte_offset()` move past the BOM, and `utf8_bom()` is now - correct for speculative stream input. `README.md` needs no change. - -### Out of Scope +## Non-goals -- An mmap source whose first row is longer than the 500KB head buffer, with a - larger `chunk_size()`, silently yields zero rows and no column names. This - happens with or without a BOM, and stream input handles the same file - correctly. Track it separately. Once it's fixed, the - `complete_prefix_length += bom_skip` adjustment from Step 2 is what keeps the - re-read window from parsing the BOM as data. +- Parsing UTF-16 or UTF-32 directly, or transcoding automatically. Those inputs + are rejected with the "use a transcoder first" error. +- Changing delimiter handling, quoting behavior, or row materialization. +- Making `CSVReader::iterator` multi-pass or caching source chunks. + +## Test Coverage + +`tests/test_read_csv.cpp` (`[read_unicode_bom]`) covers each case through both +the mmap and stream constructors: + +- UTF-8 BOM stripping with guessed and explicit formats, and `utf8_bom()`. +- Only one leading BOM is stripped. A second BOM stays in the first field, which + guards against double stripping. +- UTF-16 LE/BE and UTF-32 LE/BE rejection, including UTF-32 LE (`FF FE 00 00`) + not being reported as UTF-16. +- Empty input, a BOM-only file, and a BOM followed by one unterminated row. +- The first row's `raw_str()` and `byte_offset()` exclude the BOM. +- A 600K-row BOM file crossing chunk boundaries with serial parsing and with + speculative parsing forced on. +- `guess_format()` on BOM and UTF-16 files. + +`tests/test_edge_cases_large_rows.cpp` (`[issue_337]`) covers a UTF-8 BOM before +a header row longer than the mmap head buffer. diff --git a/include/internal/JOURNEY_OF_A_CSV_FIELD.md b/include/internal/JOURNEY_OF_A_CSV_FIELD.md index 0323433f..e30b5634 100644 --- a/include/internal/JOURNEY_OF_A_CSV_FIELD.md +++ b/include/internal/JOURNEY_OF_A_CSV_FIELD.md @@ -12,7 +12,9 @@ have changed shape. A CSV field is usually not copied while parsing. 1. A source adapter (`MmapParser` or `StreamParser`) provides a byte window. -2. `CSVParseOrchestrator` chooses serial parsing or speculative parallel parsing. +2. `CSVParseOrchestrator` strips a leading UTF-8 BOM from the first window + (rejecting UTF-16/UTF-32), then chooses serial parsing or speculative + parallel parsing. 3. `CSVParserCore` walks bytes and records field boundaries in `RawCSVFieldList`. 4. `CSVRow` stores a shared pointer to `RawCSVData`. 5. `CSVRow` slices either the original bytes or parser-realized quoted-field @@ -84,7 +86,6 @@ CSVReader - quote state - escaped quote pairs - CR, LF, and CRLF row endings -- UTF-8 BOM skip - field boundary recording - row emission @@ -143,7 +144,7 @@ does not know whether the row came from serial parsing or speculative parsing. Low-level flow: ```text -source window bytes +source window bytes (BOM already stripped by CSVParseOrchestrator) | v make_speculative_parse_chunks() @@ -151,7 +152,6 @@ make_speculative_parse_chunks() +--> chunk 0 bytes + owner + offset + sequence_number | | | +--> starts_at_record_boundary = true - | +--> scan_bom = true | +--> assumed_start_state = outside quotes | +--> chunk N bytes + owner + offset + sequence_number @@ -173,7 +173,6 @@ make_speculative_parse_chunks() +--> sequence_number +--> assumed_start_state +--> starts_at_record_boundary - +--> scan_bom ParallelCSVParser::parse_chunks() | diff --git a/include/internal/parser/core.hpp b/include/internal/parser/core.hpp index fe3f8e63..9dd69fe7 100644 --- a/include/internal/parser/core.hpp +++ b/include/internal/parser/core.hpp @@ -95,9 +95,6 @@ namespace csv { return make_ws_flags(flags.data(), flags.size()); } - /** Return the number of leading BOM bytes to skip, or throw for unsupported Unicode encodings. */ - CSV_INLINE size_t get_bom_skip_or_throw(csv::string_view data, bool& utf8_bom); - /** Explicit DFA state at a parse boundary. * * This is intentionally about parser control flow, not field metadata. @@ -132,15 +129,12 @@ namespace csv { ParserChunkOptions() noexcept = default; explicit ParserChunkOptions( ParserDFAState initial_state, - bool scan_bom = true, size_t source_start = 0 ) noexcept : initial_state(initial_state), - scan_bom(scan_bom), source_start(source_start) {} ParserDFAState initial_state; - bool scan_bom = true; size_t source_start = 0; }; @@ -426,9 +420,6 @@ namespace csv { return quote_escape_flag(parse_flag(ch), this->quote_escape_); } - /** Whether or not this CSV has a UTF-8 byte order mark. */ - CONSTEXPR bool utf8_bom() const { return this->utf8_bom_; } - void set_output(RowSink& output) noexcept { this->output_ = &output; } @@ -494,7 +485,7 @@ namespace csv { chunk, std::move(owner), output, - ParserChunkOptions(this->initial_state_, false, source_start) + ParserChunkOptions(this->initial_state_, source_start) ); } @@ -559,9 +550,6 @@ namespace csv { this->pending_linefeed_ = start_state.pending_linefeed; this->data_pos_ = 0; this->current_row_start() = 0; - if (this->scan_bom_for_current_chunk_) { - this->strip_unicode_bom(); - } auto& in = this->data_ptr_->data; @@ -679,15 +667,10 @@ namespace csv { bool field_has_double_quote_ = false; ParserDFAState initial_state_; ParserDFAState ending_state_; - bool scan_bom_for_current_chunk_ = true; /** Where we are in the current data block. */ size_t data_pos_ = 0; - /** Whether or not an attempt to find Unicode BOM has been made. */ - bool unicode_bom_scan_ = false; - bool utf8_bom_ = false; - RowSink* output_ = nullptr; ParsePolicy policy_; FieldPolicy field_policy_; @@ -802,16 +785,6 @@ namespace csv { this->emit_row(std::move(current_row_)); } - /** Handle possible Unicode byte order mark. */ - void strip_unicode_bom() { - auto& data = this->data_ptr_->data; - - if (!this->unicode_bom_scan_) { - this->data_pos_ += get_bom_skip_or_throw(data, this->utf8_bom_); - this->unicode_bom_scan_ = true; - } - } - ParserChunkResult parse_prepared_chunk( csv::string_view chunk, std::shared_ptr owner, @@ -828,7 +801,6 @@ namespace csv { this->data_ptr_->field_scalars.reserve_for_source_size(chunk.size()); this->data_ptr_->quote_arena.reserve_for_source_size(chunk.size()); this->initial_state_ = options.initial_state; - this->scan_bom_for_current_chunk_ = options.scan_bom; this->current_row_ = this->row_policy_.make_initial_row(this->data_ptr_); this->policy_.begin_chunk(this->data_ptr_); this->policy_.begin_row(this->current_row_); @@ -846,7 +818,6 @@ namespace csv { this->initial_state_ = this->ending_state_.pending_linefeed ? this->ending_state_ : ParserDFAState{}; - this->scan_bom_for_current_chunk_ = true; return remainder; } }; diff --git a/include/internal/parser/driver.hpp b/include/internal/parser/driver.hpp index 357a785b..567d5399 100644 --- a/include/internal/parser/driver.hpp +++ b/include/internal/parser/driver.hpp @@ -29,6 +29,13 @@ namespace csv { namespace internals { + /** Return the number of leading BOM bytes to skip, or throw for unsupported Unicode encodings. + * + * BOM handling happens at the source-window level (CSVParseOrchestrator and + * format guessing); CSVParserCore only ever sees BOM-free bytes. + */ + CSV_INLINE size_t get_bom_skip_or_throw(csv::string_view data, bool& utf8_bom); + namespace parser { struct GuessScore { size_t header; @@ -175,7 +182,7 @@ namespace csv { virtual bool utf8_bom() const noexcept { return this->parse_orchestrator_ ? this->parse_orchestrator_->utf8_bom() - : CSVParserCore<>::utf8_bom(); + : false; } protected: diff --git a/include/internal/parser/orchestrator.hpp b/include/internal/parser/orchestrator.hpp index 33c30f36..d011b109 100644 --- a/include/internal/parser/orchestrator.hpp +++ b/include/internal/parser/orchestrator.hpp @@ -215,9 +215,7 @@ namespace csv { owner, serial_chunk_size, this->scanner_, - base_offset, - 0, - false + base_offset ); const speculative::ParallelCSVParseResult parse_result = this->speculative_parser_->parse_chunks( diff --git a/include/internal/speculative/chunks.hpp b/include/internal/speculative/chunks.hpp index 0610d29e..dcc3d1b5 100644 --- a/include/internal/speculative/chunks.hpp +++ b/include/internal/speculative/chunks.hpp @@ -83,7 +83,6 @@ namespace csv { csv::string_view chunk; std::shared_ptr owner; bool starts_at_record_boundary = true; - bool scan_bom = true; ParserChunkResult parse_result; CSVRowFragment prefix_fragment; std::vector complete_rows; @@ -106,7 +105,6 @@ namespace csv { result.owner = owner; result.starts_at_record_boundary = starts_at_record_boundary || parse_result.initial_state.pending_linefeed; - result.scan_bom = starts_at_record_boundary && sequence_number == 0; result.parse_result = parse_result; size_t first_complete_row = 0; @@ -163,7 +161,7 @@ namespace csv { fragment.bytes, fragment.owner, rows, - ParserChunkOptions(ParserDFAState(), false, fragment.offset) + ParserChunkOptions(ParserDFAState(), fragment.offset) ); parser.end_feed(); return rows; @@ -181,7 +179,7 @@ namespace csv { chunk.chunk, chunk.owner, parsed_rows, - ParserChunkOptions(corrected_initial_state, chunk.scan_bom, chunk.offset) + ParserChunkOptions(corrected_initial_state, chunk.offset) ); return split_parsed_chunk_rows( diff --git a/include/internal/speculative/parallel_parser.hpp b/include/internal/speculative/parallel_parser.hpp index 97006920..9e0ca130 100644 --- a/include/internal/speculative/parallel_parser.hpp +++ b/include/internal/speculative/parallel_parser.hpp @@ -16,7 +16,6 @@ namespace csv { std::shared_ptr owner; ChunkSpeculation speculation; bool starts_at_record_boundary = false; - bool scan_bom = false; }; struct ParallelCSVParseResult { @@ -34,8 +33,7 @@ namespace csv { size_t chunk_size, const SpeculativeScanner& scanner, size_t base_offset = 0, - size_t first_sequence_number = 0, - bool scan_bom_for_first_chunk = true + size_t first_sequence_number = 0 ) { std::vector chunks; if (chunk_size == 0) { @@ -55,7 +53,6 @@ namespace csv { chunk.owner = owner; chunk.speculation = scanner.speculate(sequence_number, chunk.offset, bytes); chunk.starts_at_record_boundary = first_chunk; - chunk.scan_bom = first_chunk && scan_bom_for_first_chunk; // The first chunk in a speculative window starts at a known row // boundary because source windows must be aligned to incomplete-row @@ -144,7 +141,7 @@ namespace csv { chunk.bytes, chunk.owner, rows, - ParserChunkOptions(chunk.speculation.assumed_start_state, chunk.scan_bom, chunk.offset) + ParserChunkOptions(chunk.speculation.assumed_start_state, chunk.offset) ); ParsedChunkRows result = split_parsed_chunk_rows( @@ -156,7 +153,6 @@ namespace csv { chunk.starts_at_record_boundary, chunk.offset ); - result.scan_bom = chunk.scan_bom; return result; } diff --git a/tests/test_speculative_parser.cpp b/tests/test_speculative_parser.cpp index 876b3a52..7e92fe61 100644 --- a/tests/test_speculative_parser.cpp +++ b/tests/test_speculative_parser.cpp @@ -166,7 +166,7 @@ TEST_CASE("Parsed chunk rows split edge fragments from complete rows", "[raw_csv SECTION("continuation chunk keeps leading partial row as prefix") { std::vector parsed_rows; auto chunk = std::make_shared(" value\",2\n3,Bob\n4,\"tail"); - ParserChunkOptions options(ParserDFAState(true), false); + ParserChunkOptions options(ParserDFAState(true)); const auto parse_result = parser.parse_chunk(*chunk, chunk, parsed_rows, options); auto rows = split_parsed_chunk_rows( @@ -189,7 +189,7 @@ TEST_CASE("Parsed chunk rows split edge fragments from complete rows", "[raw_csv SECTION("continuation prefix preserves embedded newlines") { std::vector parsed_rows; auto chunk = std::make_shared("line one\nline two\",tail\nnext,row\npartial"); - ParserChunkOptions options(ParserDFAState(true), false); + ParserChunkOptions options(ParserDFAState(true)); const auto parse_result = parser.parse_chunk(*chunk, chunk, parsed_rows, options); auto rows = split_parsed_chunk_rows( @@ -211,7 +211,7 @@ TEST_CASE("Parsed chunk rows split edge fragments from complete rows", "[raw_csv SECTION("continuation chunk without a record boundary is one prefix fragment") { std::vector parsed_rows; auto chunk = std::make_shared(" still inside the same quoted field"); - ParserChunkOptions options(ParserDFAState(true), false); + ParserChunkOptions options(ParserDFAState(true)); const auto parse_result = parser.parse_chunk(*chunk, chunk, parsed_rows, options); auto rows = split_parsed_chunk_rows( @@ -256,7 +256,7 @@ TEST_CASE("Speculative validator repairs wrongly seeded continuation chunks", "[ *chunk1, chunk1, parsed1_wrong, - ParserChunkOptions(ParserDFAState(false), false) + ParserChunkOptions(ParserDFAState(false)) ); auto rows1_wrong = split_parsed_chunk_rows( 1, @@ -312,7 +312,7 @@ TEST_CASE("Speculative validator batch-releases repaired rows to RowCollection", *chunk1, chunk1, parsed1_wrong, - ParserChunkOptions(ParserDFAState(false), false) + ParserChunkOptions(ParserDFAState(false)) ); auto rows1_wrong = split_parsed_chunk_rows( 1, @@ -370,7 +370,7 @@ TEST_CASE("Speculative validator carries split rows across chunks without record *chunk1, chunk1, parsed1, - ParserChunkOptions(ParserDFAState(true), false) + ParserChunkOptions(ParserDFAState(true)) ); auto rows1 = split_parsed_chunk_rows(1, *chunk1, chunk1, result1, std::move(parsed1), false); @@ -380,7 +380,7 @@ TEST_CASE("Speculative validator carries split rows across chunks without record *chunk2, chunk2, parsed2, - ParserChunkOptions(ParserDFAState(true), false) + ParserChunkOptions(ParserDFAState(true)) ); auto rows2 = split_parsed_chunk_rows(2, *chunk2, chunk2, result2, std::move(parsed2), false); @@ -416,7 +416,6 @@ TEST_CASE("ParallelCSVParser repairs speculative worker output in order", "[raw_ first.speculation.sequence_number = 0; first.speculation.assumed_start_state = ParserDFAState(false); first.starts_at_record_boundary = true; - first.scan_bom = true; auto chunk1 = std::make_shared("world\",ok\n2,done,ok\n"); SpeculativeParseChunk second; @@ -427,7 +426,6 @@ TEST_CASE("ParallelCSVParser repairs speculative worker output in order", "[raw_ second.speculation.sequence_number = 1; second.speculation.assumed_start_state = ParserDFAState(false); second.starts_at_record_boundary = false; - second.scan_bom = false; std::vector chunks; chunks.push_back(first); @@ -524,7 +522,6 @@ TEST_CASE("ParallelCSVParser can leave the final split row pending", "[raw_csv_p first.speculation.sequence_number = 0; first.speculation.assumed_start_state = ParserDFAState(false); first.starts_at_record_boundary = true; - first.scan_bom = true; auto chunk1 = std::make_shared(" beta"); SpeculativeParseChunk second; @@ -535,7 +532,6 @@ TEST_CASE("ParallelCSVParser can leave the final split row pending", "[raw_csv_p second.speculation.sequence_number = 1; second.speculation.assumed_start_state = ParserDFAState(true); second.starts_at_record_boundary = false; - second.scan_bom = false; std::vector chunks; chunks.push_back(first); From daf6f48ce51261fcceb7f83bff0d64add54e7501 Mon Sep 17 00:00:00 2001 From: Vincent La Date: Sat, 10 Oct 2026 12:35:33 -0700 Subject: [PATCH 4/5] Prepare v5.5.0 release metadata Co-Authored-By: Claude Opus 5.5 --- include/csv.hpp | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/csv.hpp b/include/csv.hpp index 9197df6e..766bddde 100644 --- a/include/csv.hpp +++ b/include/csv.hpp @@ -1,5 +1,5 @@ /* -CSV for C++, version 5.4.0 +CSV for C++, version 5.5.0 https://github.com/vincentlaucsb/csv-parser MIT License diff --git a/pyproject.toml b/pyproject.toml index bc9c670a..de2a6025 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ build-backend = "scikit_build_core.build" [project] name = "fastpycsv" -version = "5.4.0" +version = "5.5.0" description = "Fast ETL-focused CSV parsing for Python, backed by Vince's CSV Parser" readme = "python/README.md" requires-python = ">=3.9" From 5c58035ee3daf26d2106a69102c601458df4515d Mon Sep 17 00:00:00 2001 From: Vincent La Date: Sat, 10 Oct 2026 13:26:53 -0700 Subject: [PATCH 5/5] Consolidate BOM invariants and correct window comments --- ARCHITECTURE.md | 2 +- docs/source/internal_deep_dives.md | 5 +- include/internal/ARCHITECTURE.md | 36 +++++++- include/internal/BOMStrippingRefactor.md | 101 ----------------------- include/internal/parser/orchestrator.hpp | 7 +- tests/test_edge_cases_large_rows.cpp | 6 +- 6 files changed, 44 insertions(+), 113 deletions(-) delete mode 100644 include/internal/BOMStrippingRefactor.md diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 27d3e9ce..f960aa04 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -7,7 +7,6 @@ Primary architecture document: Subsystem deep-dive: - include/internal/THREADSAFE_DEQUE_DESIGN.md -- include/internal/BOMStrippingRefactor.md Operational/testing guidance: - AGENTS.md @@ -18,6 +17,7 @@ Operational/testing guidance: Notes: - Internal architecture content lives under include/internal to stay close to implementation. - Detailed file map, parser data flow, and component relationship diagrams are maintained in include/internal/ARCHITECTURE.md. +- Unicode BOM ownership and source-offset invariants are maintained in include/internal/ARCHITECTURE.md. - Queue synchronization details are maintained only in THREADSAFE_DEQUE_DESIGN.md to avoid duplication. - Always update or remove incorrect comments. - Public API comments should remain user-facing and avoid references to internal helper/function details. diff --git a/docs/source/internal_deep_dives.md b/docs/source/internal_deep_dives.md index f06d919c..b781e83d 100644 --- a/docs/source/internal_deep_dives.md +++ b/docs/source/internal_deep_dives.md @@ -2,11 +2,10 @@ # Internal / Deep Dives -These notes document parser internals, concurrency invariants, and refactor -plans. They are useful when debugging or changing csv-parser itself, but most +These notes document parser internals and concurrency invariants. +They are useful when debugging or changing csv-parser itself, but most users can stay with the README and public API reference. - @subpage internal_architecture - @subpage csv_field_journey - @subpage threadsafe_deque_design -- @subpage bom_stripping_refactor diff --git a/include/internal/ARCHITECTURE.md b/include/internal/ARCHITECTURE.md index 07e2c236..6b0155e2 100644 --- a/include/internal/ARCHITECTURE.md +++ b/include/internal/ARCHITECTURE.md @@ -72,7 +72,7 @@ Two independent parser paths exist and must be kept behaviorally aligned: - CSVParserCore - Templated, non-virtual byte parser core in parser/core.hpp. - Owns DFA state, field/row construction, and concrete row-sink emission. - - Only ever sees BOM-free bytes; Unicode BOM handling belongs to the orchestrator. + - Receives source windows after leading BOM handling by the orchestrator. - Source adapters feed byte windows into it; it does not own file, mmap, or stream source mechanics. - PermissiveParsePolicy @@ -100,7 +100,6 @@ Two independent parser paths exist and must be kept behaviorally aligned: - Chooses serial CSVParserCore parsing or speculative parallel parsing for a byte window. - Owns Unicode BOM handling: scans the first source window once, rejects UTF-16/UTF-32, and hands parsers a BOM-free view while reporting completed prefixes in source bytes. - See @ref bom_stripping_refactor. - MmapParser - Reads chunks from memory maps and handles chunk-transition remainder. @@ -230,6 +229,39 @@ Fields spanning chunk boundaries must not be split/corrupted. Mmap and stream parsers must preserve the same externally observable behavior. +### Unicode BOM handling and source offsets + +`CSVParseOrchestrator` checks the first source window once, before dispatching +serial or speculative parsing. It rejects UTF-16/UTF-32 with a transcoder error +and records a leading UTF-8 BOM for `CSVReader::utf8_bom()`. Only that first BOM +is removed; another BOM remains part of the CSV data. Parser cores and +speculative chunks carry no BOM state. Format guessing separately checks and +strips the leading BOM once per `guess_format()` call, before scoring delimiter +candidates. + +Both adapters' first windows include their pre-read head buffer, up to 500KB; +the stream adapter may append more bytes before parsing. A first window too +short to classify a BOM therefore occurs only at end of input. + +Source adapters advance in original source bytes while parsers receive +`chunk.substr(skip)` at `base_offset + skip`: + +- `RawCSVData::source_start` identifies the source position of the parsed view. + A first row after a UTF-8 BOM starts at byte 3, and its `raw_str()` excludes + those three bytes. Field offsets remain relative to the backing view. +- `CSVParseWindowResult::complete_prefix_length` includes the skipped bytes, + also reported as `skipped_prefix_length`. Mmap re-reads and stream leftovers + must begin after this prefix, even when the window completed no row. +- Mmap head-window fallback uses `completed_row()` rather than position + arithmetic: skipping a BOM advances the source position without completing + a row. A BOM-only source emits no row and still consumes the BOM. + +Regression coverage lives in `tests/test_read_csv.cpp` (`[read_unicode_bom]`) +for both adapters, guessed/explicit formats, encoding rejection, tiny inputs, +row offsets, and a 600K-row file parsed serially and speculatively. The BOM +variant of `[issue_337]` in `tests/test_edge_cases_large_rows.cpp` covers a +header longer than the mmap head buffer. + ### Field storage and conversion contract The parser realizes doubled-quote fields into `RawCSVData::quote_arena`; ordinary diff --git a/include/internal/BOMStrippingRefactor.md b/include/internal/BOMStrippingRefactor.md deleted file mode 100644 index a9b2a83d..00000000 --- a/include/internal/BOMStrippingRefactor.md +++ /dev/null @@ -1,101 +0,0 @@ -@page bom_stripping_refactor BOM Stripping Refactor - -# BOM Stripping Refactor - -**Status:** implemented in v5.5.0. - -## Context - -`CSVParserCore` used to own BOM detection through `ParserChunkOptions::scan_bom`. -Every core instance (the serial parser, each speculative worker, the repair -parser, and each format-guessing parser) kept its own "already scanned" flag and -stripped the BOM by advancing its read position inside the chunk. - -That put a source-window concern inside the byte parser, and it caused three bugs: - -- **`utf8_bom()` was wrong for speculative stream input.** The first stream - window is the 500KB head plus a full read window, so it can be routed to - `parse_speculative_window()`. A speculative worker stripped the BOM and - recorded it on itself, but `CSVParseOrchestrator::utf8_bom()` read the serial - parser's flag, which never saw the BOM. -- **The first row included the BOM.** `CSVParserCore::parse()` set the row start - to 0 *before* stripping advanced `data_pos_`. So the first row's `raw_str()` - began with `EF BB BF` and its `byte_offset()` was 0. Field values were correct, - because field starts were computed from `data_pos_`. -- **A re-read first window kept the BOM.** When the mmap head window completed - no row (issue #337), `MmapParser` rewound to offset 0 and parsed a full window. - The serial core had already marked its scan as done, so the BOM bytes became - part of the first column name. - -## Design - -BOM handling is a window-boundary concern owned by `CSVParseOrchestrator` -(`parser/orchestrator.hpp`): - -1. On the first `parse_window()` call only, `get_bom_skip_or_throw()` inspects - the leading bytes. A flag records that the scan happened; there is no - inference from `base_offset == 0`. -2. UTF-16 and UTF-32 BOMs throw before any serial or speculative parser sees the - window. -3. A UTF-8 BOM sets the orchestrator's `utf8_bom_` and is skipped. The parser - receives `chunk.substr(skip)` and `base_offset + skip`. -4. The returned `CSVParseWindowResult` adds the skipped bytes back into - `complete_prefix_length` and also reports them as `skipped_prefix_length`. - -`CSVParserCore` and the speculative chunk types no longer carry any BOM state. -`ParserChunkOptions` is just `(initial_state, source_start)`. - -Format guessing (`parser/guessing.cpp`) strips the BOM itself, once per -`guess_format()` call rather than once per candidate delimiter. It therefore -still rejects UTF-16/UTF-32 during `CSVReader` construction. - -The first window is always the adapter's head buffer, which holds -min(source size, 500KB) bytes. So a first window too short to classify a BOM -happens only at end of input, and the scan does not need to defer. - -## Offset Invariants - -Source adapters speak in original source-byte offsets. Parser cores see a -BOM-free view. - -- `RawCSVData::source_start` describes the original source position of the view - the core parsed. For the first window of a BOM file this is `base_offset + 3`, - so the first row's `byte_offset()` is 3 and `raw_str()` excludes the BOM. -- Field offsets stored in `RawCSVData` are relative to the backing view they - reference, which is unchanged by the skip. -- `complete_prefix_length` includes skipped BOM bytes. `MmapParser`'s - `mmap_pos -= (length - complete_prefix_length)` and `StreamParser`'s - `leftover_` / `stream_pos_` handling therefore advance past the BOM even when - no row completed, so a re-read never starts at the BOM. -- Code that needs to know whether a window completed a row must use - `CSVParseWindowResult::completed_row()`, not position arithmetic. A skipped - BOM advances the source position without completing a row. `MmapParser::next()` - relies on this to decide whether the head buffer needs a follow-up read - (issue #337). -- A BOM-only first window produces no row and still advances past the BOM. - -## Non-goals - -- Parsing UTF-16 or UTF-32 directly, or transcoding automatically. Those inputs - are rejected with the "use a transcoder first" error. -- Changing delimiter handling, quoting behavior, or row materialization. -- Making `CSVReader::iterator` multi-pass or caching source chunks. - -## Test Coverage - -`tests/test_read_csv.cpp` (`[read_unicode_bom]`) covers each case through both -the mmap and stream constructors: - -- UTF-8 BOM stripping with guessed and explicit formats, and `utf8_bom()`. -- Only one leading BOM is stripped. A second BOM stays in the first field, which - guards against double stripping. -- UTF-16 LE/BE and UTF-32 LE/BE rejection, including UTF-32 LE (`FF FE 00 00`) - not being reported as UTF-16. -- Empty input, a BOM-only file, and a BOM followed by one unterminated row. -- The first row's `raw_str()` and `byte_offset()` exclude the BOM. -- A 600K-row BOM file crossing chunk boundaries with serial parsing and with - speculative parsing forced on. -- `guess_format()` on BOM and UTF-16 files. - -`tests/test_edge_cases_large_rows.cpp` (`[issue_337]`) covers a UTF-8 BOM before -a header row longer than the mmap head buffer. diff --git a/include/internal/parser/orchestrator.hpp b/include/internal/parser/orchestrator.hpp index d011b109..f02b15bb 100644 --- a/include/internal/parser/orchestrator.hpp +++ b/include/internal/parser/orchestrator.hpp @@ -121,9 +121,10 @@ namespace csv { // mmap head that completed no row, issue #337) from starting at // the BOM again. // - // Both adapters make the first window their head buffer, which - // holds min(source size, 500KB) bytes, so a first window too - // short to classify a BOM only happens at end of input. + // Both adapters' first windows include the pre-read head buffer + // (up to 500KB); streams may append more bytes before parsing. + // A first window too short to classify a BOM therefore occurs + // only at end of input. size_t bom_skip = 0; if (!this->bom_scanned_) { bom_skip = get_bom_skip_or_throw(chunk, this->utf8_bom_); diff --git a/tests/test_edge_cases_large_rows.cpp b/tests/test_edge_cases_large_rows.cpp index 3ea79c17..1d2d0898 100644 --- a/tests/test_edge_cases_large_rows.cpp +++ b/tests/test_edge_cases_large_rows.cpp @@ -382,9 +382,9 @@ TEST_CASE("Issue #337 - First row longer than the mmap head buffer", "[issue_337 } SECTION("UTF-8 BOM before a header row longer than the head buffer") { - // The mmap head window completes no row, so MmapParser rewinds and - // re-reads from the start of the source. The BOM must still be - // skipped on that second read instead of becoming part of "A...". + // The mmap head window completes no row, so MmapParser re-reads from + // the first row's start, after the BOM. The skipped prefix must advance + // the source offset without suppressing this fallback read. const std::string long_name = "A" + std::string(600 * 1024, 'x'); const std::string data = "\xEF\xBB\xBF" + long_name + ",B,C\n" + body;