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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
