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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
