drin commented on code in PR #13383:
URL: https://github.com/apache/arrow/pull/13383#discussion_r898398633
##########
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:
done. With this change, I was thinking of trying to have a test that
implicitly closes a RecordBatchReader in a scenario where there is a
non-successful status. Looking at the PR that added a definition for `Close()`
(https://github.com/apache/arrow/pull/13205), it seems like places where this
might be done would be `compute/exec/plan_test.cc` or `dataset/scanner_test.cc`.
Should I try to add this? I am not sure that there's much value add
validation-wise (seems like log unit tests and status unit tests should be
sufficient).
--
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]
