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/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/include/internal/ARCHITECTURE.md b/include/internal/ARCHITECTURE.md index b745c58d..6b0155e2 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. + - 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 @@ -97,6 +98,8 @@ 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. - MmapParser - Reads chunks from memory maps and handles chunk-transition remainder. @@ -226,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 5a38ad8c..00000000 --- a/include/internal/BOMStrippingRefactor.md +++ /dev/null @@ -1,99 +0,0 @@ -@page bom_stripping_refactor BOM Stripping Refactor - -# 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. - -## 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 - -- 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. - -After that first scan, all parser-core invocations should receive -`ParserChunkOptions(..., false)` or an equivalent path that disables core-level -BOM scanning. - -## 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. - -## 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. - -## Suggested Implementation Order - -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. 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 1a7c298b..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; }; @@ -161,7 +155,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; + } }; } @@ -417,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; } @@ -485,7 +485,7 @@ namespace csv { chunk, std::move(owner), output, - ParserChunkOptions(this->initial_state_, true, source_start) + ParserChunkOptions(this->initial_state_, source_start) ); } @@ -550,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; @@ -670,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_; @@ -793,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, @@ -819,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_); @@ -837,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/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..f02b15bb 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,47 @@ 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' 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_); + 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 +180,6 @@ namespace csv { ); } - private: CSVParseWindowResult parse_serial_window( csv::string_view chunk, std::shared_ptr owner, @@ -176,9 +216,7 @@ namespace csv { owner, serial_chunk_size, this->scanner_, - base_offset, - 0, - base_offset == 0 + base_offset ); const speculative::ParallelCSVParseResult parse_result = this->speculative_parser_->parse_chunks( @@ -199,6 +237,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/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/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" diff --git a/tests/test_edge_cases_large_rows.cpp b/tests/test_edge_cases_large_rows.cpp index e4191d63..1d2d0898 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 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; + + 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 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);