GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte - #50483
Conversation
…d NUL byte
SSE42Filter::Matches used _mm_cmpistrc, an implicit-length SSE4.2
string-compare intrinsic that treats 0x00 as a terminator. Since it
scans 8 raw CSV bytes at a time, a NUL byte embedded in a field could
hide a real delimiter/quote/newline sharing the same 8-byte word,
causing RunBulkFilter to blindly bulk-copy the word and silently
mis-split the row.
Switch to the explicit-length _mm_cmpestrc, passing the true lengths
of both operands (sizeof(WordType) for the data word, sizeof(BulkFilterType)
for the filter) instead of relying on NUL-termination.
Verified locally: reverting just this fix reproduces the exact
predicted failure ("Expected 64 columns, got 1") in the added
regression test; with the fix, that test and the rest of the CSV
test suite (272 tests) pass.
This fix and its regression test were AI-generated (Claude), under
human review and local verification.
Co-Authored-By: Claude <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
Pull request overview
Fixes an SSE4.2-specific CSV parsing correctness bug where embedded NUL (0x00) bytes could cause the SIMD bulk-filter path to miss structural characters (quote/comma/newline), leading to silent row mis-splitting once bulk filtering activates.
Changes:
- Switch
SSE42Filter::Matchesfrom implicit-length_mm_cmpistrcto explicit-length_mm_cmpestrcto avoid NUL-termination behavior. - Add a regression test that forces bulk-filter activation and then parses a quoted field containing an embedded NUL right before the closing quote.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/arrow/csv/lexing_internal.h | Updates SSE4.2 bulk-filter matching to use explicit-length string-compare semantics. |
| cpp/src/arrow/csv/parser_test.cc | Adds a regression test covering embedded-NUL behavior after bulk filtering activates. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@AntoinePrv I've added you as a reviewer in case you can take a look |
|
CI failure appears to be unrelated to this PR, could a committer check/re-run it please? |
| // same 8-byte SIMD word, causing the parser to keep consuming subsequent | ||
| // bytes as if still inside the quoted field. | ||
| constexpr int32_t num_cols = 64; | ||
| constexpr int32_t num_filler_rows = 512; // = kTargetChunkSize / num_cols |
There was a problem hiding this comment.
Instead of having filler rows with no NUL bytes, this test would be more robust by putting NUL bytes in every cell value, IMHO.
| // Look up every byte in `w` in the SIMD filter. Use the explicit-length | ||
| // comparison since `w` may contain an embedded NUL byte, which the | ||
| // implicit-length _mm_cmpistrc would otherwise treat as a terminator. | ||
| return _mm_cmpestrc(_mm_set1_epi64x(w), static_cast<int>(sizeof(WordType)), filter_, |
There was a problem hiding this comment.
The explicit-length instructions are slower according to various source out there, we should run some benchmarks to see if there are significant regressions.
github-actions
Bot
added
awaiting committer review
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 92cb521. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
|
I see massive regressions locally on an AMD Zen 2 CPU: |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 92cb521. There were 12 benchmark results indicating a performance regression:
The full Conbench report has more details. |
…ompare @ursabot's benchmarks showed the previous _mm_cmpestrc fix caused 10-73% throughput regressions on several CSV parsing/chunking benchmarks (e.g. ChunkCSVVehiclesExample -72.9%), since the explicit-length compare is measurably slower than the implicit-length one it replaced. Per pitrou's suggestion, revert SSE42Filter::Matches to the original _mm_cmpistrc, and instead disable the bulk filter entirely for any block containing a NUL byte: BlockParserImpl now scans each block once via memchr (block_has_nul_), gating use_bulk_filter_ on it being unset. This keeps the fast path's cost paid only on NUL-free blocks, which is the common case, while blocks with an embedded NUL fall back to the already-correct per-byte path. This change and its test updates were AI-generated (Claude), under human review and local verification. Co-Authored-By: Claude <noreply@anthropic.com>
|
On my local CPU (AMD Zen 2) the new approach entirely fixes the performance regression. |
… staleness use_bulk_filter_ could stay set across Parse() calls, letting a new block's own NUL slip through. chunker.cc's Lexer has a separate bulk filter untouched by the parser.cc fix. Fix both, and factor the NUL check into CanUseOnBlock() on each filter class. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
cpp/src/arrow/csv/parser.cc:567
- On SSE4.2 builds,
bulk_filter.CanUseOnBlock(view)ends up doing a fullmemchrscan of every block up front, even in cases where the bulk filter may never activate (short values) and even for NUL-free data. That O(n) pre-scan can partially negate the bulk filter’s intended speedup on large inputs. Consider implementing the explicit-length intrinsic fix (so NULs are safe without a pre-scan) or otherwise deferring the NUL scan until the moment the parser is about to enableuse_bulk_filter_.
size_t total_view_length = 0;
block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
if (!block_has_nul_ && !bulk_filter.CanUseOnBlock(view)) {
cpp/src/arrow/csv/parser_test.cc:942
- This test name and comment encode the implementation detail (“DisableBulkFilter”) rather than the behavioral contract (embedded NUL bytes must not corrupt parsing). That makes the test misleading if the implementation later switches to a safer SIMD path (as described in the PR text) and no longer needs to disable bulk filtering.
TEST(BlockParser, EmbeddedNulBytesDisableBulkFilter) {
// Regression test for GH-50481: disables the bulk filter for any block
// with an embedded NUL, so every cell here carries one.
constexpr int32_t num_cols = 64;
| // _mm_cmpistrc is an implicit-length compare: it treats a NUL byte as a | ||
| // terminator and can miss a real delimiter/quote/newline sharing an 8-byte | ||
| // word with one. Never use this filter on a block that contains a NUL. | ||
| bool CanUseOnBlock(std::string_view data) const { | ||
| return data.empty() || std::memchr(data.data(), '\0', data.size()) == nullptr; |
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 862cba9. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
|
Hmm, can run |
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cpp/src/arrow/csv/chunker_test.cc:350
- The comment says the 256-byte probe in
ShouldUseBulkFilter()“decides to use the bulk filter”, but the updated implementation short-circuits onCanUseOnBlock()before running that probe when an embedded NUL is present (SSE4.2 case). This makes the test comment misleading.
// Lead-in with no structural bytes so ShouldUseBulkFilter's probe of the
// first 256 bytes decides to use the bulk filter here.
std::string lead_in(300, 'x');
| bool ShouldUseBulkFilter(const char* data, const char* data_end) { | ||
| if (!bulk_filter_.CanUseOnBlock( | ||
| std::string_view(data, static_cast<size_t>(data_end - data)))) { | ||
| return false; | ||
| } |
|
I updated the PR description and will merge this PR if CI passes. Thank you @HannaWeissberg for doing this! |
|
I enjoyed contributing, thank you @pitrou for helping me! |
|
There's always more to do if you would like to keep contributing, by the way :) |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 862cba9. There were 9 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 3d771d0. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
Fixes #50481.
arrow::csv::TableReader/BlockParsercan silently mis-split a row when a text field contains an embedded NUL (0x00) byte, once the reader has processed enough data to switch into its SIMD "bulk filter" scanning path.What changes are included in this PR?
SSE42Filter::Matches(cpp/src/arrow/csv/lexing_internal.h) uses_mm_cmpistrc, an implicit-length SSE4.2 string-compare intrinsic that treats0x00as a terminator in both operands. Since the caller feeds it 8 raw CSV bytes at a time, a real quote/comma/newline sharing an 8-byte word with an embedded NUL becomes invisible to the filter —RunBulkFilterthen trusts the filter's "no special chars" answer and bulk-copies the whole word, silently swallowing the structural character.An initial version of this fix switched to the explicit-length
_mm_cmpestrc, passing the true length of each operand instead of relying on NUL-termination. However, this proves to yield massive performance regressions on some CPUs (for example, some CSV parsing benchmarks became 2x slower on an AMD Zen 2 CPU). This is corroborated by the instruction timing from Agner Fog's instruction tables.The final version of the fix instead checks for NUL bytes in an entire block. This can be done very quickly using
memchr(which is typically vectorized on modern libc's). Since most real-world CSV files don't have embedded NUL bytes, the SIMD bulk filter optimization will most of the time still be enabled.Are these changes tested?
Yes, using new regression tests that reproduce the exact trigger.
Are there any user-facing changes?
No, just a bugfix.
This PR (fix, test, and description) was AI-generated (Claude), under human review and local verification described above.