[GitHub] [arrow] pitrou commented on a change in pull request #11854: ARROW-12629: [C++] Option for disabling read ahead for csv and json readeers

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #11854:
URL: https://github.com/apache/arrow/pull/11854#discussion_r767959155



##########
File path: cpp/src/arrow/csv/options.h
##########
@@ -166,6 +166,9 @@ struct ARROW_EXPORT ReadOptions {
   /// Create read options with default values
   static ReadOptions Defaults();
 
+  /// weather to use an IO thread to read ahead

Review comment:
       ```suggestion
     /// Whether to use an IO thread to read ahead while other blocks are decoding
   ```

##########
File path: cpp/src/arrow/json/options.h
##########
@@ -65,6 +65,8 @@ struct ARROW_EXPORT ReadOptions {
   /// Block size we request from the IO layer; also determines the size of
   /// chunks when use_threads is true
   int32_t block_size = 1 << 20;  // 1 MB
+  /// weather we will use a separate thread for read ahead
+  bool use_read_ahead = true;

Review comment:
       ```suggestion
     bool use_readahead = true;
   ```

##########
File path: cpp/src/arrow/csv/options.h
##########
@@ -166,6 +166,9 @@ struct ARROW_EXPORT ReadOptions {
   /// Create read options with default values
   static ReadOptions Defaults();
 
+  /// weather to use an IO thread to read ahead

Review comment:
       (feel free to reword differently if you find a better wording)

##########
File path: cpp/src/arrow/csv/options.h
##########
@@ -166,6 +166,9 @@ struct ARROW_EXPORT ReadOptions {
   /// Create read options with default values
   static ReadOptions Defaults();
 
+  /// weather to use an IO thread to read ahead
+  bool use_read_ahead = false;

Review comment:
       Also, probably name this `use_readahead` ("readahead" is a noun in this context)

##########
File path: cpp/src/arrow/csv/options.h
##########
@@ -166,6 +166,9 @@ struct ARROW_EXPORT ReadOptions {
   /// Create read options with default values
   static ReadOptions Defaults();
 
+  /// weather to use an IO thread to read ahead
+  bool use_read_ahead = false;

Review comment:
       Is there any reason to make this false by default?

##########
File path: cpp/src/arrow/json/options.h
##########
@@ -65,6 +65,8 @@ struct ARROW_EXPORT ReadOptions {
   /// Block size we request from the IO layer; also determines the size of
   /// chunks when use_threads is true
   int32_t block_size = 1 << 20;  // 1 MB
+  /// weather we will use a separate thread for read ahead

Review comment:
       Can you update the comment to match the one in the CSV options?

##########
File path: cpp/src/arrow/json/reader_test.cc
##########
@@ -35,10 +35,11 @@ using util::string_view;
 
 using internal::checked_cast;
 
-class ReaderTest : public ::testing::TestWithParam<bool> {
+class ReaderTest : public ::testing::TestWithParam<std::pair<bool, bool>> {

Review comment:
       This will start being a bit cryptic. Perhaps:
   
   ```suggestion
   struct ReaderTestParam {
     bool use_threads;
     bool use_readahead;
   };
   
   class ReaderTest : public ::testing::TestWithParam<ReaderTestParam> {
   ```




-- 
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