You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "HannaWeissberg (via GitHub)" <gi...@apache.org> on 2026/07/12 14:28:53 UTC
[PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
HannaWeissberg opened a new pull request, #50483:
URL: https://github.com/apache/arrow/pull/50483
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629511342
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
Review Comment:
Instead of hardcoding the memchr test here, how about adding a method on bulk filters such as `bool CanUseOnBlock(std::string_view)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5036267680
@ursabot please benchmark lang=C++
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629517074
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -533,12 +534,13 @@ class BlockParserImpl {
}
if (batch_.num_rows_ > start_num_rows && batch_.num_cols_ > 0) {
- // Use bulk filter only if average value length is >= 10 bytes,
- // as the bulk filter has a fixed cost that isn't compensated
- // when values are too short.
+ // Use bulk filter only if average value length is >= 10 bytes
+ // (its fixed cost isn't compensated for short values), and the block
+ // has no embedded NUL bytes (see block_has_nul_).
Review Comment:
I think Copilot is right that `csv/chunker.cc` must be fixed as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3630527766
##########
cpp/src/arrow/csv/chunker.cc:
##########
@@ -56,6 +56,10 @@ class Lexer {
// Decide whether it's worth using a bulk filter over the given data area
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;
+ }
Review Comment:
`ShouldUseBulkFilter()` now calls `bulk_filter_.CanUseOnBlock()` on the full `[data, data_end)` range before doing the existing 32-word probe. On SSE4.2 builds this performs a full `memchr` scan even in cases where the probe would quickly decide bulk filtering is not worthwhile, adding avoidable O(n) work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5046945290
I enjoyed contributing, thank you @pitrou for helping!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5045747539
@ursabot please benchmark lang=C++
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629486041
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -533,12 +534,13 @@ class BlockParserImpl {
}
if (batch_.num_rows_ > start_num_rows && batch_.num_cols_ > 0) {
- // Use bulk filter only if average value length is >= 10 bytes,
- // as the bulk filter has a fixed cost that isn't compensated
- // when values are too short.
+ // Use bulk filter only if average value length is >= 10 bytes
+ // (its fixed cost isn't compensated for short values), and the block
+ // has no embedded NUL bytes (see block_has_nul_).
Review Comment:
This change works around the SSE4.2 implicit-length compare by disabling the bulk filter when a NUL exists in the block, but `SSE42Filter::Matches` in `lexing_internal.h` still uses `_mm_cmpistrc` (implicit length) and the row chunker (`chunker.cc` Lexer bulk filter) still relies on the same filter. That means NUL-related misparsing can still occur via other call sites, and it also doesn’t match the PR description (which describes switching to `_mm_cmpestrc`). Consider fixing `SSE42Filter::Matches` directly so all users of `PreferredBulkFilterType` are corrected without an O(n) pre-scan / behavior change.
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
+ // Only the SSE4.2 bulk filter is affected by embedded NUL bytes,
+ // so only scan for them when that filter is available.
+ if (!block_has_nul_ && !view.empty() &&
+ memchr(view.data(), '\0', view.length()) != nullptr) {
+ block_has_nul_ = true;
+ }
+#endif
}
if (total_view_length > std::numeric_limits<uint32_t>::max()) {
Review Comment:
`block_has_nul_` is computed before parsing, but `use_bulk_filter_` can carry over from a previous Parse() call. If it was already true, this block may still enter the bulk-filter parsing path before `use_bulk_filter_` gets recomputed, so an embedded NUL can still trigger the original mis-parse. Consider clearing `use_bulk_filter_` for the current block once a NUL is detected (before any parsing begins).
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -936,5 +936,56 @@ TEST(BlockParser, RowNumberAppendedToError) {
}
}
+TEST(BlockParser, EmbeddedNulBytesDisableBulkFilter) {
+ // Regression test for GH-50481: the bulk filter's implicit-length SIMD
+ // compare can miss a delimiter sharing a word with an embedded NUL byte.
+ // The fix disables the bulk filter for any block containing a NUL, so
+ // every cell here carries one. num_cols is explicit so the bulk filter
+ // isn't delayed by the single-row column-count-inference parse path.
+ constexpr int32_t num_cols = 64;
Review Comment:
This test constructs NUL bytes in *every* cell, which (with the current implementation) guarantees `block_has_nul_` is true and therefore the bulk filter will be disabled for the entire block. That means the test won’t catch regressions in the SIMD bulk-filter path that originally caused GH-50481, and it also hard-codes assumptions about the intended fix (“disable the bulk filter”) rather than validating correctness when bulk filtering is active. Consider adjusting the test data so the parser is forced into bulk-filter mode (long NUL-free filler), and only the trigger row contains the embedded NUL adjacent to a structural character, so the SIMD path is exercised once the root fix is applied.
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
+ // Only the SSE4.2 bulk filter is affected by embedded NUL bytes,
+ // so only scan for them when that filter is available.
+ if (!block_has_nul_ && !view.empty() &&
+ memchr(view.data(), '\0', view.length()) != nullptr) {
Review Comment:
`memchr` is called unqualified while including `<cstring>`. In C++, `<cstring>` only guarantees `std::memchr`; relying on `::memchr` being injected into the global namespace can be non-portable. Prefer `std::memchr` here (similar to `csv/writer.cc`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3630157897
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -138,6 +144,13 @@ class SSE42Filter {
_SIDD_UBYTE_OPS | _SIDD_CMP_EQUAL_ANY);
}
+ // _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.
Review Comment:
Let's explain why we don't use the explicit-length compare instruction
```suggestion
// _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.
// We could instead use the explicit-length compare _mm_cmpestrc but
// it comes with a massive performance cost on some CPUs
// (some benchmarks were measured to be twice slower in
// https://github.com/apache/arrow/pull/50483).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5045748372
Benchmark runs are scheduled for commit 862cba9d83de346e41c107a46dea020c954b66e4. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5036269120
Benchmark runs are scheduled for commit 92cb521393659dbde7579d744c7b1fea0f33a938. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5036390850
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}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629909153
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -138,6 +144,13 @@ class SSE42Filter {
_SIDD_UBYTE_OPS | _SIDD_CMP_EQUAL_ANY);
}
+ // _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;
Review Comment:
The PR description (and linked issue) state the fix is switching SSE4.2 scanning from implicit-length `_mm_cmpistrc` to explicit-length `_mm_cmpestrc`, but this code still uses `_mm_cmpistrc` and instead relies on pre-scanning blocks for NUL bytes + disabling the bulk filter. Please either update the PR description/tests to match the actual approach (including its perf/behavior implications), or implement the `_mm_cmpestrc` change so NUL bytes are handled without needing a full-block `memchr` scan and bulk-filter disable.
##########
cpp/src/arrow/csv/chunker_test.cc:
##########
@@ -341,6 +341,26 @@ TEST_P(BaseChunkerTest, EscapingAndQuoting) {
}
}
+TEST_P(BaseChunkerTest, EmbeddedNulBytesDisableBulkFilter) {
+ // Regression test for GH-50481 in Lexer's own bulk filter (used for
+ // chunking, separately from BlockParser's).
+ //
+ // 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');
Review Comment:
This test name/comment hard-code the current mitigation (“DisableBulkFilter”) instead of asserting the intended behavior (embedded NUL bytes must not corrupt chunk boundaries). If the underlying SIMD filter is fixed (e.g., explicit-length compares), the test should still read correctly without implying bulk filtering must be disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629601187
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -533,12 +534,13 @@ class BlockParserImpl {
}
if (batch_.num_rows_ > start_num_rows && batch_.num_cols_ > 0) {
- // Use bulk filter only if average value length is >= 10 bytes,
- // as the bulk filter has a fixed cost that isn't compensated
- // when values are too short.
+ // Use bulk filter only if average value length is >= 10 bytes
+ // (its fixed cost isn't compensated for short values), and the block
+ // has no embedded NUL bytes (see block_has_nul_).
Review Comment:
Will add on chunker.cc, not on matches because of the regression
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629881315
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
+ // Only the SSE4.2 bulk filter is affected by embedded NUL bytes,
+ // so only scan for them when that filter is available.
+ if (!block_has_nul_ && !view.empty() &&
+ memchr(view.data(), '\0', view.length()) != nullptr) {
+ block_has_nul_ = true;
+ }
+#endif
}
if (total_view_length > std::numeric_limits<uint32_t>::max()) {
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629882963
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
+ // Only the SSE4.2 bulk filter is affected by embedded NUL bytes,
+ // so only scan for them when that filter is available.
+ if (!block_has_nul_ && !view.empty() &&
+ memchr(view.data(), '\0', view.length()) != nullptr) {
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5048221973
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 862cba9d83de346e41c107a46dea020c954b66e4.
There were 9 benchmark results indicating a performance regression:
- Pull Request Run on `amd64-c6a-4xlarge-linux` at [2026-07-22 14:55:07Z](https://conbench.arrow-dev.org/compare/runs/46287fc2bbeb4014a3322d97c6470f42...5b49052ec05843b298b5baf014f63043/)
- [`ChunkCSVVehiclesExample` (C++) with source=cpp-micro, suite=arrow-csv-parser-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/06a5122d18c5729f8000abb7062e7652...06a60da34d347cea8000162aa18e30c5)
- Pull Request Run on `test-mac-arm` at [2026-07-22 14:08:33Z](https://conbench.arrow-dev.org/compare/runs/0855fd98d15b4a69ac0d649a93933136...ac5e9b55dfe9456abefcab7f56ddd370/)
- [`BenchmarkTemporalBinary` (C++) with params=<NanosecondsBetween, non_zoned>/4194304/100, source=cpp-micro, suite=arrow-compute-scalar-temporal-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/06a56e7d805c737d800087705b428864...06a60d28c8d5770e8000266f30044608)
- and 7 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/88972192474) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-4979213856
CI failure appears to be unrelated to this PR, could a committer check/re-run it please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5045884727
Hmm, can run `pre-commit run -a cpp` to fix the lint issues?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50483:
URL: https://github.com/apache/arrow/pull/50483
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-4951536770
:warning: GitHub issue #50481 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5039992517
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 92cb521393659dbde7579d744c7b1fea0f33a938.
There were 12 benchmark results indicating a performance regression:
- Pull Request Run on `test-mac-arm` at [2026-07-21 22:26:58Z](https://conbench.arrow-dev.org/compare/runs/0855fd98d15b4a69ac0d649a93933136...1ffe91777a5a4dc7aecffa15474b9907/)
- [`GrouperWithMultiTypes` (C++) with params="{utf8_view}"/4096/1, source=cpp-micro, suite=arrow-compute-grouper-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/06a56e7fd77e72168000dbb2be0102d8...06a5ff5999ce726280002c5494c95e92)
- [`GrouperWithMultiTypes` (C++) with params="{utf8_view}"/1024/1, source=cpp-micro, suite=arrow-compute-grouper-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/06a56e7fc9587a548000d21c44f2eda9...06a5ff598c5b73138000c72ab53ddabd)
- and 10 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/88787135437) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5047222951
There's always more to do if you would like to keep contributing, by the way :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3566512059
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -936,5 +936,45 @@ TEST(BlockParser, RowNumberAppendedToError) {
}
}
+TEST(BlockParser, EmbeddedNulInQuotedFieldAfterBulkFilterActivates) {
+ // Regression test for GH-50481. Once the running average value length
+ // crosses the bulk filter's activation threshold, a NUL byte embedded in a
+ // quoted field could hide the real closing quote if both landed in the
+ // 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
+
+ std::string csv;
+ for (int32_t r = 0; r < num_filler_rows; ++r) {
+ for (int32_t c = 0; c < num_cols; ++c) {
+ if (c) csv += ',';
+ // 12 bytes/value, well above the bulk filter's 10-bytes/value
+ // activation threshold.
+ csv += "xxxxxxxxxxxx";
+ }
+ csv += '\n';
+ }
+ // This row's first field carries a real embedded NUL byte immediately
+ // before its closing quote - the byte pattern a misaligned
+ // implicit-length SIMD scan can hide.
+ csv += "\"abc";
+ csv += '\0';
+ csv += "def\"";
+ for (int32_t c = 1; c < num_cols; ++c) {
+ csv += ",xxxxxxxxxxxx";
+ }
+ csv += '\n';
+
+ BlockParser parser(ParseOptions::Defaults());
Review Comment:
This test currently constructs `BlockParser` with `num_cols=-1`, which triggers the 1-row "infer number of columns" parse path. With 64 columns × 12-byte values, that first-row inference is already enough to enable `use_bulk_filter_` before the 512 filler rows are processed, so the test doesn't actually exercise the intended "bulk filter activates after enough rows" transition described in the comment/name. Constructing the parser with a known column count makes the bulk-filter activation depend on the filler rows/chunking as intended and keeps the regression aligned with GH-50481's trigger condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5067145242
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 3d771d0889e7e5df7f4d000fcfba8ca6c1b3d39e.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/89422608983) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3566486447
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -936,5 +936,44 @@ TEST(BlockParser, RowNumberAppendedToError) {
}
}
+TEST(BlockParser, EmbeddedNulInQuotedFieldAfterBulkFilterActivates) {
+ // Regression test for GH-50481. Once the running average value length
+ // crosses the bulk filter's activation threshold, a NUL byte embedded in a
+ // quoted field could hide the real closing quote if both landed in the
+ // 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
+
+ std::string csv;
+ for (int32_t r = 0; r < num_filler_rows; ++r) {
+ for (int32_t c = 0; c < num_cols; ++c) {
+ if (c) csv += ',';
+ // 12 bytes/value, well above the bulk filter's 10-bytes/value
+ // activation threshold.
+ csv += "xxxxxxxxxxxx";
+ }
+ csv += '\n';
+ }
+ // This row's first field carries a real embedded NUL byte immediately
+ // before its closing quote - the byte pattern a misaligned
+ // implicit-length SIMD scan can hide.
+ csv += "\"abc";
+ csv += '\0';
+ csv += "def\"";
+ for (int32_t c = 1; c < num_cols; ++c) {
+ csv += ",xxxxxxxxxxxx";
+ }
+ csv += '\n';
+
+ BlockParser parser(ParseOptions::Defaults());
+ AssertParseFinal(parser, csv);
+ ASSERT_EQ(parser.num_rows(), num_filler_rows + 1);
+
+ std::vector<std::string> last_row;
+ GetLastRow(parser, &last_row);
+ ASSERT_EQ(last_row[0], std::string("abc\0def", 7));
Review Comment:
The test indexes `last_row[0]` without first asserting that the row has any columns; if parsing regresses in a way that yields 0 columns, this becomes undefined behavior and can crash instead of producing a clear assertion failure. Add a size assertion before indexing.
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -133,8 +133,11 @@ class SSE42Filter {
explicit SSE42Filter(const ParseOptions& options) : filter_(MakeFilter(options)) {}
bool Matches(WordType w) const {
- // Look up every byte in `w` in the SIMD filter.
- return _mm_cmpistrc(_mm_set1_epi64x(w), filter_,
+ // 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), sizeof(WordType), filter_,
+ sizeof(BulkFilterType),
_SIDD_UBYTE_OPS | _SIDD_CMP_EQUAL_ANY);
Review Comment:
_mm_cmpestrc takes `int` lengths; passing `sizeof(...)` (a `size_t`) can trigger conversion warnings (and potentially -Werror failures) on some toolchains. Cast the sizes to `int` explicitly since the values are fixed and small here (8 and 16).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629972119
##########
cpp/src/arrow/csv/chunker_test.cc:
##########
@@ -341,6 +341,26 @@ TEST_P(BaseChunkerTest, EscapingAndQuoting) {
}
}
+TEST_P(BaseChunkerTest, EmbeddedNulBytesDisableBulkFilter) {
+ // Regression test for GH-50481 in Lexer's own bulk filter (used for
+ // chunking, separately from BlockParser's).
+ //
+ // 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');
Review Comment:
It tests disabling bulk filter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629621386
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -936,5 +936,56 @@ TEST(BlockParser, RowNumberAppendedToError) {
}
}
+TEST(BlockParser, EmbeddedNulBytesDisableBulkFilter) {
+ // Regression test for GH-50481: the bulk filter's implicit-length SIMD
+ // compare can miss a delimiter sharing a word with an embedded NUL byte.
+ // The fix disables the bulk filter for any block containing a NUL, so
+ // every cell here carries one. num_cols is explicit so the bulk filter
+ // isn't delayed by the single-row column-count-inference parse path.
+ constexpr int32_t num_cols = 64;
Review Comment:
The test is for checking if there's an embedded NUL byte
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "HannaWeissberg (via GitHub)" <gi...@apache.org>.
HannaWeissberg commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3629885057
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -561,8 +563,17 @@ class BlockParserImpl {
values_size_ = 0;
size_t total_view_length = 0;
+ block_has_nul_ = false;
for (const auto& view : views) {
total_view_length += view.length();
+#if defined(ARROW_HAVE_SSE4_2) && (defined(__x86_64__) || defined(_M_X64))
Review Comment:
Added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5044848917
On my local CPU (AMD Zen 2) the new approach entirely fixes the performance regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5046416064
I updated the PR description and will merge this PR if CI passes. Thank you @HannaWeissberg for doing this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50483:
URL: https://github.com/apache/arrow/pull/50483#discussion_r3623710653
##########
cpp/src/arrow/csv/lexing_internal.h:
##########
@@ -133,8 +133,11 @@ class SSE42Filter {
explicit SSE42Filter(const ParseOptions& options) : filter_(MakeFilter(options)) {}
bool Matches(WordType w) const {
- // Look up every byte in `w` in the SIMD filter.
- return _mm_cmpistrc(_mm_set1_epi64x(w), filter_,
+ // 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_,
Review 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.
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -936,5 +936,45 @@ TEST(BlockParser, RowNumberAppendedToError) {
}
}
+TEST(BlockParser, EmbeddedNulInQuotedFieldAfterBulkFilterActivates) {
+ // Regression test for GH-50481. Once the running average value length
+ // crosses the bulk filter's activation threshold, a NUL byte embedded in a
+ // quoted field could hide the real closing quote if both landed in the
+ // 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-5036452927
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 `memcmp`).
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50481: [C++] Fix CSV reader mis-parsing rows with an embedded NUL byte [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50483:
URL: https://github.com/apache/arrow/pull/50483#issuecomment-4955505142
@AntoinePrv I've added you as a reviewer in case you can take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org