ARROW-5494: [Python] Create FileSystem bindings by kszucs · Pull Request #5258 · apache/arrow

kszucs · 2019-09-03T10:39:47Z

FileSystem binding without S3 (it will be implemented in a followup PR)
Moved arrow/python/util/datetime.h one level upper, enabling to be used by the bindings

kszucs · 2019-09-04T13:24:22Z

@pitrou apply ARROW_PYTHON_EXPORT for all?

Well, the functions which were inlined should be kept inline IMO.

Then I need to move all of the implementation to the header, including the static functions (practically the whole implementation).

That's how it was already, right?

If we keep everything as internal than that could work, although I prefer readable header files.
Actually we should benchmark whether it has a performance impact or not.
Could you recommend me specific benchmarks if we have any?

We don't seem to have any datetime-heavy benchmarks in the ASV benchmarks directory.
However, you can probably devise a timeit-based microbenchmark easily. No need to be sophisticated :-)

kszucs · 2019-09-05T16:54:41Z

@wesm how can I properly set the visibility for https://github.com/apache/arrow/pull/5258/files#diff-6de8e16eea663cbb2694f9e66e71bd75R55 ?

wesm · 2019-09-05T22:37:39Z

@kszucs it's a typedef... visibility only has to do with exported compiled symbols

kszucs · 2019-09-06T07:54:43Z

Ahh, indeed! Thanks!

kszucs · 2019-09-06T14:10:24Z

@pitrou I have a "not a subpath error" which is probably windows related? https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/27224120/job/dx9qmxdj5tnenu94#L2057

pitrou · 2019-09-09T13:44:54Z

@pitrou I have a "not a subpath error" which is probably windows related?

Right now the filesystem classes expect forward slashes, and it seems you're passing backslashes. You should try pathlib.Path.as_posix.

pitrou

Thanks for doing this. It's gonna be very useful. I posted some comments below.

pitrou · 2019-09-09T13:48:35Z

We don't seem to have any datetime-heavy benchmarks in the ASV benchmarks directory.
However, you can probably devise a timeit-based microbenchmark easily. No need to be sophisticated :-)

pitrou · 2019-09-11T13:34:38Z

Looks like the AppVeyor failure needs fixing, can you take a look @kszucs ?

jorisvandenbossche

I quickly tried this out, and added a few comments. But looks good!

jorisvandenbossche · 2019-09-11T15:08:35Z

For the pathlib comment, the other alternative would also be to add it as a dependency (for python 2 only, just like also enum34 is a py2 only dependency for enum backport).
I would say that if we think it is nice to return a pathlib, we shouldn't preclude ourselves doing that just for the few months of py2 support we still have.

kszucs · 2019-09-11T15:16:07Z

After transitioning to py3 only it would be reasonable to use pathlib.Path objects as return types indeed (at least that's my preference). I suggest to create a JIRA where we can discuss further (possible involves other parts of pyarrow), meanwhile I've set the return types to string.

pitrou · 2019-09-11T16:39:31Z

After transitioning to py3 only it would be reasonable to use pathlib.Path objects as return types indeed (at least that's my preference).

You mean from get_target_stats?

pitrou · 2019-09-11T17:07:28Z

it would be reasonable to use pathlib.Path objects as return types indeed (at least that's my preference)

Actually, no. pathlib paths are only for the local filesystem, not for arbitrary resources (e.g. S3). Instead you should return a plain string.

jorisvandenbossche · 2019-09-11T18:47:47Z

Actually, no. pathlib paths are only for the local filesystem, not for arbitrary resources (e.g. S3). Instead you should return a plain string.

Ah, that solves the python 2 / should we depend on pathlib question then.

kszucs · 2019-09-11T18:48:15Z

We could have a pathlib compatible Path implementation as well. Whatever, the API we're providing is not stable yet.

pitrou · 2019-09-16T13:57:18Z

For now I'm not doing a full review again, feel free to ping me when you think you're ready (and CI is fixed :-)).

kszucs · 2019-09-17T10:57:00Z

@pitrou this should be green now. I'd like to move forward with bindings for S3 once it is merged.

pitrou

Just two more comments, sorry ;-)

pitrou · 2019-09-17T11:04:01Z

+ since the C++ side then decodes from utf-8. On Unix, os.fsencode may be
+ better.
+ """
+ return tobytes(_stringify_path(path))


I still think this shouldn't accept Path objects, only str and bytes. Path objects are only valid for local paths, but here we are dealing with paths in arbitrary systems.

@pitrou done, actually we don't accept bytes neither, only strings (we're discussing it previously)

kszucs · 2019-09-17T15:15:58Z

@pitrou done, builds are green, the travis error is about codecov upload failure.

pitrou · 2019-09-17T16:14:00Z

This is a very nice start, thank you :-)