ARROW-6720: [C++] Add FileSystemFactory and HadoopFileSystem under arrow::fs by xuechendi · Pull Request #5717 · apache/arrow

xuechendi · 2019-10-23T07:16:12Z

We previously work on JIRA Arrow-6720, PR#5522 to add a Parquet Reader/Writer Java API to Arrow
According to previous comments, I splitted that PR into three small ones.

For this PR, I added a new filesystem_util under arrow/filesystem to provide a filesystemfactory here. And also wrapped hdfs codes under arrow/io to arrow/filesystem, so filesystemfactory will open filesystem by its specified path and then return a unified filesystem instance.

Signed-off-by: Chendi Xue chendi.xue@intel.com

github-actions · 2019-10-23T07:31:04Z

https://issues.apache.org/jira/browse/ARROW-6720

emkornfield · 2019-10-24T04:26:24Z

@pitrou would you have time to review? (I think you've been working on file system stuff?)

bkietz · 2019-10-24T13:33:30Z

I'm working on the related PR (currently still a draft): #5709
I'll review this

xuechendi · 2019-10-24T13:49:24Z

I'm working on the related PR (currently still a draft): #5709
I'll review this

Cool! thanks, @bkietz . The reason I am doing this is that we want to add a parquet reader/writer java api in arrow, and cpp part of codes are in PR #5719 .
When doing this, we need to add some unified API so we can read either from localfs or hdfs, that is why I wrapped hdfs under arrow/io into arrow/filesystem and also added a filesystemfactory.
I hope we can work on this together and get this PR or some similiar APIs ready as soon as possible, so parquet and also orc jni wrapper can take use of this.

bkietz

Thanks for doing this, it looks like a good start. Please add some test cases

bkietz · 2019-10-24T13:36:52Z

Since this is just a struct, please simplify by providing a default constructor (or just default member initializers) instead of a factory function. Probably better: make a constructor which requires the path (since a reasonable default doesn't exist for that)

bkietz · 2019-10-24T13:39:10Z

I don't think this mutator is necessary; IMHO
opts.use_hdfs3 = true; opts.replication = 2;
is more expressive than
opts.ConfigureHDFSOptions(true, 2);
since the options are specified by name.

actually, instead of duplicating use_hdfs3 in both FileSystemOptions and HdfsOptions, it would probably be better FileSystemOptions could simply contain an instance of HdfsOptions (required iff fs_type is inferred as hdfs)

bkietz · 2019-10-24T13:45:39Z

If fs_type is inferred from the path, I think this inference should be folded into PathInfo::Make. (since you use fs_type below, keep it as a member of PathInfo)

bkietz · 2019-10-24T13:49:37Z

Suggested change
}break;
break;
}

bkietz · 2019-10-24T13:50:54Z

This needs to be far more rigorous. For example, this would infer /home/bkietz/data/hdfs_perf/dat.par as hdfs. Look at arrow/util/uri.h

bkietz · 2019-10-24T13:59:29Z

I don't think it's necessary to add a pimpl'd class for this; I think it's fine to just declare
Status MakeFileSystem(const FileSystemOptions& options, std::shared_ptr<FileSystem>* fs);

@pitrou 's comment would make this even simpler:
Status MakeFileSystem(const std::string& uri, std::shared_ptr<FileSystem>* fs);

bkietz · 2019-10-24T14:03:20Z

Suggested change
enumstructFileSystemType { HDFS, LOCAL, S3 };
enumclassFileSystemType { HDFS, LOCAL, S3 };

bkietz · 2019-10-24T14:11:09Z

