Skip to content

ARROW-7062: [C++][Dataset] Ensure ParquetFileFormat::Open catch parqu… #5789

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

Closed
fsaintjacques wants to merge 1 commit into from
Closed

ARROW-7062: [C++][Dataset] Ensure ParquetFileFormat::Open catch parqu… #5789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/src/arrow/dataset/file_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class ARROW_DS_EXPORT FileSource {
Compression::type compression = Compression::UNCOMPRESSED)
: FileSource(FileSource::BUFFER, compression) {
buffer_ = std::move(buffer);
path_ = "<Buffer>";
}

bool operator==(const FileSource& other) const {
Expand Down
7 changes: 6 additions & 1 deletion cpp/src/arrow/dataset/file_parquet.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,12 @@ Status ParquetFileFormat::OpenReader(
std::shared_ptr<io::RandomAccessFile> input;
RETURN_NOT_OK(source.Open(&input));

*out = parquet::ParquetFileReader::Open(input);
try {
*out = parquet::ParquetFileReader::Open(input);
} catch (const ::parquet::ParquetException& e) {
return ::arrow::Status::IOError("Could not open parquet input source '",
source.path(), "': ", e.what());
}
return Status::OK();
}

Expand Down
16 changes: 16 additions & 0 deletions cpp/src/arrow/dataset/file_parquet_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,22 @@ TEST_F(TestParquetFileFormat, ScanRecordBatchReader) {
ASSERT_EQ(row_count, kNumRows);
}

TEST_F(TestParquetFileFormat, OpenFailureWithRelevantError) {
auto format = ParquetFileFormat();

std::shared_ptr<Schema> dont_care;

std::shared_ptr<Buffer> buf = std::make_shared<Buffer>(util::string_view(""));
EXPECT_RAISES_WITH_MESSAGE_THAT(IOError, testing::HasSubstr("<Buffer>"),
format.Inspect({buf}, &dont_care));

constexpr auto file_name = "herp/derp";
ASSERT_OK_AND_ASSIGN(
auto fs, fs::internal::MockFileSystem::Make(fs::kNoTime, {fs::File(file_name)}));
EXPECT_RAISES_WITH_MESSAGE_THAT(IOError, testing::HasSubstr(file_name),
format.Inspect({file_name, fs.get()}, &dont_care));
}

TEST_F(TestParquetFileFormat, Inspect) {
auto reader = GetRecordBatchReader();
auto source = GetFileSource(reader.get());
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/arrow/dataset/test_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,15 +294,15 @@ Status JSONRecordBatchFileFormat::MakeFragment(const FileSource& source,
class TestFileSystemBasedDataSource : public ::testing::Test {
public:
void MakeFileSystem(const std::vector<fs::FileStats>& stats) {
ASSERT_OK(fs::internal::MockFileSystem::Make(fs::kNoTime, stats, &fs_));
ASSERT_OK_AND_ASSIGN(fs_, fs::internal::MockFileSystem::Make(fs::kNoTime, stats));
}

void MakeFileSystem(const std::vector<std::string>& paths) {
std::vector<fs::FileStats> stats{paths.size()};
std::transform(paths.cbegin(), paths.cend(), stats.begin(),
[](const std::string& p) { return fs::File(p); });

ASSERT_OK(fs::internal::MockFileSystem::Make(fs::kNoTime, stats, &fs_));
ASSERT_OK_AND_ASSIGN(fs_, fs::internal::MockFileSystem::Make(fs::kNoTime, stats));
}

void MakeSource(const std::vector<fs::FileStats>& stats,
Expand Down
23 changes: 13 additions & 10 deletions cpp/src/arrow/filesystem/filesystem_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,21 +200,27 @@ class TestMockFS : public ::testing::Test {
return stream->Write(s.data(), static_cast<int64_t>(s.length()));
}

void CheckDirs(const std::vector<DirInfo>& expected) {
ASSERT_EQ(fs_->AllDirs(), expected);
std::vector<DirInfo> AllDirs() {
return arrow::internal::checked_pointer_cast<MockFileSystem>(fs_)->AllDirs();
}

std::vector<FileInfo> AllFiles() {
return arrow::internal::checked_pointer_cast<MockFileSystem>(fs_)->AllFiles();
}

void CheckDirs(const std::vector<DirInfo>& expected) { ASSERT_EQ(AllDirs(), expected); }

void CheckDirPaths(const std::vector<std::string>& expected) {
std::vector<DirInfo> infos;
infos.reserve(expected.size());
for (const auto& s : expected) {
infos.push_back({s, time_});
}
ASSERT_EQ(fs_->AllDirs(), infos);
ASSERT_EQ(AllDirs(), infos);
}

void CheckFiles(const std::vector<FileInfo>& expected) {
ASSERT_EQ(fs_->AllFiles(), expected);
ASSERT_EQ(AllFiles(), expected);
}

void CreateFile(const std::string& path, const std::string& data) {
Expand All @@ -223,7 +229,7 @@ class TestMockFS : public ::testing::Test {

protected:
TimePoint time_;
std::shared_ptr<MockFileSystem> fs_;
std::shared_ptr<FileSystem> fs_;
};

TEST_F(TestMockFS, Empty) {
Expand Down Expand Up @@ -366,14 +372,11 @@ TEST_F(TestMockFS, OpenAppendStream) {
}

TEST_F(TestMockFS, Make) {
std::shared_ptr<FileSystem> fs;
ASSERT_OK(MockFileSystem::Make(time_, {}, &fs));
fs_ = std::static_pointer_cast<MockFileSystem>(fs);
ASSERT_OK_AND_ASSIGN(fs_, MockFileSystem::Make(time_, {}));
CheckDirs({});
CheckFiles({});

ASSERT_OK(MockFileSystem::Make(time_, {Dir("A/B/C"), File("A/a")}, &fs));
fs_ = std::static_pointer_cast<MockFileSystem>(fs);
ASSERT_OK_AND_ASSIGN(fs_, MockFileSystem::Make(time_, {Dir("A/B/C"), File("A/a")}));
CheckDirs({{"A", time_}, {"A/B", time_}, {"A/B/C", time_}});
CheckFiles({{"A/a", time_, ""}});
}
Expand Down
8 changes: 3 additions & 5 deletions cpp/src/arrow/filesystem/mockfs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,8 @@ Status MockFileSystem::CreateFile(const std::string& path, const std::string& co
return file->Close();
}

Status MockFileSystem::Make(TimePoint current_time, const std::vector<FileStats>& stats,
std::shared_ptr<FileSystem>* out) {
Result<std::shared_ptr<FileSystem>> MockFileSystem::Make(
TimePoint current_time, const std::vector<FileStats>& stats) {
auto fs = std::make_shared<MockFileSystem>(current_time);
for (const auto& s : stats) {
switch (s.type()) {
Expand All @@ -685,9 +685,7 @@ Status MockFileSystem::Make(TimePoint current_time, const std::vector<FileStats>
}
}

*out = fs;

return Status::OK();
return fs;
Copy link
Copy Markdown
Member

@bkietz bkietz Nov 8, 2019

Choose a reason for hiding this comment

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

This will need a move on some platforms to avoid a spurious copy (and clang will sometimes emit a warning about it)

Suggested change
return fs;
return std::move(fs);

Copy link
Copy Markdown
Contributor Author

@fsaintjacques fsaintjacques Nov 8, 2019

Choose a reason for hiding this comment

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

Every time I do this, I get a warning that it may disable RVO. Anyway, this is a mock class.

}

} // namespace internal
Expand Down
5 changes: 3 additions & 2 deletions cpp/src/arrow/filesystem/mockfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <vector>

#include "arrow/filesystem/filesystem.h"
#include "arrow/result.h"

namespace arrow {
namespace fs {
Expand Down Expand Up @@ -100,8 +101,8 @@ class ARROW_EXPORT MockFileSystem : public FileSystem {

// Create a MockFileSystem out of (empty) FileStats. The content of every
// file is empty and of size 0. All directories will be created recursively.
static Status Make(TimePoint current_time, const std::vector<FileStats>& stats,
std::shared_ptr<FileSystem>* out);
static Result<std::shared_ptr<FileSystem>> Make(TimePoint current_time,
const std::vector<FileStats>& stats);

class Impl;

Expand Down
12 changes: 12 additions & 0 deletions cpp/src/arrow/testing/gtest_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ inline Status GenericToStatus(const Result<T>& res) {
ASSERT_EQ((message), _st.ToString()); \
} while (false)

#define EXPECT_RAISES_WITH_MESSAGE_THAT(ENUM, matcher, expr) \
Copy link
Copy Markdown
Member

@bkietz bkietz Nov 8, 2019

Choose a reason for hiding this comment

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

:+1"

do { \
auto _res = (expr); \
::arrow::Status _st = ::arrow::internal::GenericToStatus(_res); \
if (!_st.Is##ENUM()) { \
FAIL() << "Expected '" ARROW_STRINGIFY(expr) "' to fail with " ARROW_STRINGIFY( \
ENUM) ", but got " \
<< _st.ToString(); \
} \
EXPECT_THAT(_st.ToString(), (matcher)); \
} while (false)

#define ASSERT_OK(expr) \
do { \
auto _res = (expr); \
Expand Down