Skip to content

Commit

Permalink
PARQUET-537: Ensure that LocalFileSource is properly closed.
Browse files Browse the repository at this point in the history
Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes apache#68 from asandryh/PARQUET-537 and squashes the following commits:

18dca87 [Aliaksei Sandryhaila] Added a unit test.
dfb7a0b [Aliaksei Sandryhaila] PARQUET-537: Ensure that LocalFileSource is properly closed.

Change-Id: I9f2544a51e350464983f7ca511970b434d009f3a
  • Loading branch information
Aliaksei Sandryhaila authored and julienledem committed Mar 2, 2016
1 parent 490d236 commit 0ca90ed
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 3 deletions.
4 changes: 4 additions & 0 deletions cpp/src/parquet/file/reader-internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ void SerializedFile::Close() {
source_->Close();
}

SerializedFile::~SerializedFile() {
Close();
}

std::shared_ptr<RowGroupReader> SerializedFile::GetRowGroup(int i) {
std::unique_ptr<SerializedRowGroup> contents(new SerializedRowGroup(source_.get(),
&metadata_.row_groups[i]));
Expand Down
1 change: 1 addition & 0 deletions cpp/src/parquet/file/reader-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class SerializedFile : public ParquetFileReader::Contents {
virtual int64_t num_rows() const;
virtual int num_columns() const;
virtual int num_row_groups() const;
virtual ~SerializedFile();

private:
// This class takes ownership of the provided data source
Expand Down
8 changes: 6 additions & 2 deletions cpp/src/parquet/file/reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ RowGroupStatistics RowGroupReader::GetColumnStats(int i) const {
// ParquetFileReader public API

ParquetFileReader::ParquetFileReader() : schema_(nullptr) {}
ParquetFileReader::~ParquetFileReader() {}
ParquetFileReader::~ParquetFileReader() {
Close();
}

std::unique_ptr<ParquetFileReader> ParquetFileReader::OpenFile(const std::string& path,
bool memory_map) {
Expand All @@ -91,7 +93,9 @@ void ParquetFileReader::Open(std::unique_ptr<ParquetFileReader::Contents> conten
}

void ParquetFileReader::Close() {
contents_->Close();
if (contents_) {
contents_->Close();
}
}

int ParquetFileReader::num_row_groups() const {
Expand Down
35 changes: 34 additions & 1 deletion cpp/src/parquet/reader-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@
// under the License.

#include <gtest/gtest.h>
#include <fcntl.h>
#include <cstdlib>
#include <cstdint>
#include <iostream>
#include <memory>
#include <string>

#include "parquet/file/reader.h"
#include "parquet/file/reader-internal.h"
#include "parquet/column/reader.h"
#include "parquet/column/scanner.h"
#include "parquet/util/input.h"

using std::string;

Expand All @@ -50,7 +53,6 @@ class TestAllTypesPlain : public ::testing::Test {
std::unique_ptr<ParquetFileReader> reader_;
};


TEST_F(TestAllTypesPlain, NoopConstructDestruct) {
}

Expand Down Expand Up @@ -121,4 +123,35 @@ TEST_F(TestAllTypesPlain, DebugPrintWorks) {
ASSERT_GT(result.size(), 0);
}


class TestLocalFileSource : public ::testing::Test {
public:
void SetUp() {
std::string dir_string(data_dir);

std::stringstream ss;
ss << dir_string << "/" << "alltypes_plain.parquet";

file.reset(new LocalFileSource());
file->Open(ss.str());
}

void TearDown() {}

protected:
std::unique_ptr<LocalFileSource> file;
};

TEST_F(TestLocalFileSource, FileClosedOnDestruction) {
int file_desc = file->file_descriptor();
{
auto contents = SerializedFile::Open(std::move(file));
std::unique_ptr<ParquetFileReader> result(new ParquetFileReader());
result->Open(std::move(contents));
}
ASSERT_EQ(-1, fcntl(file_desc, F_GETFD));
ASSERT_EQ(EBADF, errno);
}


} // namespace parquet_cpp

0 comments on commit 0ca90ed

Please sign in to comment.