You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2024/02/01 18:04:31 UTC
[PR] GH-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
pitrou opened a new pull request, #39892:
URL: https://github.com/apache/arrow/pull/39892
### Rationale for this change
When writing the CSV reader, we thought that the parser not finding the same line limits as the chunker should never happen, hence the terse "chunker out of sync" error message.
It turns out that, if the input contains multiline cell values and the `newlines_in_values` option was not enabled, the chunker can happily delimit a block on a newline that's inside a quoted string. The parser will then see truncated data and will stop parsing, yielding a parsed size that's smaller than the first block (see added comment in the code).
### What changes are included in this PR?
* Add some parser tests that showcase the condition encountered in GH-39857
* Improve error message to guide users towards the solution
### Are these changes tested?
There's no functional change, the error message itself isn't tested.
### Are there any user-facing changes?
No.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1476224449
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
I don't think so; BlockParser::Parse's docstring states that only the last block is allowed to be truncated. That suggests that both these calls should raise an error rather than return. IIUC, we expect the chunker to ensure that the parser need not handle partial blocks. I think instead of unit testing the parser in an erroneous condition, we should test the reader and ensure that an error is raised and the parser never enters that condition.
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
+ AssertParsePartial(parser, Views({"a,b,c\nd,", "e,f\n"}), 6);
+
+ // More sophisticated: non-last block ends on some newline inside a quoted string
Review Comment:
o_O
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478968863
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
Nit: Could we at least add a comment indicating that these tests exercise a condition which would be invalid for BlockParser?
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478507640
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
I don't think the BlockParser actually needs to return an error (*maybe* a debug assertion). I'm mostly concerned with how we test your solution. By analogy: if we had a calculator object, it'd be better to test whatever uses that object rather than unit testing `calculator.divide(1, 0)`. BlockParser might be refactored later in a way which doesn't change reader behavior as long as chunking is valid but *does* invalidate these unit tests, and I'd like to avoid placing such a constraint on future developers.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39892:
URL: https://github.com/apache/arrow/pull/39892#issuecomment-1931151315
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a6e577d031d20a1a7d3dd01536b9a77db5d1bff8.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/21300497186) has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478445540
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
That's a good point, let me try that.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478490847
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
Ok, so the problem is that it makes harder to return a meaningful error message. The CSV parser itself has no idea of who did the chunking and how, so it can't start mentioning `newlines_in_values`. It would have to return a more generic `Status::Invalid`, then the CSV would have to detect that particular `Status::Invalid` (how?) and rewrite the error message.
This is a lot of complication for a bit of unnecessary purity in the `BlockParser` API (which isn't terribly pretty to begin with), so I would rather keep the current solution in the PR.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478507640
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
I don't think the BlockParser actually needs to return an error (*maybe* a debug assertion). I'm mostly concerned with how we test your solution. By analogy: if we had a calculator object, it'd be better to test whatever uses that object rather than unit testing `calculator.divide(1, 0)`. BlockParser might be refactored later in a way which doesn't change reader behavior as long as chunking is valid but *does* invalidate these unit tests, and I'd like to avoid placing such a constraint on future developers. To that end: let's test the reader instead of the parser
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39892:
URL: https://github.com/apache/arrow/pull/39892#issuecomment-1921911781
:warning: GitHub issue #39857 **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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1479849698
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
Yes, I'll add it.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #39892:
URL: https://github.com/apache/arrow/pull/39892
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478490847
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
Ok, so the problem is that it makes harder to return a meaningful error message. The CSV parser itself has no idea of who did the chunking and how, so it can't start mentioning `newlines_in_values`. It would have to return a more generic `Status::Invalid`, then the CSV would have to detect that particular `Status::Invalid` (how?) and rewrite the error message to make it informative for the user.
This is a lot of complication for a bit of unnecessary purity in the `BlockParser` API (which isn't terribly pretty to begin with), so I would rather keep the current solution in the PR.
--
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-39857: [C++] Improve error message for "chunker out of sync" condition [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39892:
URL: https://github.com/apache/arrow/pull/39892#discussion_r1478510064
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -376,6 +383,19 @@ TEST(BlockParser, TruncatedData) {
}
}
+TEST(BlockParser, TruncatedDataViews) {
+ // If non-last block is truncated, parsing stops
+ BlockParser parser(ParseOptions::Defaults(), /*num_cols=*/3);
+ AssertParsePartial(parser, Views({"a,b,", "c\n"}), 0);
+ // (XXX should we guarantee this one below?)
Review Comment:
Yes, adding a test for the CSV reader would certainly be good. I'll try to look for a reliable way to reproduce it there (without involving the original data file :-)).
--
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