GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte by HannaWeissberg · Pull Request #50483 · apache/arrow

HannaWeissberg · 2026-07-12T14:28:50Z

Rationale for this change

Fixes #50481. arrow::csv::TableReader/BlockParser can 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) used _mm_cmpistrc, an implicit-length SSE4.2 string-compare intrinsic that treats 0x00 as 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 — RunBulkFilter then trusts the filter's "no special chars" answer and bulk-copies the whole word, silently swallowing the structural character.

This switches to the explicit-length _mm_cmpestrc, passing the true length of each operand (sizeof(WordType) for the 8-byte data word, sizeof(BulkFilterType) for the 16-byte filter — derived from the type rather than hardcoded, so it stays correct if this class is ever widened to a larger SIMD register) instead of relying on NUL-termination.

Also adds a regression test (BlockParser.EmbeddedNulInQuotedFieldAfterBulkFilterActivates in parser_test.cc) that reproduces the exact trigger: enough filler rows to cross the bulk filter's average-bytes-per-value activation threshold, followed by a row whose first field has a real embedded NUL immediately before its closing quote.

Are these changes tested?

Yes. Verified locally (not just by inspection):

Reverting just the header fix while keeping the new test reproduces the exact predicted failure: Expected 64 columns, got 1: "abc\0def",... and a wrong row count.
Restoring the fix makes that test pass.
The full arrow-csv-test suite (272 tests, 47 suites — parser, chunker, converter, reader, writer, column_builder, column_decoder) passes with the fix, with no regressions.

Are there any user-facing changes?

No API changes. This only affects internal CSV byte-scanning correctness on SSE4.2-capable x86 builds; behavior for data without embedded NUL bytes is unchanged (verified via a direct intrinsic-level comparison of both instructions against the actual filter construction, with and without a NUL present).

This PR (fix, test, and description) was AI-generated (Claude), under human review and local verification described above.

GitHub Issue: [C++] SSE4.2 CSV reader silently mis-parses rows when a field contains an embedded NUL byte #50481

…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>

github-actions · 2026-07-12T14:29:15Z

⚠️ GitHub issue #50481 has been automatically assigned in GitHub to PR creator.

Copilot

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::Matches from implicit-length _mm_cmpistrc to explicit-length _mm_cmpestrc to 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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

raulcd · 2026-07-13T07:29:49Z

@AntoinePrv I've added you as a reviewer in case you can take a look

AntoinePrv

LGTM

HannaWeissberg · 2026-07-15T09:48:36Z

CI failure appears to be unrelated to this PR, could a committer check/re-run it please?

pitrou · 2026-07-21T15:59:13Z

+  // 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


Instead of having filler rows with no NUL bytes, this test would be more robust by putting NUL bytes in every cell value, IMHO.

pitrou · 2026-07-21T16:04:48Z

+    // 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_,


The explicit-length instructions are slower according to various source out there, we should run some benchmarks to see if there are significant regressions.

pitrou · 2026-07-21T16:05:25Z

@ursabot please benchmark lang=C++

rok · 2026-07-21T16:05:34Z

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.

pitrou · 2026-07-21T16:18:11Z

I see massive regressions locally on an AMD Zen 2 CPU:

              benchmark        baseline       contender  change %                                                                                                                                                             counters
 ParseCSVFlightsExample 695.448 MiB/sec 623.760 MiB/sec   -10.308  {'family_index': 8, 'per_family_instance_index': 0, 'run_name': 'ParseCSVFlightsExample', 'repetitions': 8, 'repetition_index': 4, 'threads': 1, 'iterations': 199}
  ParseCSVStocksExample   2.068 GiB/sec   1.032 GiB/sec   -50.091  {'family_index': 10, 'per_family_instance_index': 0, 'run_name': 'ParseCSVStocksExample', 'repetitions': 8, 'repetition_index': 3, 'threads': 1, 'iterations': 286}
ParseCSVVehiclesExample   3.264 GiB/sec   1.613 GiB/sec   -50.578  {'family_index': 9, 'per_family_instance_index': 0, 'run_name': 'ParseCSVVehiclesExample', 'repetitions': 8, 'repetition_index': 1, 'threads': 1, 'iterations': 80}
  ChunkCSVStocksExample   4.602 GiB/sec   1.299 GiB/sec   -71.774   {'family_index': 5, 'per_family_instance_index': 0, 'run_name': 'ChunkCSVStocksExample', 'repetitions': 8, 'repetition_index': 3, 'threads': 1, 'iterations': 660}
ChunkCSVVehiclesExample   7.440 GiB/sec   2.016 GiB/sec   -72.908 {'family_index': 4, 'per_family_instance_index': 0, 'run_name': 'ChunkCSVVehiclesExample', 'repetitions': 8, 'repetition_index': 6, 'threads': 1, 'iterations': 186}

pitrou · 2026-07-21T16:25:05Z

To avoid the regression, we could arrange to run the bulk filter only if the entire block does not contain a NUL byte (which can be checked extremely quickly using memchr).

NUL bytes being exceptional in CSV files, common cases would not see a slowdown.

(of course, this would be only for SSE4.2, not NEON)

HannaWeissberg requested a review from pitrou as a code owner July 12, 2026 14:28

Copilot AI review requested due to automatic review settings July 12, 2026 14:28

Copilot started reviewing on behalf of HannaWeissberg July 12, 2026 14:29 View session

github-actions Bot added Component: C++ awaiting review Awaiting review labels Jul 12, 2026

Copilot AI reviewed Jul 12, 2026

View reviewed changes

Comment thread cpp/src/arrow/csv/lexing_internal.h Outdated

Comment thread cpp/src/arrow/csv/parser_test.cc

Adding int casting to sizeof()

576461b

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 12, 2026 14:34

Copilot started reviewing on behalf of HannaWeissberg July 12, 2026 14:34 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

Adding full size assert check

4b2dca0

Copilot AI review requested due to automatic review settings July 12, 2026 14:41

Copilot started reviewing on behalf of HannaWeissberg July 12, 2026 14:41 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

Comment thread cpp/src/arrow/csv/parser_test.cc Outdated

Fix test

92cb521

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 12, 2026 15:10

Copilot started reviewing on behalf of HannaWeissberg July 12, 2026 15:11 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

raulcd requested a review from AntoinePrv July 13, 2026 07:29

AntoinePrv approved these changes Jul 15, 2026

View reviewed changes

pitrou reviewed Jul 21, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 21, 2026

Uh oh!

Conversation

HannaWeissberg commented Jul 12, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jul 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

raulcd commented Jul 13, 2026

Uh oh!

AntoinePrv left a comment

Choose a reason for hiding this comment

Uh oh!

HannaWeissberg commented Jul 15, 2026

Uh oh!

pitrou Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

pitrou Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 21, 2026

Uh oh!

rok commented Jul 21, 2026

Uh oh!

pitrou commented Jul 21, 2026

Uh oh!

pitrou commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HannaWeissberg commented Jul 12, 2026 •

edited by github-actions Bot

Loading

pitrou commented Jul 21, 2026 •

edited

Loading