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 2022/05/20 18:45:00 UTC

[GitHub] [arrow] vibhatha opened a new pull request, #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

vibhatha opened a new pull request, #13205:
URL: https://github.com/apache/arrow/pull/13205

   Draft PR
   
   - [ ] Adding `Close` method for `RecordBatchReader`


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1136558312

   @westonpace updated the PR. 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r887428991


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +92,15 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override {
+    std::shared_ptr<RecordBatch> batch;
+    RETURN_NOT_OK(ReadNext(&batch));
+    while (batch != nullptr) {
+      RETURN_NOT_OK(ReadNext(&batch));
+    }
+    return Status::OK();

Review Comment:
   That seems a little heavyweight.  I think most of our record batch readers can safely abandon a read partway through.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1133208098

   https://issues.apache.org/jira/browse/ARROW-16515


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r887428991


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +92,15 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override {
+    std::shared_ptr<RecordBatch> batch;
+    RETURN_NOT_OK(ReadNext(&batch));
+    while (batch != nullptr) {
+      RETURN_NOT_OK(ReadNext(&batch));
+    }
+    return Status::OK();

Review Comment:
   That seems a little heavyweight.  I don't most of our record batch readers can safely abandon a read partway through.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1142189892

   @westonpace @lidavidm updated the PR. 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r885578680


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -464,6 +464,13 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
     std::shared_ptr<Schema> schema,
     std::function<Future<util::optional<ExecBatch>>()> gen, MemoryPool* pool) {
   struct Impl : RecordBatchReader {
+    ~Impl() {
+      auto st = this->Close();
+      if (!st.ok()) {
+        ARROW_LOG(WARNING) << "MakeGeneratorReader failed in finalzing reading.";
+      }

Review Comment:
   👍



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r887430168


##########
cpp/src/arrow/record_batch.cc:
##########
@@ -390,4 +390,12 @@ Result<std::shared_ptr<RecordBatchReader>> RecordBatchReader::Make(
   return std::make_shared<SimpleRecordBatchReader>(std::move(batches), schema);
 }
 
+RecordBatchReader::~RecordBatchReader() {
+  auto st = this->Close();
+  if (!st.ok()) {
+    ARROW_LOG(WARNING) << "Implicitly called RecordBatchReader::Close failed: "
+                       << st.ToString();

Review Comment:
   ```suggestion
                          << st;
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r882190050


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -464,6 +464,13 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
     std::shared_ptr<Schema> schema,
     std::function<Future<util::optional<ExecBatch>>()> gen, MemoryPool* pool) {
   struct Impl : RecordBatchReader {
+    ~Impl() {
+      auto st = this->Close();
+      if (!st.ok()) {
+        ARROW_LOG(WARNING) << "MakeGeneratorReader failed in finalzing reading.";
+      }

Review Comment:
   Agreed, this should just be the default. I would also make the message clear that it's because Close() was not explicitly called, and include the status in the message. 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r887429332


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +92,15 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override {
+    std::shared_ptr<RecordBatch> batch;
+    RETURN_NOT_OK(ReadNext(&batch));
+    while (batch != nullptr) {
+      RETURN_NOT_OK(ReadNext(&batch));
+    }
+    return Status::OK();

Review Comment:
   Fair enough.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1133519386

   > @westonpace while we are at it, shall we add a ‘IsClosed’ method returning a bool?
   
   I don't feel too strongly either way but I'd lean towards not adding `IsClosed`.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r884536170


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -464,6 +464,13 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
     std::shared_ptr<Schema> schema,
     std::function<Future<util::optional<ExecBatch>>()> gen, MemoryPool* pool) {
   struct Impl : RecordBatchReader {
+    ~Impl() {
+      auto st = this->Close();
+      if (!st.ok()) {
+        ARROW_LOG(WARNING) << "MakeGeneratorReader failed in finalzing reading.";
+      }

Review Comment:
   I understand, but can we include the `arrow/util/logging.h` in public APIs?
   



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r886705401


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +92,15 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override {
+    std::shared_ptr<RecordBatch> batch;
+    RETURN_NOT_OK(ReadNext(&batch));
+    while (batch != nullptr) {
+      RETURN_NOT_OK(ReadNext(&batch));
+    }
+    return Status::OK();

Review Comment:
   I wonder if this should just be made the default implementation?



##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -74,6 +74,13 @@ class ScannerRecordBatchReader : public RecordBatchReader {
                                     TaggedRecordBatchIterator delegate)
       : schema_(std::move(schema)), delegate_(std::move(delegate)) {}
 
+  ~ScannerRecordBatchReader() {
+    auto st = this->Close();
+    if (!st.ok()) {
+      ARROW_LOG(WARNING) << "ScannerRecordBatchReader failed in finalzing reading.";

Review Comment:
   Is there any need to override the destructor here? Also this error message is less informative than the one on the base class.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace merged pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace merged PR #13205:
URL: https://github.com/apache/arrow/pull/13205


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r878620265


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +85,8 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override { return Status::OK(); }

Review Comment:
   Sure. 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r885543631


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -464,6 +464,13 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
     std::shared_ptr<Schema> schema,
     std::function<Future<util::optional<ExecBatch>>()> gen, MemoryPool* pool) {
   struct Impl : RecordBatchReader {
+    ~Impl() {
+      auto st = this->Close();
+      if (!st.ok()) {
+        ARROW_LOG(WARNING) << "MakeGeneratorReader failed in finalzing reading.";
+      }

Review Comment:
   We can move the definition to the `.cc` file, no?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r887598656


##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -74,6 +74,13 @@ class ScannerRecordBatchReader : public RecordBatchReader {
                                     TaggedRecordBatchIterator delegate)
       : schema_(std::move(schema)), delegate_(std::move(delegate)) {}
 
+  ~ScannerRecordBatchReader() {
+    auto st = this->Close();
+    if (!st.ok()) {
+      ARROW_LOG(WARNING) << "ScannerRecordBatchReader failed in finalzing reading.";

Review Comment:
   Yes, may be now we don't need this as we modified the base class. 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r878620428


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -476,6 +476,8 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
       return Status::OK();
     }
 
+    Status Close() override { return Status::OK(); }

Review Comment:
   👍



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r885727602


##########
cpp/src/arrow/record_batch.cc:
##########
@@ -390,4 +390,11 @@ Result<std::shared_ptr<RecordBatchReader>> RecordBatchReader::Make(
   return std::make_shared<SimpleRecordBatchReader>(std::move(batches), schema);
 }
 
+RecordBatchReader::~RecordBatchReader() {
+  auto st = this->Close();
+  if (!st.ok()) {
+    ARROW_LOG(WARNING) << "Implicityly called Close, but failed with " << st.message();

Review Comment:
   ```suggestion
       ARROW_LOG(WARNING) << "Implicitly called RecordBatchReader::Close failed: " << st.ToString();
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r878616768


##########
cpp/src/arrow/record_batch.h:
##########
@@ -243,6 +243,9 @@ class ARROW_EXPORT RecordBatchReader {
     return batch;
   }
 
+  /// \brief finalize reader
+  virtual Status Close() = 0;

Review Comment:
   Instead of making this pure virtual (`= 0`) can you make this virtual and give it an implementation?
   ```suggestion
     virtual Status Close() {
       return Status::OK();
     };
   ```



##########
cpp/src/arrow/dataset/scanner.cc:
##########
@@ -85,6 +85,8 @@ class ScannerRecordBatchReader : public RecordBatchReader {
     return Status::OK();
   }
 
+  Status Close() override { return Status::OK(); }

Review Comment:
   We should execute exec plans until they finish.  So this should look something like...
   ```
   Status Close() override {
     std::shared_ptr<RecordBatch> batch;
     RETURN_NOT_OK(ReadNext(&batch));
     while (batch != nullptr) {
       RETURN_NOT_OK(ReadNext(&batch));
     }
     return Status::OK();
   }
   ```



##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -476,6 +476,8 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
       return Status::OK();
     }
 
+    Status Close() override { return Status::OK(); }

Review Comment:
   This should read from the generator until the end is reached.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1133497690

   @westonpace while we are at it, shall we add a ‘IsClosed’ method returning a bool?


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1144690692

   cc @westonpace some tests are consistently failing. But doesn't seem to be related to this PR. 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1144783183

   I kicked most of the CI pipelines.
   
   Note https://issues.apache.org/jira/browse/ARROW-16722 about some of the failures


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] vibhatha commented on pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13205:
URL: https://github.com/apache/arrow/pull/13205#issuecomment-1144793477

   > I kicked most of the CI pipelines.
   > 
   > Note https://issues.apache.org/jira/browse/ARROW-16722 about some of the failures
   
   Thanks for the note @lidavidm 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #13205: ARROW-16515: [C++] Adding a Close method to RecordBatchReader

Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13205:
URL: https://github.com/apache/arrow/pull/13205#discussion_r882189069


##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -464,6 +464,13 @@ std::shared_ptr<RecordBatchReader> MakeGeneratorReader(
     std::shared_ptr<Schema> schema,
     std::function<Future<util::optional<ExecBatch>>()> gen, MemoryPool* pool) {
   struct Impl : RecordBatchReader {
+    ~Impl() {
+      auto st = this->Close();
+      if (!st.ok()) {
+        ARROW_LOG(WARNING) << "MakeGeneratorReader failed in finalzing reading.";
+      }

Review Comment:
   I think we want this behavior in the default destructor for `RecordBatchReader`.  CC @lidavidm for second opinion. 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org