[GitHub] [arrow] wjones127 commented on a diff in pull request #34170: GH-33618: [C++] add option needs_extended_file_info and implement localFS

["wjones127 (via GitHub)" <gi...@apache.org>]

wjones127 commented on code in PR #34170:
URL: https://github.com/apache/arrow/pull/34170#discussion_r1104991979


##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -132,8 +132,14 @@ struct ARROW_EXPORT FileSelector {
   bool recursive;
   /// The maximum number of subdirectories to recurse into.
   int32_t max_recursion;
-
-  FileSelector() : allow_not_found(false), recursive(false), max_recursion(INT32_MAX) {}
+  /// If true, use stat() to obtain extended file metadata.

Review Comment:
   Since the default is `true`, maybe better to frame as what happens if false. Also maybe preferable to frame in implementation-agnostic way (stat isn't invoked on Windows, and isn't applicable to remote filesystems):
   ```suggestion
     /// If false, may skip retrieving size and modification time metadata.
     ///
     /// On local filesystem, setting to false avoids an additional system call.
     /// On other filesystems, this may have no effect.
   ```



##########
cpp/src/arrow/filesystem/localfs.cc:
##########
@@ -210,6 +211,22 @@ Result<FileInfo> StatFile(const std::string& path) {
   return info;
 }
 
+Result<FileInfo> IdentifyFile(const std::string& path) {

Review Comment:
   This is defined within the POSIX branch (else) of the `#ifdef _WIN32` condition above. Since it's platform agnostic, I think you can pull this out to the top-level.



##########
cpp/src/arrow/filesystem/localfs_test.cc:
##########
@@ -472,6 +472,24 @@ TYPED_TEST(TestLocalFS, StressGetFileInfoGenerator) {
   }
 }
 
+TYPED_TEST(TestLocalFS, NeedsExtendedFileInfo) {
+  // Test setting needs_extended_file_info to true and false

Review Comment:
   Comment isn't accurate. I think the true case is well covered by other tests though.
   ```suggestion
     // Test setting needs_extended_file_info to false
   ```



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