You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/21 14:49:07 UTC
[GitHub] [arrow] pitrou opened a new pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
pitrou opened a new pull request #10566:
URL: https://github.com/apache/arrow/pull/10566
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#discussion_r656064551
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -307,14 +321,17 @@ Result<std::unique_ptr<parquet::arrow::FileReader>> ParquetFileFormat::GetReader
auto properties = MakeReaderProperties(*this, parquet_scan_options.get(), pool);
ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- std::unique_ptr<parquet::ParquetFileReader> reader;
- try {
- reader = parquet::ParquetFileReader::Open(std::move(input), std::move(properties));
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
+ auto maybe_reader = [&]() -> Result<std::unique_ptr<parquet::ParquetFileReader>> {
Review comment:
Hmm, I'm a bit lukewarm. Other similar functionality is already managed as macros (`ARROW_RETURN_NOT_OK`, `RETURN_IF_PYERROR`...). It seems like it would introduce a gratuitous variation. Also, given the size of the C++ metaprogramming alternative, this is possibly a reasonable use case for a macro.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#issuecomment-865120177
https://issues.apache.org/jira/browse/ARROW-13135
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#discussion_r655503062
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -270,24 +279,29 @@ ParquetFileFormat::ParquetFileFormat(const parquet::ReaderProperties& reader_pro
}
Result<bool> ParquetFileFormat::IsSupported(const FileSource& source) const {
- try {
- ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- ARROW_ASSIGN_OR_RAISE(auto parquet_scan_options,
- GetFragmentScanOptions<ParquetFragmentScanOptions>(
- kParquetTypeName, nullptr, default_fragment_scan_options));
- auto reader = parquet::ParquetFileReader::Open(
- std::move(input), MakeReaderProperties(*this, parquet_scan_options.get()));
- std::shared_ptr<parquet::FileMetaData> metadata = reader->metadata();
- return metadata != nullptr && metadata->can_decompress();
- } catch (const ::parquet::ParquetInvalidOrCorruptedFileException& e) {
- ARROW_UNUSED(e);
- return false;
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
-
- return true;
+ auto maybe_is_supported = [&]() -> Result<bool> {
Review comment:
Please move this to `IsSupportedImpl` or so, then
```suggestion
auto maybe_is_supported = IsSupportedImpl(*this, source);
```
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -307,14 +321,17 @@ Result<std::unique_ptr<parquet::arrow::FileReader>> ParquetFileFormat::GetReader
auto properties = MakeReaderProperties(*this, parquet_scan_options.get(), pool);
ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- std::unique_ptr<parquet::ParquetFileReader> reader;
- try {
- reader = parquet::ParquetFileReader::Open(std::move(input), std::move(properties));
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
+ auto maybe_reader = [&]() -> Result<std::unique_ptr<parquet::ParquetFileReader>> {
Review comment:
Less fragile than macros would be:
```c++
auto maybe_reader = parquet::CatchNotOk([&] {
return parquet::ParquetFileReader::Open(std::move(input), std::move(properties));
});
```
Could be implemented like:
```c++
template <typename Fn, typename Ret>
struct CatchNotOkImpl {
static typename EnsureResult<Ret>::type Call(Fn&& fn) { return std::forward<Fn>(fn)(); }
};
template <typename Fn>
struct CatchNotOkImpl<Fn, Status> {
static Status Call(Fn&& fn) { return std::forward<Fn>(fn)(); }
};
template <typename Fn>
struct CatchNotOkImpl<Fn, void> {
static Status Call(Fn&& fn) {
std::forward<Fn>(fn)();
return Status::OK();
}
};
template <typename Fn, typename Impl = CatchNotOkImpl<Fn, decltype(std::declval<Fn>()())>>
auto CatchNotOk(Fn&& fn) -> decltype(Impl::Call(std::forward<Fn>(fn))) try {
return Impl::Call(std::forward<Fn>(fn));
} catch (const ParquetStatusException& e) {
return e.status();
} catch (const ParquetException& e) {
return Status::IOError(e.what());
}
```
What do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#issuecomment-865120177
https://issues.apache.org/jira/browse/ARROW-13135
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #10566:
URL: https://github.com/apache/arrow/pull/10566
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#discussion_r656064664
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -270,24 +279,29 @@ ParquetFileFormat::ParquetFileFormat(const parquet::ReaderProperties& reader_pro
}
Result<bool> ParquetFileFormat::IsSupported(const FileSource& source) const {
- try {
- ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- ARROW_ASSIGN_OR_RAISE(auto parquet_scan_options,
- GetFragmentScanOptions<ParquetFragmentScanOptions>(
- kParquetTypeName, nullptr, default_fragment_scan_options));
- auto reader = parquet::ParquetFileReader::Open(
- std::move(input), MakeReaderProperties(*this, parquet_scan_options.get()));
- std::shared_ptr<parquet::FileMetaData> metadata = reader->metadata();
- return metadata != nullptr && metadata->can_decompress();
- } catch (const ::parquet::ParquetInvalidOrCorruptedFileException& e) {
- ARROW_UNUSED(e);
- return false;
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
-
- return true;
+ auto maybe_is_supported = [&]() -> Result<bool> {
Review comment:
Will do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#issuecomment-866220429
CI failures are unrelated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#issuecomment-866061013
Rebased to fix conflicts, will merge if CI green.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10566: ARROW-13135: [C++] Fix Status propagation from Parquet exception
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10566:
URL: https://github.com/apache/arrow/pull/10566#discussion_r655503062
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -270,24 +279,29 @@ ParquetFileFormat::ParquetFileFormat(const parquet::ReaderProperties& reader_pro
}
Result<bool> ParquetFileFormat::IsSupported(const FileSource& source) const {
- try {
- ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- ARROW_ASSIGN_OR_RAISE(auto parquet_scan_options,
- GetFragmentScanOptions<ParquetFragmentScanOptions>(
- kParquetTypeName, nullptr, default_fragment_scan_options));
- auto reader = parquet::ParquetFileReader::Open(
- std::move(input), MakeReaderProperties(*this, parquet_scan_options.get()));
- std::shared_ptr<parquet::FileMetaData> metadata = reader->metadata();
- return metadata != nullptr && metadata->can_decompress();
- } catch (const ::parquet::ParquetInvalidOrCorruptedFileException& e) {
- ARROW_UNUSED(e);
- return false;
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
-
- return true;
+ auto maybe_is_supported = [&]() -> Result<bool> {
Review comment:
Please move this to `IsSupportedImpl` or so, then
```suggestion
auto maybe_is_supported = IsSupportedImpl(*this, source);
```
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -307,14 +321,17 @@ Result<std::unique_ptr<parquet::arrow::FileReader>> ParquetFileFormat::GetReader
auto properties = MakeReaderProperties(*this, parquet_scan_options.get(), pool);
ARROW_ASSIGN_OR_RAISE(auto input, source.Open());
- std::unique_ptr<parquet::ParquetFileReader> reader;
- try {
- reader = parquet::ParquetFileReader::Open(std::move(input), std::move(properties));
- } catch (const ::parquet::ParquetException& e) {
- return Status::IOError("Could not open parquet input source '", source.path(),
- "': ", e.what());
- }
+ auto maybe_reader = [&]() -> Result<std::unique_ptr<parquet::ParquetFileReader>> {
Review comment:
Less fragile than macros would be:
```c++
auto maybe_reader = parquet::CatchNotOk([&] {
return parquet::ParquetFileReader::Open(std::move(input), std::move(properties));
});
```
Could be implemented like:
```c++
template <typename Fn, typename Ret>
struct CatchNotOkImpl {
static typename EnsureResult<Ret>::type Call(Fn&& fn) { return std::forward<Fn>(fn)(); }
};
template <typename Fn>
struct CatchNotOkImpl<Fn, Status> {
static Status Call(Fn&& fn) { return std::forward<Fn>(fn)(); }
};
template <typename Fn>
struct CatchNotOkImpl<Fn, void> {
static Status Call(Fn&& fn) {
std::forward<Fn>(fn)();
return Status::OK();
}
};
template <typename Fn, typename Impl = CatchNotOkImpl<Fn, decltype(std::declval<Fn>()())>>
auto CatchNotOk(Fn&& fn) -> decltype(Impl::Call(std::forward<Fn>(fn))) try {
return Impl::Call(std::forward<Fn>(fn));
} catch (const ParquetStatusException& e) {
return e.status();
} catch (const ParquetException& e) {
return Status::IOError(e.what());
}
```
What do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org