Add support for Arrow Flight package installation#11588
Add support for Arrow Flight package installation#11588Rijin-N wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
|
Hi @Rijin-N! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
✅ Deploy Preview for meta-velox canceled.
|
|
@Rijin-N If the connector is being added to Prestissimo, why can't we add these dependencies there? |
| run_and_time install_fbthrift | ||
| run_and_time install_duckdb | ||
| run_and_time install_stemmer | ||
| run_and_time install_abseil |
There was a problem hiding this comment.
I think these are not direct dependencies and should be called from install_arrow to indicate these are only needed by Arrow - and only if BUILD_ARROW_FLIGHT was turned on.
There was a problem hiding this comment.
The install_grpc function has been moved inside the install_arrow function. Other functions were not moved, as they are also required for the GCS connector.
| # Install gcs dependencies | ||
| # https://github.com/googleapis/google-cloud-cpp/blob/main/doc/packaging.md#required-libraries | ||
|
|
||
| # abseil-cpp |
There was a problem hiding this comment.
This is also called for macOS - for development using the GCS connector. So this can't be removed because it is also a dependency for that.
scripts/setup-centos9.sh
Outdated
| export CFLAGS=${CXXFLAGS//"-std=c++17"/} # Used by LZO. | ||
| CMAKE_BUILD_TYPE="${BUILD_TYPE:-Release}" | ||
| BUILD_DUCKDB="${BUILD_DUCKDB:-true}" | ||
| BUILD_ARROW_FLIGHT="${BUILD_ARROW_FLIGHT:-OFF}" |
There was a problem hiding this comment.
The value should be true/false (follow DUCKDB). How is this going to work from a prestissimo point of view? When prestissimo is running the setup script it needs to have this env var set.
I'm not sure adding there here is the correct way. ARROW_FLIGHT is a Prestissimo only feature.
This would fit better into the Prestissimo setup-adapters.sh as it is conditional.
The could then rebuild Arrow with the additional dependencies of grpc, abseil etc.
There was a problem hiding this comment.
Addressed the first part of this comment. The second part will be addressed after receiving a response to this comment #11588 (comment)
scripts/setup-ubuntu.sh
Outdated
| cd ${DEPENDENCY_DIR}/protobuf | ||
| ./configure --prefix=${INSTALL_PREFIX} | ||
| make "-j${NPROC}" | ||
| make install |
There was a problem hiding this comment.
This and the next line commands will need ${SUDO}.
scripts/setup-ubuntu.sh
Outdated
| libfl-dev \ | ||
| tzdata | ||
| tzdata \ | ||
| pkg-config |
There was a problem hiding this comment.
This is already added on line 95 and part of the build config. It is not a direct build dependency.
There was a problem hiding this comment.
Removed pkg-config. It was not present in some older versions of Velox, which is why I had added it here.
@majetideepak @czentgr Since the Arrow dependencies are already installed in the velox setup scripts and if we want to arrow flight support we need to add it there. If you can suggest a better way to accommodate this in Prestissimo, we can make the changes. |
Rijin-N
force-pushed
the
arrow-flight
branch
from
7eea59b to
5bd299f
Compare
|
@Rijin-N Arrow is modular. We can move all of this to the corresponding Prestissimo install scripts and install only arrow flight dependencies (ArrowFlight_LIBRARIES in the Presto PR) there. |
|
As suggested by @majetideepak, we have moved the Arrow Flight package installation logic to the Prestissimo install scripts. Hence, we are closing this PR. |
5 participants
Added an option to install the Arrow Flight package, required for adding Prestissimo support for Arrow Flight connectors (PR: prestodb/presto#24082).