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/06 15:24:22 UTC
[PR] GH-39962: [C++] Small CSV reader refactoring [arrow]
pitrou opened a new pull request, #39963:
URL: https://github.com/apache/arrow/pull/39963
### What changes are included in this PR?
Factor our some shared functionality in the CSV reader to avoid code duplication.
### Are these changes tested?
Yes, by existing tests.
### 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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1930042768
:warning: GitHub issue #39962 **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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39963:
URL: https://github.com/apache/arrow/pull/39963#discussion_r1482675456
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -719,66 +726,25 @@ class ReaderMixin {
return Status::OK();
}
- struct ParseResult {
- std::shared_ptr<BlockParser> parser;
- int64_t parsed_bytes;
- };
-
- Result<ParseResult> Parse(const std::shared_ptr<Buffer>& partial,
- const std::shared_ptr<Buffer>& completion,
- const std::shared_ptr<Buffer>& block, int64_t block_index,
- bool is_final) {
- static constexpr int32_t max_num_rows = std::numeric_limits<int32_t>::max();
- auto parser = std::make_shared<BlockParser>(
- io_context_.pool(), parse_options_, num_csv_cols_, num_rows_seen_, max_num_rows);
-
- std::shared_ptr<Buffer> straddling;
- std::vector<std::string_view> views;
- if (partial->size() != 0 || completion->size() != 0) {
- if (partial->size() == 0) {
- straddling = completion;
- } else if (completion->size() == 0) {
- straddling = partial;
- } else {
- ARROW_ASSIGN_OR_RAISE(
- straddling, ConcatenateBuffers({partial, completion}, io_context_.pool()));
- }
- views = {std::string_view(*straddling), std::string_view(*block)};
- } else {
- views = {std::string_view(*block)};
- }
- uint32_t parsed_size;
- if (is_final) {
- RETURN_NOT_OK(parser->ParseFinal(views, &parsed_size));
- } else {
- RETURN_NOT_OK(parser->Parse(views, &parsed_size));
- }
- // See BlockParsingOperator for explanation.
- const int64_t bytes_before_buffer = partial->size() + completion->size();
- if (static_cast<int64_t>(parsed_size) < bytes_before_buffer) {
- return Status::Invalid(
- "CSV parser got out of sync with chunker. This can mean the data file "
- "contains cell values spanning multiple lines; please consider enabling "
- "the option 'newlines_in_values'.");
- }
+ Result<ParsedBlock> Parse(const CSVBlock& block) {
+ DCHECK(parsing_operator_.has_value());
Review Comment:
IIUC, `consume_bytes` is defined for serial reads where we can give the CSV parser a block that's truncated at the end, and keep the unprocessed data for the next parser call. For threaded reads, we cannot do that so we use a more elaborate chunking step instead.
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1932252997
Revision: 7284df9dd303503399d1500c18c60b4237011fe0
Submitted crossbow builds: [ursacomputing/crossbow @ actions-34dc118548](https://github.com/ursacomputing/crossbow/branches/all?query=actions-34dc118548)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914718/job/21323583901)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914564/job/21323583554)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914338/job/21323582348)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/21323586705)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914402/job/21323582595)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914890/job/21323584688)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914787/job/21323584453)|
|test-fedora-38-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914652/job/21323583747)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914340/job/21323582451)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914321/job/21323582445)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914814/job/21323584702)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914391/job/21323582788)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914769/job/21323583932)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914723/job/21323583756)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/7816914884/job/21323584434)|
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1930050363
Revision: f8b9e841dfb5f9fb1663c5dfd9b723f00bdf2491
Submitted crossbow builds: [ursacomputing/crossbow @ actions-44f37e41c4](https://github.com/ursacomputing/crossbow/branches/all?query=actions-44f37e41c4)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066948/job/21278636377)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066789/job/21278635446)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066947/job/21278636729)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/21278638724)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802067121/job/21278637401)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/7802067081/job/21278636675)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066966/job/21278636983)|
|test-fedora-38-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066785/job/21278635462)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066961/job/21278636646)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/7802067182/job/21278637536)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066847/job/21278635603)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066840/job/21278635573)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066890/job/21278636212)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/7802067295/job/21278638172)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/7802066791/job/21278636344)|
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1934560059
@github-actions crossbow submit -g cpp
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #39963:
URL: https://github.com/apache/arrow/pull/39963
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39963:
URL: https://github.com/apache/arrow/pull/39963#discussion_r1481900836
##########
cpp/src/arrow/csv/reader.cc:
##########
@@ -719,66 +726,25 @@ class ReaderMixin {
return Status::OK();
}
- struct ParseResult {
- std::shared_ptr<BlockParser> parser;
- int64_t parsed_bytes;
- };
-
- Result<ParseResult> Parse(const std::shared_ptr<Buffer>& partial,
- const std::shared_ptr<Buffer>& completion,
- const std::shared_ptr<Buffer>& block, int64_t block_index,
- bool is_final) {
- static constexpr int32_t max_num_rows = std::numeric_limits<int32_t>::max();
- auto parser = std::make_shared<BlockParser>(
- io_context_.pool(), parse_options_, num_csv_cols_, num_rows_seen_, max_num_rows);
-
- std::shared_ptr<Buffer> straddling;
- std::vector<std::string_view> views;
- if (partial->size() != 0 || completion->size() != 0) {
- if (partial->size() == 0) {
- straddling = completion;
- } else if (completion->size() == 0) {
- straddling = partial;
- } else {
- ARROW_ASSIGN_OR_RAISE(
- straddling, ConcatenateBuffers({partial, completion}, io_context_.pool()));
- }
- views = {std::string_view(*straddling), std::string_view(*block)};
- } else {
- views = {std::string_view(*block)};
- }
- uint32_t parsed_size;
- if (is_final) {
- RETURN_NOT_OK(parser->ParseFinal(views, &parsed_size));
- } else {
- RETURN_NOT_OK(parser->Parse(views, &parsed_size));
- }
- // See BlockParsingOperator for explanation.
- const int64_t bytes_before_buffer = partial->size() + completion->size();
- if (static_cast<int64_t>(parsed_size) < bytes_before_buffer) {
- return Status::Invalid(
- "CSV parser got out of sync with chunker. This can mean the data file "
- "contains cell values spanning multiple lines; please consider enabling "
- "the option 'newlines_in_values'.");
- }
+ Result<ParsedBlock> Parse(const CSVBlock& block) {
+ DCHECK(parsing_operator_.has_value());
Review Comment:
It might be desirable to hoist the assertion from ReadAsync()
```suggestion
DCHECK(parsing_operator_.has_value());
DCHECK(!block.consume_bytes);
```
and maybe also to add a comment clarifying when consume_bytes will be null (IIUC, when dealing with a final block where we don't need to track consumption anymore?)
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1934564140
Revision: 2804b2a0c4c718887bfe243235d0b476c00c3fbb
Submitted crossbow builds: [ursacomputing/crossbow @ actions-79e6f2b964](https://github.com/ursacomputing/crossbow/branches/all?query=actions-79e6f2b964)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245441/job/21373812467)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245230/job/21373811169)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245712/job/21373813108)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/21373814113)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245498/job/21373811900)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245424/job/21373811930)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245704/job/21373813112)|
|test-fedora-38-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245139/job/21373811163)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245572/job/21373812257)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245206/job/21373811156)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245625/job/21373812758)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245980/job/21373813518)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245556/job/21373812245)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245776/job/21373813107)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/7833245195/job/21373811154)|
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1932247914
@github-actions crossbow submit -g cpp
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1930045215
@github-actions crossbow submit -g cpp
--
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-39962: [C++] Small CSV reader refactoring [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39963:
URL: https://github.com/apache/arrow/pull/39963#issuecomment-1935146932
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit de3cdc00c21fd3e9d8d763099591f23720ca8d1f.
There was 1 benchmark result indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2024-02-08 22:08:10Z](https://conbench.ursa.dev/compare/runs/9029e9c932544a56bccfce401782cae7...39c3337ce83240f4980f4634d00dd6c7/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-17, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/065c548ed71b713b8000212968eaafc9...065c569947c27a388000d46b5718ef84)
The [full Conbench report](https://github.com/apache/arrow/runs/21388445485) 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