ARROW-8218: [C++] Decompress record batch messages in parallel at field level. Only allow LZ4_FRAME, ZSTD compression #6777
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pitrou
reviewed
Mar 31, 2020
pitrou
reviewed
Mar 31, 2020
|
I addressed the comments and also parallelized the compression path. Would someone take another look at these new changes? |
wesm
commented
Mar 31, 2020
Comment on lines
+203
to
+210
| if (options_.use_threads) { | ||
| return ::arrow::internal::ParallelFor(static_cast<int>(out_->body_buffers.size()), | ||
| CompressOne); | ||
| } else { | ||
| for (size_t i = 0; i < out_->body_buffers.size(); ++i) { | ||
| RETURN_NOT_OK(CompressOne(i)); | ||
| } | ||
| return Status::OK(); |
There was a problem hiding this comment.
This "optional parallelism" pattern occurs frequently, I'll open a JIRA about factoring it out into a helper function.
bkietz
approved these changes
Mar 31, 2020
cpp/src/arrow/ipc/reader.cc
Outdated
Comment on lines
345
to
346
| Status DecompressBuffers(const std::vector<std::shared_ptr<ArrayData>>& fields, | ||
| Compression::type compression, const IpcReadOptions& options) { |
There was a problem hiding this comment.
since you're mutating the contents of fields, should this be
Suggested change
| Status DecompressBuffers(const std::vector<std::shared_ptr<ArrayData>>& fields, | |
| Compression::type compression, const IpcReadOptions& options) { | |
| Status DecompressBuffers(Compression::type compression, const IpcReadOptions& options, | |
| std::vector<std::shared_ptr<ArrayData>>* fields) { |
?
There was a problem hiding this comment.
Sort of a weird case not often seen, since the vector itself is not mutated. When I see std::vector<T>* in a function signature that suggests that the vector is modified. Thoughts?
There was a problem hiding this comment.
It's true; the vector itself isn't mutated and const correctness isn't broken here. I was only thinking of trying to communicate mutation to a future reader. Unfortunately all we have is the ampersand and comments, neither of which is ideal here (... unless we start tossing std::vector<const T> around everywhere we want to be clear that the elements are immutable)
|
Flaky Thrift download again |
|
Googletest download flaked in Appveyor: It passed on my fork though. +1. Merging this |
wesm
pushed a commit
that referenced
this pull request
May 9, 2020
…eather V2 This PR always puts the compressed size in little-endian format for Feather V2 since the reader expected the little-endian format. Based on [the discussion](#6777 (comment)) at #6777, [this commit](aa28280) reads compressed_length in Feather V2 format as little-endian. However, the writer [puts compressed_length in native-endian](https://github.com/apache/arrow/blob/master/cpp/src/arrow/ipc/writer.cc#L177). This PR can fix failures related to reading compressed feather format in `arrow-ipc-read-write-test` and `arrow-feather-test`. Closes #7137 from kiszk/ARROW-8747 Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>
kou
pushed a commit
that referenced
this pull request
May 15, 2020
This PR writes and reads Plasma header (version, type, and length) in the big-endian format. It allows us to make it easy to interpret a header of Plasma data among different endian machines. The current issue is to write Plasma header in native endian at [here](https://github.com/apache/arrow/blob/master/cpp/src/plasma/io.cc#L65-L71). It is not possible to know version, type, and length of a given Plasma file among different platforms. Feather V2 also uses little-endian for the header based on [the discussion](#6777 (comment)). This PR uses little-endian by following this discussion. Closes #7146 from kiszk/ARROW-8757 Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I also changed the metadata key to "ARROW:experimental_compression", if anyone has opinions.
Haven't run benchmarks but will do so tomorrow.