drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r899683135
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -393,7 +393,7 @@ Result<std::shared_ptr<RecordBatchReader>>
RecordBatchReader::Make(
RecordBatchReader::~RecordBatchReader() {
auto st = this->Close();
if (!st.ok()) {
- ARROW_LOG(WARNING) << "Implicitly called RecordBatchReader::Close failed:
" << st;
+ st.Warn("Implicitly called RecordBatchReader::Close failed");
Review Comment:
So, I looked at why ARROW-16515 was created and it was related to
`SubstraitExecutor` and keeping a reference to it. The destructor in that class
uses `ARROW_CHECK_OK` which is similar to `ARROW_WARN_NOT_OK` but logs at the
`FATAL` level instead. I don't see a clear test case for `Warn()` in that
context.
For now, I created a test class that extends RecordBatchReader and returns
non-OK in it's Close() method as a simple, lightweight approach to exercising
this. I am not sure how important it is to validate the logging mechanism
itself, but at least this adds code coverage of the macro and a "failing"
Close() method called from a constructor.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