Code style: this name should be ParseLocal. I see that you're following the enumeration; if you like you could use a template to follow it more closely:
template <FileSystemType> static Status Parse(const std::string& full_path, std::shared_ptr<PathInfo>* path_info); template <> static Status Parse<FileSystemType::LOCAL>(const std::string& full_path, std::shared_ptr<PathInfo>* path_info) { // LOCAL parse impl }

bkietz · 2019-10-24T14:16:28Z

If FileSystemFactory is not explicitly specific to hdfs then it should be compiled even if ARROW_HDFS is not set (and should not break the build in that case; this will probably require some #ifdef blocks in filesystem_utils.cc). If it is specific to HDFS then it should be refactored to make that explicit

pitrou · 2019-10-24T14:24:48Z

I'd rather see the FileSystemFactory and FileSystemOptions stuff removed from this PR.

Ideally, I'd like high-level filesystem configuration to be URI-based. URI representation for remote and local paths is quite widespread, and it makes things easier for bindings. See e.g. https://docs.dask.org/en/latest/remote-data-services.html

For example, you could have a URI such as hdfs://host/some/path?replication=3 which would hold necessary information.

bkietz · 2019-10-24T14:29:04Z

sorry, my mistake.

xuechendi · 2019-10-24T14:35:41Z

I'd rather see the FileSystemFactory and FileSystemOptions stuff removed from this PR.
Ideally, I'd like high-level filesystem configuration to be URI-based. URI representation for remote and local paths is quite widespread, and it makes things easier for bindings. See e.g. https://docs.dask.org/en/latest/remote-data-services.html
For example, you could have a URI such as hdfs:///some/path?replication=3 which would hold necessary information.

hi, @pitrou , I see, I can add a URI-based path parser, thanks. And the reason why I put Hdfs and Filesystem_utils in the same PR is because the ultimate goal of doing this is to enable a parquet java reader/writer like the one #5522 did, this is more like a step stone to that, so filesystemfactory is more important to me in this PR. Hope that will be OK to you.

xuechendi · 2019-10-24T14:36:42Z

sorry, my mistake.

No problem, and thanks for all you review, I will fix those tomorrow.

pitrou · 2019-10-24T15:57:31Z

The options and factory here are not ok as designed. Choosing HDFS should certainly not be a boolean flag. As I said, using a URI mechanism instead should be more flexible and easier for users.

xuechendi · 2019-10-24T23:04:03Z

The usehdfs3 flag here is not for choosing HDFS, it’s more like an option to tell HDFS using hdfs3 lib, and it will be fixed once we use URL based path

xuechendi · 2019-10-25T16:01:19Z

Hi, @bkietz @pitrou
I submitted a new commit to remove FilesystemOptions and using URI-based path to pass options.
For URI parser, I used a regex to split URI into four part: host/port/path/options. Then handle each part respectively.
Here is an test example:

======
Original string is hdfs://vsr602:9000/path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet?replication=1&use_hdfs3=1
Type is HDFS
Host is vsr602
port is 9000
path is /path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet
replication: 1
use_hdfs3: 1

======
Original string is hdfs://192.168.0.1:9000/path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet?replication=1&use_hdfs3=1
Type is HDFS
Host is 192.168.0.1
port is 9000
path is /path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet
replication: 1
use_hdfs3: 1

======
Original string is file:///path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet
Type is LOCALFS
path is /path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet

======
Original string is /path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet
Type is LOCALFS
path is /path/to/ws_sold_date_sk=2452642/part-00196-5adaa592-957b-475d-95f7-64881c8e0c68.c000.snappy.parquet

kou · 2019-10-30T01:06:20Z

It's worked on #5756.
We can ignore it in this PR.

xuechendi · 2019-10-30T13:00:01Z

@wesm @pitrou @bkietz ,hi, guys, tests are added and codes passed all checks, thanks

bkietz · 2019-10-31T15:50:29Z

Is it necessary to make PathInfo public? it's only constructible from a string, so it seems that it should be an implementation detail of arrow::fs::MakeFileSystem(). If you want to unit test PathInfo separately, you can declare it in an internal header (for example, filesystem/util_internal.h) so that it can be included by tests as well

I made PathInfo public is because after we getting the std::shared_ptr instance, we will also need to use path_info->GetFileName() and path_info-> GetDirectoryName() to see what file is asked in the URI-based path, take filesystem_utils_test#133 as an example.

It seems that only the filesystem factory and unit tests will need to make that query, so PathInfo can be moved to an internal header.

@bkietz, oh, I aimed to use unit tests to indicate that later on who used file systemfactory will need Pathinfo
If that doesn’t make clear to you, do you mind to take a look of my following pr #5719 ? It will need pathinfo to open a parquet input file and output stream check this link to line of codes

@bkietz , as I explained before, I had submitted a huge pr #5522 before and @emkornfield asked me to make the pr into smaller ones , so I submitted two PR #5717 and #5719 for file system unified api and parquet jni bridge, that is why I needed this pathinfo class as a public one, hope this makes sense to you!

@xuechendi Your only usage of it outside file system factory in that PR is extracting the directory and file name in the jni bridge. If you need that to be public then you could expose (for example) static std::string HadoopFileSystem::GetPath(const std::string& uri) as a separate helper function and keep PathInfo private

hi, @bkietz , I am not sure if I get what you proposed, I thought hadoopfilesystem is a class for filesystem operations, such as create dir, delete dir, open read file and open write file, so we should not store infomations like filepath inside hadoopfilesystem, am I right?
And if I expose a function called getFilePath inside hadoopfilesystem, then I still need to expose one inside s3fs and localfs when I need to open files through path, why don't we just put this as an public PathInfo class inside filesystem_utils.h?
Do you mind if we can discuss on this in an IM? I felt like we can only discuss on this one time a day, which make the conversion very slow. my gmail account is xuechendi@gmail.com

If the only use case for the PathInfo class is to retrieve file paths from uris then that doesn't warrant a public class. You're right that it applies to more than HDFS, maybe you could make it a static member of Uri:
Result<std::string> Uri::GetPath(const std::string& uri) { Uri parsed; RETURN_NOT_OK(parsed.Parse(uri)); return parsed.path(); }

@bkietz , I see, I’ll add it to URI, thanks

bkietz · 2019-10-31T15:53:53Z

Suggested change
#if !defined _WIN32
#ifndef _WIN32

wesm · 2019-10-31T21:26:31Z

I'll let @bkietz and @pitrou look after this, if you need me to take a look at something please let me know

codecov-io · 2019-11-01T02:39:25Z

Codecov Report

Merging #5717 into master will increase coverage by 0.42%.
The diff coverage is 54.39%.

@@ Coverage Diff @@## master #5717 +/- ##
==========================================
+ Coverage 88.99% 89.42% +0.42% 
==========================================
Files 1006 820 -186 Lines 137241 122141 -15100 Branches 1501 0 -1501 ==========================================
- Hits 122143 109225 -12918 + Misses 14733 12916 -1817 + Partials 365 0 -365

Impacted Files	Coverage Δ
cpp/src/arrow/filesystem/hdfs.h	`100% <100%> (ø)`
cpp/src/arrow/filesystem/filesystem_utils.h	`100% <100%> (ø)`
cpp/src/arrow/filesystem/hdfs.cc	`22.06% <22.06%> (ø)`
cpp/src/arrow/filesystem/hdfs_test.cc	`48.57% <48.57%> (ø)`
cpp/src/arrow/filesystem/filesystem_utils.cc	`71.96% <71.96%> (ø)`
cpp/src/arrow/filesystem/filesystem_utils_test.cc	`98.3% <98.3%> (ø)`
cpp/src/arrow/json/converter.cc	`90.05% <0%> (-1.76%)`	⬇️
cpp/src/arrow/json/chunked_builder.cc	`80% <0%> (-1.67%)`	⬇️
cpp/src/arrow/util/thread_pool_test.cc	`97.66% <0%> (-0.94%)`	⬇️
go/arrow/ipc/writer.go
... and 198 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ed30a...dd317d7. Read the comment docs.

xuechendi · 2019-11-01T11:13:57Z

Hi, @bkietz , I've fixed codes according to your review, thanks

pitrou · 2019-11-04T17:14:30Z

@xuechendi Once #5770 is merged, you can use it to avoid parsing query strings yourself...

xuechendi · 2019-11-04T22:33:35Z

@pitrou, I see, when do you think #5770 will be merged, and do you think this PR need to wait until #5770 being merged?

pitrou · 2019-11-05T08:50:33Z

@xuechendi It's merged now.

Signed-off-by: Chendi Xue <chendi.xue@intel.com>

1. Make MakeFileSystem as a namespace function 2. Use uri.h to parse path string Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi · 2019-11-06T23:10:01Z

@bkietz @pitrou , I’ve modified codes to use uri.h for path string parsing and made pathinfo private.

pitrou

Thank you @xuechendi. This is much better already!

pitrou · 2019-11-07T13:25:12Z

+class HadoopFileSystem::Impl {
+ public:
+ explicit Impl(HdfsOptions options) : options_(std::move(options)) {}
+ ~Impl() { auto status = Close(); }


Shouldn't you log errors here?

Yes, I should, but since this is a destruction method, I am not sure the preferred way, should I throw an exception here? Or any other suggestions?

pitrou · 2019-11-07T13:26:42Z

+ RETURN_NOT_OK(client_->GetChildren(select.base_dir, &file_list));
+ for (auto file : file_list) {
+ FileStats stat;
+ RETURN_NOT_OK(GetTargetStats(file, &stat));


It seems you are not handling the allow_non_existent, recursive and max_recursion members in Selector.

This is fixed in latest commit

pitrou · 2019-11-07T13:27:46Z

+ std::vector<std::string> file_list;
+ RETURN_NOT_OK(client_->GetChildren(path, &file_list));
+ for (auto file : file_list) {
+ RETURN_NOT_OK(client_->Delete(path + file));


What if file is a directory? DeleteDirContents is supposed to be recursive.

Delete Function in io/hdfs.cc is able to delete the input path either is file or directory, and I will change the recursive flag to true.

pitrou · 2019-11-07T13:29:38Z

+}
+
+void HdfsOptions::ConfigureHdfsUser(const std::string& user_name) {
+ if (!user_name.empty()) {


My idea is if we don't need to specify username, then leave it as originally is instead of changing it to "", so if there is some assumption for hdfs_config, for example, if user is null, then use is XXX, I won't make override that.

pitrou · 2019-11-07T13:53:33Z

+ fs_type_ = GetFileSystemTypeFromString(uri.scheme());
+ switch (fs_type_) {
+ case FileSystemType::HDFS:
+ options_.emplace("host_name", uri.host());


I think all the HDFS specifics (options, etc.) should be moved to hfds.h / hdfs.cc.
For example you could have:
classARROW_EXPORT HadoopFileSystem : public FileSystem { public:// .../// Create a HdfsFileSystem instance from the given options.static Status Make(const HdfsOptions& options, std::shared_ptr<HadoopFileSystem>* out); /// Create a HdfsFileSystem instance from the given URI.static Status Make(const Uri& uri, std::shared_ptr<HadoopFileSystem>* out); };
(and similarly LocalFileSystem::Make)

I did as that way is because S3Options is configured outside S3FileSystem, so I am thinking to do the same thing here.

S3Options is configured in s3fs.h / s3fs.cc. Similarly, HDFS options should be configured in hdfs.h / hdfs.cc.

I am a little confused, if S3Options is configured internally in s3fs.cc why s3fs.h makes both S3Options and Make function which uses S3Options as input ARROW_EXPORT? Doesn’t that means users should create and configure S3Options instance firstly then pass to Make function as parameter? Can you give me a more specific example of how you would like to see ?

xuechendi · 2019-11-11T13:48:04Z

@pitrou, I’ve fixed issues you mentioned and passed CI tests, one remaining one is I didn’t return error in the hadoopfilesystem destruction method, maybe you can give me some advice ?

Signed-off-by: Chendi Xue <chendi.xue@intel.com>

pitrou · 2019-11-12T16:40:05Z

@xuechendi I'm working on a new PR to replace this one.

xuechendi · 2019-11-12T22:23:04Z

@pitrou , I am confused, Does that means this PR won’t be merged? I took quite an effort on this, may I know the reason?

pitrou · 2019-11-12T22:31:15Z

Don't worry @xuechendi, I'm basing my work on yours. It's just a way of getting to completion quicker ;-)

xuechendi · 2019-11-12T23:01:33Z

@pitrou, I see, thanks!

xuechendi · 2019-11-13T15:25:33Z

This PR has been moved to #5820 by @pitrou .

xuechendi mentioned this pull request Oct 23, 2019
ARROW-6720:[JAVA][C++]Support Parquet Read and Write in Java #5522
Closed

xuechendi force-pushed the wip_upstream_filesystem branch 6 times, most recently from 93ed5d2 to 0393435Compare October 23, 2019 13:21

xuechendi mentioned this pull request Oct 23, 2019
ARROW-6720: [C++] Add Parquet Reader/Writer Adapter for JNI Bridge #5719
Closed

fsaintjacques changed the title ~~ARROW-6720:[C++]Add FileSystemFactory and HadoopFileSystem under arrow fs~~ARROW-6720:[C++] Add FileSystemFactory and HadoopFileSystem under arrow fsOct 23, 2019

emkornfield changed the title ~~ARROW-6720:[C++] Add FileSystemFactory and HadoopFileSystem under arrow fs~~ARROW-6720: [C++] Add FileSystemFactory and HadoopFileSystem under arrow fsOct 24, 2019

emkornfield mentioned this pull request Oct 24, 2019
ARROW-6055: [C++] Refactor arrow::io hdfs to implement arrow::filesystem #5709
Closed

xuechendi force-pushed the wip_upstream_filesystem branch from 0393435 to ae352c2Compare October 24, 2019 08:11

xuechendi changed the title ~~ARROW-6720: [C++] Add FileSystemFactory and HadoopFileSystem under arrow fs~~ARROW-6720: [C++] Add FileSystemFactory and HadoopFileSystem under arrow::fsOct 24, 2019

bkietz assigned bkietz and unassigned bkietz Oct 24, 2019

bkietz self-requested a review October 24, 2019 13:33

bkietz requested changes Oct 24, 2019
View reviewed changes

bkietz closed this Oct 24, 2019

bkietz reopened this Oct 24, 2019

xuechendi closed this Oct 30, 2019

xuechendi reopened this Oct 30, 2019

bkietz requested changes Oct 31, 2019
View reviewed changes

xuechendi force-pushed the wip_upstream_filesystem branch from 17cea20 to dd317d7Compare November 1, 2019 01:31

[C++]Add filesystem_utils_test and hdfs_test
64983b8
Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi force-pushed the wip_upstream_filesystem branch from dd317d7 to 9fe3b0dCompare November 6, 2019 05:09

[C++]Add Filesystem_utils and HadoopFileSystem
ff5aa70
1. Make MakeFileSystem as a namespace function 2. Use uri.h to parse path string Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi force-pushed the wip_upstream_filesystem branch from 9fe3b0d to ff5aa70Compare November 6, 2019 05:29

xuechendi closed this Nov 6, 2019

xuechendi reopened this Nov 6, 2019

pitrou requested changes Nov 7, 2019
View reviewed changes

[C++] issue fixing according to review comments from@pitrou
282ec95
Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi force-pushed the wip_upstream_filesystem branch from fdb1937 to 282ec95Compare November 12, 2019 01:33

wesm closed this Nov 21, 2019

asfimport mentioned this pull request Nov 26, 2024
[JAVA][C++]Support Parquet Read and Write in Java apache/arrow-java#279
Open

	enumstructFileSystemType { HDFS, LOCAL, S3 };
	enumclassFileSystemType { HDFS, LOCAL, S3 };

Uh oh!

Conversation

xuechendi commented Oct 23, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Oct 23, 2019

Uh oh!

emkornfield commented Oct 24, 2019

Uh oh!

bkietz commented Oct 24, 2019

Uh oh!

xuechendi commented Oct 24, 2019

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkietz commented Oct 24, 2019

Uh oh!

xuechendi commented Oct 24, 2019

Uh oh!

xuechendi commented Oct 24, 2019

Uh oh!

pitrou commented Oct 24, 2019

Uh oh!

xuechendi commented Oct 24, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuechendi commented Oct 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Oct 30, 2019

Uh oh!

xuechendi commented Oct 30, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkietzNov 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuechendi commented Oct 23, 2019•
edited
Loading

pitrou commented Oct 24, 2019•
edited
Loading

xuechendi commented Oct 24, 2019 via email •
edited
Loading

xuechendi commented Oct 25, 2019•
edited
Loading

bkietzNov 2, 2019•
edited
Loading

pitrou commented Nov 4, 2019•
edited
Loading