You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/11 21:53:45 UTC
[GitHub] [arrow] anjakefala opened a new issue, #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
anjakefala opened a new issue, #33618:
URL: https://github.com/apache/arrow/issues/33618
### Describe the enhancement requested
Here is an example `std::filesystem`:
```
for (const auto &path : std::filesystem::directory_iterator(dataDir)) {
if (path.is_regular_file()) {
// logic here
}
}
```
Here is a similar example using `arrow::fs::FileSelect()`:
```
auto selector = arrow::fs::FileSelector();
selector.base_dir = dir;
selector.allow_not_found = false;
selector.recursive = false;
auto fileInfo = fs_->GetFileInfo(selector);
for (const auto &pathInfo: fileInfo.ValueOrDie()) {
pathInfo.IsFile();
// OR
pathInfo.IsDirectory();
}
```
The `arrow::fs::FileSelector()` example takes 10x as long. Under the hood there is a stat() call which fills in extra metadata information, and this increases time spent making the call. `std::filesystem` instead uses `readdir`, which checks whether an entry is a file or directory, but does not provide extra metadata information. This results in a much faster system call.
`std::filesystem::directory_iterator` gives the user a `directory_entry` which does have a `file_size` attribute. Calling `file_size` the first time triggers a stat call to the file but a stat call is not made if you don't call `file_size`.
I propose a similar adjustment to the `arrow::fs` API, which allows a user to perform an entry identity check with a `readdir` system call, such that a `stat` call is only used if the user requests the extra metadata (e.g. file size and mtime).
### Component(s)
C++
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anjakefala commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
anjakefala commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1396304799
> If local filesystems are the odd one out, perhaps it doesn't make sense in the filesystem API and we should instead recommend a work around for that special case.
Since the `datasets` API does make use of `arrow::fs`, and the local filesystem is a very common use-case, I do think there is an argument to be made for implementing this change, even if it only applies to the localFS.
> it seems worth it to check what other filesystems (S3, Azure, GCS, HDFS, etc.) such an optimization is meaningful for.
@wjones127 did some preliminary research on this front:
> The following provide LastModified + Size at no additional cost:
> * S3 ([docs](https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#API_ListObjectsV2_Examples))
> * GCS ([docs](https://cloud.google.com/storage/docs/json_api/v1/objects/list))
> * Azure Blob store ([docs](https://learn.microsoft.com/en-us/rest/api/storageservices/list-blobs?tabs=azure-ad#response-body), although you can request to leave out the metadata (not sure if this is any faster though)
I tried to answer this question for HDFS ([docs 1](https://hadoop.apache.org/docs/r2.7.0/hadoop-project-dist/hadoop-common/FileSystemShell.html#ls) [docs 2](https://apache.googlesource.com/hadoop-common/+/e39a8dcd753c0e26c62a67026a554cf2242b7c0f/hadoop-hdfs-project/hadoop-hdfs/src/main/native/libhdfs/hdfs.c#2974).
It seems like `HDFS dfs` also uses `stat()`, and would probably also benefit from a similar to change, but I am not sure of the work involved for that. `GetFileInfo` seems to be the API used to set the metadata.
@westonpace suggested that the change would be to add something like `needs_extended_file_info` to the file selector or the GetFileInfo call. Would it make sense to add something such that for localfs we decide whether to use `stat` or `readdir` system call? Is it feasible to add an argument to a function in `arrow::fs` that only applies to one of the filesystems and does nothing for the others? If it does, I can review the relevant code, and propose code.
--
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] anjakefala commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
anjakefala commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1379534704
take
--
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] wjones127 commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
wjones127 commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1397675298
> Would it make sense to add something such that for localfs we decide whether to use stat or readdir system call?
Seems like it could make sense.
> Is it feasible to add an argument to a function in arrow::fs that only applies to one of the filesystems and does nothing for the others?
`LocalFileSystem` is a subclass of `FileSystem`. So one option is to add a separate method on `LocalFileSystem`, and have users downcast when they want to use that method. For example, see how LocalFileSystem implements it's specialized `Equals` method: https://github.com/apache/arrow/blob/bf8780d0ff794c50312d799a9e877430e99dcf8b/cpp/src/arrow/filesystem/localfs.cc#L294-L301
The other method is what you suggested, adding a `needs_extended_file_info` parameter to `FileSelector`.
I don't have a strong opinion on which would be better right now.
--
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 issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
westonpace commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1397743668
> LocalFileSystem is a subclass of FileSystem. So one option is to add a separate method on LocalFileSystem, and have users downcast when they want to use that method. For example, see how LocalFileSystem implements it's specialized Equals method:
This hides the potential optimization from the user though. Most users would still use the inefficient path unless they happen to be reading this issue.
--
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] anjakefala commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by "anjakefala (via GitHub)" <gi...@apache.org>.
anjakefala commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1423158795
I wrote a first draft (that does not compile yet), and ran into a few complications:
* `readdir` seems to expect a directory stream pointer, and then it will return the next directory entry in that stream: https://man7.org/linux/man-pages/man3/readdir.3.html An example of a system call that returns a directory stream: https://man7.org/linux/man-pages/man3/opendir.3.html. From these it seems that it is not a 1-on-1 replacement with `stat`, which accepts a path to a specific entry. My understanding is we cannot give `readdir` a path to an entry, and ask it if the entry is a file or directory. The way it seems to usually be used is by looping through the entries in a directory, and checking what they are e.g. https://stackoverflow.com/questions/39429803/how-to-list-first-level-directories-only-in-c/39430337#39430337.
There is probably a way to use `readdir` to get the information we want, but before I invest more time into that, I wanted to suggest an alternative approach.
It might be simpler to instead use `std::filesystem::is_directory`: https://en.cppreference.com/w/cpp/filesystem/is_directory and `std::filesystem::is_regular_file` which are supported in C++17. My understanding is that Apache Arrow is on C++17 compiler, and that these functions are faster than `stat` (though, of course, I will verify this).
I am going to explore the `std::filesystem` approach, but am welcoming feedback or insight.
--
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] anjakefala commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by "anjakefala (via GitHub)" <gi...@apache.org>.
anjakefala commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1409216712
To add a `needs_extended_file_info` parameter to `GetFileInfo`, these are all of the files that would need to be updated:
modified: cpp/src/arrow/filesystem/filesystem.cc
modified: cpp/src/arrow/filesystem/filesystem.h
modified: cpp/src/arrow/filesystem/gcsfs.cc
modified: cpp/src/arrow/filesystem/gcsfs.h
modified: cpp/src/arrow/filesystem/hdfs.cc
modified: cpp/src/arrow/filesystem/hdfs.h
modified: cpp/src/arrow/filesystem/localfs.cc
modified: cpp/src/arrow/filesystem/localfs.h
modified: cpp/src/arrow/filesystem/mockfs.cc
modified: cpp/src/arrow/filesystem/mockfs.h
modified: cpp/src/arrow/filesystem/s3fs.cc
modified: cpp/src/arrow/filesystem/s3fs.h
modified: python/pyarrow/src/arrow/python/filesystem.cc
modified: python/pyarrow/src/arrow/python/filesystem.h
--
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] wjones127 commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1409326671
If you added the option to the `FileSelector` class, does that reduce the number of required changes?
--
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] anjakefala commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by "anjakefala (via GitHub)" <gi...@apache.org>.
anjakefala commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1423154266
Adding it to the `FileSelector` class does indeed reduce the number of required changes! That is the approach I am going to go with.
--
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 issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
westonpace commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1379550834
I don't think we can implement this without changes to the filesystem API. We have:
```
virtual Result<FileInfoVector> GetFileInfo(const std::vector<std::string>& paths);
```
So, we don't know at call time if the user is just after the file type or if they are after the type and the size.
`FileInfo` is not virtual so we can't lazily call `stat` in the same way that `std::filesystem` does.
However, I believe this would speed up dataset discovery (assuming the user is not asking to inspect all fragments) and so I am interested :)
--
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
Re: [I] [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()` [arrow]
Posted by "thisisnic (via GitHub) via issues" <is...@arrow.apache.org>.
thisisnic closed issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
URL: https://github.com/apache/arrow/issues/33618
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()` [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-3921660636
This issue has been marked as stale because it has had no activity in the past 2 years. Please remove the stale label or comment below, or this issue will be closed in 14 days.
--
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] wjones127 commented on issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by GitBox <gi...@apache.org>.
wjones127 commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1379539863
Since `arrow::fs` is meant to be an abstraction over multiple file systems, it seems worth it to check what other filesystems (S3, Azure, GCS, HDFS, etc.) such an optimization is meaningful for. If local filesystems are the odd one out, perhaps it doesn't make sense in the filesystem API and we should instead recommend a work around for that special case.
--
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 issue #33618: [C++] Improve arrow::fs::FileSelect performance for `IsFile()` and `IsDirectory()`
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #33618:
URL: https://github.com/apache/arrow/issues/33618#issuecomment-1423532901
Using `std::filesystem` sounds like a great approach even if `readdir` was working. We should probably start investigating other places we can eventually migrate to `std::filesystem` as it may help reduce both complexity and duplicate (windows / linux /mac) paths.
--
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