-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-6720: [C++] Add HDFS implementation to filesystem layer #5820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7effffc to
245634f
Compare
05512af to
cf6f3fe
Compare
8423502 to
9793132
Compare
|
@nealrichardson Will need your help for the R AppVeyor failure: |
|
Looks like ARROW_FILESYSTEM now requires a new dependency? Is it vendored? |
|
It's built by the Arrow build chain, but it's not in the packages used by R-Arrow ("rarow"?) on Windows. It seems to work elsewhere, including R Ubuntu here: https://github.com/apache/arrow/pull/5820/checks?check_run_id=301764483 |
|
Yes but Windows is "special". I'll dig in a bit and see if I can understand what the deal is. Alternatively, is uriparser needed for all of filesystem or just HDFS? |
|
It's needed for all of filesystem (it's needed for Flight too). |
|
🤔 there's also this message higher up: https://ci.appveyor.com/project/pitrou/arrow/builds/28838764#L1576 |
|
It says "Building uriparser from source" which is expected at this point. |
|
Ok, and on master it's not building uriparser (cf. https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/28838835/job/788gr6gydfhrdk4v?fullLog=true) Have you tried without adding Maybe it's bundled somehow and not needed? Otherwise, where does it get built to? |
|
@ursabot crossbow submit macos-r-autobrew |
Yes, it fails with a link error: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/28836148/job/o95eh8ft96n3n0pl#L3348 |
|
AMD64 Conda Crossbow Submit (#76833) builder has been succeeded. Revision: 9793132e05e5b674c492fe9e64f442239cd2266c Submitted crossbow builds: ursa-labs/crossbow @ ursabot-324
|
|
Ok so we need it. We're building it and that seems not to error, but it's not getting bundled with the build. That is, we're dealing with https://github.com/apache/arrow/blob/master/ci/appveyor-build-r.sh, which calls https://github.com/apache/arrow/blob/master/ci/PKGBUILD and https://github.com/apache/arrow/blob/master/ci/windows-pkg-arrow-for-r.sh. |
|
Does it bundle other dependencies such as double-conversion? |
|
double-conversion is an external dependency, so that gets downloaded and bundled here: https://github.com/apache/arrow/blob/master/ci/windows-pkg-arrow-for-r.sh#L61 |
|
If we don't want to use the vendored uriparser, we could port https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-uriparser/PKGBUILD to the |
I'm not sure what you mean by that. Personally, I have no preference :-) I just don't know how to do it. |
|
Neither do I :) cf. #5814 |
|
Locally it gets built into |
|
Looks like we have the same problem on macOS: https://travis-ci.org/ursa-labs/crossbow/builds/611598498#L831 |
|
Can I get you to handle the issue? |
|
Realistically I can’t commit to doing it before going on vacation. But it doesn’t require me, there’s no other special R knowledge needed, I think we just need to mv the built liburiparser to the right directory. |
cpp/src/arrow/filesystem/hdfs.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should take the path as parameter and return FileStats directly, would make thing more natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because it acts on an existing FileStats. I mean other members have been set by the caller.
|
@pitrou I did some exploration in the Appveyor build and I think I see how to grab the liburiparser that we build; however, it seems that we build a .dll but for R we need a static library. I see there is a uriparser cmake flag |
|
@nealrichardson that flag is already set to off when building uriparser; it really looks like we're trying to always build it static |
|
And yet I see liburiparser-1.dll is what is made on Appveyor: https://ci.appveyor.com/project/nealrichardson/arrow/builds/28875888?fullLog=true#L1177 Locally I do see liburiparser.a though. |
|
Maybe there's another flag needed somewhere for mingw? https://stackoverflow.com/a/42979732/11897522 |
9793132 to
5ff9750
Compare
|
there was a merge conflict due to ARROW-6633. I've rebased |
5ff9750 to
d7c80e7
Compare
387a4ea to
3b1b8a5
Compare
|
I've rebased for the uriparser vendoring, this should fix the R build on Windows. |
bkietz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is green, I'll merge this now
Based on initial work by @xuechendi.