feat: Add GEOS library as an optional dependency #12243

wraymo · 2025-02-03T21:03:17Z

Summary
This PR introduces GEOS as an optional dependency, marking the first step in splitting #12053 to enable geospatial support.

netlify · 2025-02-03T21:03:37Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`34c141a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67c0a194d2d23000087fa526
😎 Deploy Preview	https://deploy-preview-12243--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

czentgr

Thanks for this.

czentgr · 2025-02-05T16:18:43Z

CMakeLists.txt

@@ -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)


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.

Thanks for the review, updated the code

kgpai · 2025-02-05T20:35:50Z

scripts/setup-centos9.sh

@@ -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


Can the geos versions be pinned ?

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?

Pinning to some top level like 3.10.1 is good enough :)

kgpai · 2025-02-05T20:35:56Z

CMakeLists.txt

@@ -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)


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.

kgpai

LGTM ;
Would be great if you can pin the versions. Thanks !

jagill · 2025-02-06T18:39:21Z

scripts/setup-centos9.sh

@@ -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


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?

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.

We've had problems in the past with dependencies coming from brew being the latest version (fmt was one of those).
In my opinion, any velox dependency should come from a predefined version and not let the OS pick which one to use. For xsimd and simdjson we should install it into the system as well. Not sure why we rely on the bundling. Likely because these are header only dependencies so it doesn't really matter.

Thanks @czentgr. That makes sense. Having a pre-defined version ensures the same behaviour for different platforms. But if we install from different package managers, it might still end up with different versions.

Do you think it's a good idea to make it bundled by default and remove it from system dependency, which ensures all environments use the same version, reducing potential compatibility issues.

Or do you prefer a hybrid approach - installing the default version from the package manager and keeping a bundled version? Users can switch to the bundled version if there's an error.

We have been using the hybrid approach in that most dependencies are system dependencies but also offer the bundling of the same versions. Where the OS doesn't give the desired version we build them from scratch via the install methods. So we can pin a version and install it from the scripts and through bundling and be consistent everywhere. Basically not relying on the OS especially because the OS package versions can lag behind quite a bit.

Updated the code

czentgr · 2025-02-18T18:49:11Z

scripts/setup-centos9.sh

@@ -235,6 +237,14 @@ function install_cuda {
  dnf install -y cuda-nvcc-$dashed cuda-cudart-devel-$dashed cuda-nvrtc-devel-$dashed cuda-driver-devel-$dashed
 }

+function install_geos {
+  if $BUILD_GEOS ; then


I think this needs to improve to be sure we compare correctly

if [ "$BUILD_GEOS" = true ]; then

See https://stackoverflow.com/questions/2953646/how-can-i-declare-and-use-boolean-variables-in-a-shell-script which has a nice explanation why this isn't sufficient and is actually bad (notably the shell command execution).

Good point, I just follow the way to install duckdb (including this buggy test command and echo). Probably we need to change it as well in another PR.

czentgr · 2025-02-18T18:50:56Z

scripts/setup-centos9.sh

@@ -235,6 +237,14 @@ function install_cuda {
  dnf install -y cuda-nvcc-$dashed cuda-cudart-devel-$dashed cuda-nvrtc-devel-$dashed cuda-driver-devel-$dashed
 }

+function install_geos {
+  if $BUILD_GEOS ; then
+    echo 'Building GEOS'


We don't echo things for other dependencies. This script runs with -x and so each command is shown.

czentgr · 2025-02-20T17:00:53Z

@wraymo Please rebase as well (the signature fuzzer should be fixed).
Interesting, can you check on why the geos is found in the system (even though it is not available in the dependency image) and not set to use the BUNDLED version? I don't think it is part of the dependency image because it later can't find the cmake files for it that should be installed into the system if present.

wraymo · 2025-02-20T18:03:33Z

I'm not sure if there's a way to rebuild both the adapter and CentOS images. Since the adapter image is based on the CentOS image, and the CentOS image's Dockerfile runs setup-centos.sh, which installs GEOS by default, the image should already have GEOS installed.

However, I suspect that the current CI/CD pipeline is using a cached image, which might explain why GEOS is missing. Additionally, because VELOX_DEPENDENCY_SOURCE is set to SYSTEM for Linux release with adapters (unless explicitly overridden for certain dependencies), it does not use the bundled version of GEOS.

To resolve this, we likely need to either:

Rebuild the CentOS and adapter images to ensure GEOS is included.
Explicitly set geos_SOURCE: BUNDLED to use the bundled version instead.

What do you think? @czentgr

czentgr · 2025-02-20T18:26:32Z

@wraymo The containers are not used during this PR build that would contain the geos library. You could build new dependency images and push then to github and then use them in the PR for a test. They will get rebuild after this PR is merged. But it doesn't matter, for this PR it should use the bundling but it is not.
You could try to build it in the container that the CI pipeline is using to figure out what the env is like.

Edit: Ahh yes, in the build it might set it to SYSTEM. Yes, please add the explicit override in the CI for this PR and set it to bundled.
Once the new dependency images are added this can be changed again.

wraymo · 2025-02-20T19:07:06Z

@czentgr Done. Could you rerun the pipeline?

assignUser · 2025-02-22T12:44:04Z

LGTM not sure why the tests or signature checks are failing as you didn't touch anything relevant. Maybe geos cmake is setting variables that leak into the main build? (yeah probably this: https://github.com/libgeos/geos/blob/0fdc40af1fb125b59efc3d154c672314beff4249/CMakeLists.txt#L123)

The easiest way to fix this is to unset it but there might be more vars being set. Moving it into a subdir like folly and boost will protect through dir scope.

czentgr · 2025-02-24T14:36:16Z

I think the signature changes issue got resolved. Lets rebase one more time and re-run.

wraymo · 2025-02-24T16:10:59Z

just rebased

wraymo · 2025-02-25T04:12:45Z

Not sure what causes the test failure, but when I build the adapter image locally and compile Velox with bundled GEOS, the tests run successfully.

wraymo · 2025-02-27T04:47:35Z

@czentgr @assignUser Do you have any insights into the failed test cases? They pass on my local setup.

assignUser · 2025-02-27T12:01:10Z

@wraymo I assume you have geos installed locally? Try to use the bundled version, that will likely make the error show up. If so I assume it's this issue

wraymo · 2025-02-27T19:56:00Z

@assignUser I was using the bundled version without installing the system dependency. I’ve now added unset in geos.cmake, and all tests pass successfully.

wraymo · 2025-02-28T16:02:36Z

@assignUser Could you take a look at the changes?

wraymo requested review from assignUser and majetideepak as code owners February 3, 2025 21:03

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 3, 2025

wraymo mentioned this pull request Feb 4, 2025

feat: Add spatial support #12053

Draft

czentgr reviewed Feb 5, 2025

View reviewed changes

wraymo force-pushed the geos_dep branch from 9c9acf6 to 81c926d Compare February 5, 2025 17:37

wraymo requested a review from czentgr February 5, 2025 17:41

kgpai reviewed Feb 5, 2025

View reviewed changes

wraymo force-pushed the geos_dep branch from 81c926d to ceb8bd5 Compare February 5, 2025 22:13

kgpai approved these changes Feb 6, 2025

View reviewed changes

wraymo force-pushed the geos_dep branch 2 times, most recently from efff04f to 1660caa Compare February 6, 2025 17:17

jagill reviewed Feb 6, 2025

View reviewed changes

wraymo mentioned this pull request Feb 11, 2025

feat: Add Geometry Presto type #12274

Open

wraymo force-pushed the geos_dep branch 2 times, most recently from 35e8ee0 to eaf4b64 Compare February 13, 2025 03:42

czentgr requested changes Feb 18, 2025

View reviewed changes

wraymo force-pushed the geos_dep branch from eaf4b64 to 8342bb3 Compare February 18, 2025 22:08

wraymo requested a review from czentgr February 18, 2025 22:08

wraymo force-pushed the geos_dep branch from 8342bb3 to 3f13512 Compare February 18, 2025 22:17

czentgr approved these changes Feb 20, 2025

View reviewed changes

wraymo force-pushed the geos_dep branch from 3f13512 to e6ef65d Compare February 20, 2025 18:06

wraymo force-pushed the geos_dep branch from e6ef65d to 92e87e6 Compare February 20, 2025 19:03

wraymo force-pushed the geos_dep branch from 92e87e6 to 24a73ea Compare February 24, 2025 16:10

wraymo force-pushed the geos_dep branch from 24a73ea to 550bba4 Compare February 26, 2025 20:41

wraymo force-pushed the geos_dep branch from 550bba4 to 7b9d42c Compare February 27, 2025 16:12

add geos library as an optional dependency

34c141a

wraymo force-pushed the geos_dep branch from 7b9d42c to 34c141a Compare February 27, 2025 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add GEOS library as an optional dependency #12243

feat: Add GEOS library as an optional dependency #12243

wraymo commented Feb 3, 2025

netlify bot commented Feb 3, 2025 •

edited

Loading

czentgr left a comment

czentgr Feb 5, 2025

wraymo Feb 5, 2025 •

edited

Loading

kgpai Feb 5, 2025

wraymo Feb 5, 2025

kgpai Feb 6, 2025

kgpai Feb 5, 2025

wraymo Feb 5, 2025

kgpai left a comment

jagill Feb 6, 2025

wraymo Feb 6, 2025 •

edited

Loading

czentgr Feb 10, 2025

wraymo Feb 11, 2025 •

edited

Loading

czentgr Feb 12, 2025

wraymo Feb 13, 2025

czentgr Feb 18, 2025 •

edited

Loading

wraymo Feb 18, 2025 •

edited

Loading

czentgr Feb 18, 2025

czentgr commented Feb 20, 2025

wraymo commented Feb 20, 2025

czentgr commented Feb 20, 2025 •

edited

Loading

wraymo commented Feb 20, 2025

assignUser commented Feb 22, 2025 •

edited

Loading

czentgr commented Feb 24, 2025

wraymo commented Feb 24, 2025

wraymo commented Feb 25, 2025

wraymo commented Feb 27, 2025

assignUser commented Feb 27, 2025

wraymo commented Feb 27, 2025 •

edited

Loading

wraymo commented Feb 28, 2025

feat: Add GEOS library as an optional dependency #12243

Are you sure you want to change the base?

feat: Add GEOS library as an optional dependency #12243

Conversation

wraymo commented Feb 3, 2025

netlify bot commented Feb 3, 2025 • edited Loading

✅ Deploy Preview for meta-velox ready!

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraymo Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraymo Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraymo Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

wraymo Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr commented Feb 20, 2025

wraymo commented Feb 20, 2025

czentgr commented Feb 20, 2025 • edited Loading

wraymo commented Feb 20, 2025

assignUser commented Feb 22, 2025 • edited Loading

czentgr commented Feb 24, 2025

wraymo commented Feb 24, 2025

wraymo commented Feb 25, 2025

wraymo commented Feb 27, 2025

assignUser commented Feb 27, 2025

wraymo commented Feb 27, 2025 • edited Loading

wraymo commented Feb 28, 2025

netlify bot commented Feb 3, 2025 •

edited

Loading

wraymo Feb 5, 2025 •

edited

Loading

wraymo Feb 6, 2025 •

edited

Loading

wraymo Feb 11, 2025 •

edited

Loading

czentgr Feb 18, 2025 •

edited

Loading

wraymo Feb 18, 2025 •

edited

Loading

czentgr commented Feb 20, 2025 •

edited

Loading

assignUser commented Feb 22, 2025 •

edited

Loading

wraymo commented Feb 27, 2025 •

edited

Loading