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