ARROW-7058: [C++] FileSystemDataSourceDiscovery should apply partition schemes relative to its base dir #5772
Conversation
… schemes relative to its base dir
There was a problem hiding this comment.
From an API perspective, this is the wrong approach. The caller can't disable this non-trivially (passing a selector with an empty base is... an implementation detail hack) and forces the caller to always pass a Selector even if there's none, e.g how can the user pass just an explicit list of FileStats?
This should live in PartitionScheme, independent of DataSourceDiscovery, Either via a prefix optional parameter or a composed StripBasePartitionScheme(child_scheme, prefix) if you don't want to add the prefix to all relevant schemes.
@nealrichardson any idea on how to make this ergonomic?
fsaintjacques
changed the title
|
IDK about C++ API ergonomics, I just know what the expected behavior should be. If I tell you my dataset is in |
| for (const auto& file : files) { | ||
| const auto& path = file.path(); | ||
| if (file.path().substr(0, base_dir.size()) != base_dir) continue; |
There was a problem hiding this comment.
We need to decide what should be the most friendly behavior in such case. I'd tempted to vote for parse without trimming instead of skipping the file.
std::string path = file.path();
if (!base_dir.empty() && path.substr(0, base_dir.size()) == base_dir) {
path = path.substr(base_dir.size());
}There was a problem hiding this comment.
Parsing some paths with a known prefix removed and other paths with an unknown prefix seems like a foot gun to me. I'd expect the files which lie outside base_dir to be picked up with a different instance of Discovery (with the correct base_dir for those files).
There was a problem hiding this comment.
I agree with Ben. If I'm supplying my own file paths, and some are relative to the given "base_dir" and some aren't, (1) the simple partition scheme of applying a schema to path segments probably won't just work across all of them if they don't share a base_dir, and if I want to use it I should put them in different Discovery instances; and (2) if we tried to get partition info from files outside of base_dir, it will probably error in parsing the path segments, so continue and not assign partition data to that file is probably safest.
* ARROW-7067: [CI] Disable code coverage on Travis-CI Closes apache#5778 from pitrou/ARROW-7067-travis-disable-coverage and squashes the following commits: bda5ff4 <Antoine Pitrou> ARROW-7067: Disable code coverage on Travis-CI Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]> * ARROW-7058: [C++] FileSystemDataSourceDiscovery should apply partition schemes relative to its base dir @nealrichardson Closes apache#5772 from bkietz/7058-FileSystemDataSourceDisco and squashes the following commits: c0edfa5 <Benjamin Kietzman> make base_dir of partition schemes explicitly optional f2f9689 <Benjamin Kietzman> add DCHECK for path containing selector 1bae5ff <Benjamin Kietzman> ARROW-7058: FilSystemDataSourceDiscovery should apply partition schemes relative to its base dir Authored-by: Benjamin Kietzman <[email protected]> Signed-off-by: François Saint-Jacques <[email protected]> * Refactor docker-compose file and use it with github actions. * Turn off gandiva and flight for the HDFS test [skip ci] * Missing --pyargs argument for the python test command [skip ci] * Add CentOS version to the manylinux image names [skip ci] * Fix manylinux volumes [skip ci]
@nealrichardson