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