Skip to content

ARROW-1019: [C++] Implement compressed streams #2777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
pitrou wants to merge 7 commits into from
Closed

ARROW-1019: [C++] Implement compressed streams #2777

pitrou wants to merge 7 commits into from

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Oct 16, 2018 •
edited
Loading

Implement CompressedInputStream and CompressedOutputStream C++ classes. Tested with gzip, brotli and zstd codecs.

I initially intended to expose the functionality in Python, but NativeFile expects a RandomAccessFile in read mode (rather than a mere InputStream).

@pitrou pitrou mentioned this pull request Oct 18, 2018
Closed
@pitrou pitrou force-pushed the ARROW-1019-compressed-streams branch from 09e6e3c to 0efd75f Compare October 18, 2018 14:10
@wesm
Copy link
Member

wesm commented Oct 18, 2018

Will review this today

wesm
wesm reviewed Oct 18, 2018
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, but this looks great!

cpp/src/arrow/io/compressed.cc
#include <memory>
#include <mutex>
#include <string>
#include <utility>
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include ordering https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

cpp/src/arrow/io/compressed.cc Outdated

class CompressedOutputStream::Impl {
public:
Impl(MemoryPool* pool, Codec* codec, std::shared_ptr<OutputStream> raw)
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do const std::shared_ptr<T>& to possibly avoid extra copy

cpp/src/arrow/io/compressed.cc
int64_t bytes_read, bytes_written;
int64_t input_len = nbytes;
int64_t output_len = compressed_->size() - compressed_pos_;
uint8_t* output = compressed_->mutable_data() + compressed_pos_;
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there might be a use for an intermediate abstraction "CompressionBuffer" that encapsulates some of this book-keeping that shows up in many places (unit tests and implementations). This could be passed into the stream compressor functions instead of raw pointers, allowing the compressor to request that the buffer be enlarged, etc.

Copy link
Member Author

@pitrou pitrou Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how it would look like, though.

cpp/src/arrow/io/compressed.cc
if (is_open_) {
is_open_ = false;
RETURN_NOT_OK(FinalizeCompression());
return raw_->Close();
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we definitely want to close the passed output stream? I'm trying to think if there are scenarios where we would not want to

Copy link
Member Author

@pitrou pitrou Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by default, yes. We could add a constructor argument if we need to keep the underlying file alive in some cases.

cpp/src/arrow/io/compressed.cc
bool need_more_output;
int64_t bytes_read, bytes_written;
int64_t input_len = compressed_->size() - compressed_pos_;
const uint8_t* input = compressed_->data() + compressed_pos_;
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here re: output buffer bookkeeping

cpp/src/arrow/io/compressed.cc Outdated

private:
// Write 64 KB compressed data at a time
static const int64_t CHUNK_SIZE = 64 * 1024;
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kChunkSize

cpp/src/arrow/io/compressed.h
std::shared_ptr<CompressedOutputStream>* out);
static Status Make(MemoryPool* pool, util::Codec* codec,
std::shared_ptr<OutputStream> raw,
std::shared_ptr<CompressedOutputStream>* out);
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use const std::shared_ptr<T>& here and elsewhere in the public APIs for consistency

cpp/src/arrow/io/compressed.h
/// \brief Create a compressed output stream wrapping the given output stream.
static Status Make(util::Codec* codec, std::shared_ptr<OutputStream> raw,
std::shared_ptr<CompressedOutputStream>* out);
static Status Make(MemoryPool* pool, util::Codec* codec,
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can codec be const? Also, can it by const Codec&?

Copy link
Member Author

@pitrou pitrou Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, Codec::MakeCompressor and Codec::MakeDecompressor are non-const methods. IMHO it doesn't mean much to have a const Codec.

Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more of an argument passing consistency. We generally use const T& for immutable arguments, T* for mutable arguments, and const T* in the rarer case when the input may be null

cpp/src/arrow/io/compressed.h
std::shared_ptr<CompressedInputStream>* out);
static Status Make(MemoryPool* pool, util::Codec* codec,
std::shared_ptr<InputStream> raw,
std::shared_ptr<CompressedInputStream>* out);
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

cpp/src/arrow/io/io-compressed-test.cc
ASSERT_RAISES(IOError, stream->Read(1024, &out_buf));
}

// NOTE: Snappy doesn't support streaming decompression
Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could (should?) define a framed format of our own devising

Copy link
Member Author

@pitrou pitrou Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is it useful? The main point here is to interact with existing files.

Copy link
Member

@wesm wesm Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I will say YAGNI for now ;)

@pitrou pitrou force-pushed the ARROW-1019-compressed-streams branch from a0b333b to 64461f3 Compare October 18, 2018 17:41
@wesm
Copy link
Member

wesm commented Oct 18, 2018

+1. Let me have a look at the appveyor build

@wesm
Copy link
Member

wesm commented Oct 18, 2018

I'll wait a little while to make sure appveyor looks good then merge

@wesm wesm closed this in eab7d5f Oct 18, 2018
@pitrou pitrou deleted the ARROW-1019-compressed-streams branch October 18, 2018 19:53
wesm pushed a commit that referenced this pull request Oct 20, 2018
@pitrou @wesm
ARROW-3380: [Python] Support reading gzipped CSV files
2d7fbb2 
Also works for other bundled compression types.

Based on PR #2777.

Author: Antoine Pitrou <antoine@python.org>

Closes #2786 from pitrou/ARROW-3380-gzipped-csv-read and squashes the following commits:

9a2244f <Antoine Pitrou> ARROW-3380:  Support reading gzipped CSV files
@asfimport asfimport mentioned this pull request Oct 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesm wesm wesm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pitrou @wesm