feat: Add GEOS library as an optional dependency #12243
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
CMakeLists.txt
Outdated
| @@ -138,6 +138,7 @@ option(VELOX_ENABLE_ABFS "Build Abfs Connector" OFF) | |||
| option(VELOX_ENABLE_HDFS "Build Hdfs Connector" OFF) | |||
| option(VELOX_ENABLE_PARQUET "Enable Parquet support" ON) | |||
| option(VELOX_ENABLE_ARROW "Enable Arrow support" OFF) | |||
| option(VELOX_ENABLE_GEO "Enable Geospatial support" ON) | |||
There was a problem hiding this comment.
We probably should turn this OFF by default for now. Otherwise, we just bundle it needlessly.
We should add this to the setup scripts to make it a system dependency.
There was a problem hiding this comment.
Thanks for the review, updated the code
scripts/setup-centos9.sh
Outdated
| @@ -78,7 +78,7 @@ function install_velox_deps_from_dnf { | |||
| dnf_install libevent-devel \ | |||
| openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \ | |||
| libdwarf-devel elfutils-libelf-devel curl-devel libicu-devel bison flex \ | |||
| libsodium-devel zlib-devel | |||
| libsodium-devel zlib-devel geos-devel | |||
There was a problem hiding this comment.
Can the geos versions be pinned ?
There was a problem hiding this comment.
The current version is 3.10.1-1.el9 (an old version). I can replace it with geos-devel-3.10.1-1.el9. Does that work for you? Also, do you want to pin the version for the Ubuntu setup?
There was a problem hiding this comment.
Pinning to some top level like 3.10.1 is good enough :)
| @@ -138,6 +138,7 @@ option(VELOX_ENABLE_ABFS "Build Abfs Connector" OFF) | |||
| option(VELOX_ENABLE_HDFS "Build Hdfs Connector" OFF) | |||
| option(VELOX_ENABLE_PARQUET "Enable Parquet support" ON) | |||
| option(VELOX_ENABLE_ARROW "Enable Arrow support" OFF) | |||
| option(VELOX_ENABLE_GEO "Enable Geospatial support" OFF) | |||
There was a problem hiding this comment.
Can you add VELOX_ENABLE_GEO to the adapters build : https://github.com/facebookincubator/velox/blob/main/.github/workflows/linux-build-base.yml#L87 ?
If it doesnt get exercised by default, we need a signal these changes pass CI.
| @@ -78,7 +78,7 @@ function install_velox_deps_from_dnf { | |||
| dnf_install libevent-devel \ | |||
| openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \ | |||
| libdwarf-devel elfutils-libelf-devel curl-devel libicu-devel bison flex \ | |||
| libsodium-devel zlib-devel | |||
| libsodium-devel zlib-devel geos-devel-3.10.1 | |||
There was a problem hiding this comment.
Just checking (this may be my ignorance here): We are using Geos 3.13.0 above and 3.10.1 (devel) here. Is that fine or will that break things?
There was a problem hiding this comment.
Good question. It looks like only brew keeps the version up to date (3.13.0), while the latest versions available are 3.10.1 in epel and 3.12.1 in apt. Based on the release logs of GEOS, there don't seem to be any missing features relevant to our requirements, but I'm unsure about the performance differences.
One option is to remove it as a system dependency, similar to what we did for simdjson or xsimd. For example, we don't install libsimdjson-dev or simdjson-devel in the Ubuntu and CentOS setup scripts (even though they're available) but only include them in the macOS setup script. In CMakeLists.txt, we just specify the minimum version in resolve_dependency in case the user uses a non-bundled version.
Summary
This PR introduces GEOS as an optional dependency, marking the first step in splitting #12053 to enable geospatial support.