Should Geospatial features be gated behind a compiler flag?

[jagill]

In #12094, we proposed a roadmap to add geospatial types and functions into Velox. Geospatial functions in Presto-Java are typically used by a small percentage of users, but those users depend on it. Geospatial functions require dependencies to do the heavy algorithmic lifting. GEOS is the core library that does the computation, but to support spherical/earth-based coordinates, we'll likely require either GDAL or Proj. All of these are relatively heavyweight libraries, and GDAL in particular is challenging to build.

Given that engines which use Velox may not require geospatial types or functions, they may not want to have to include these libraries. We could gate their inclusion with a compiler flag (currently VELOX_ENABLE_GEO), which allows people to not include these but also adds build complexity. If we choose to gate, we could choose to gate all geospatial capabilities, or only those that require the library. For example, Bing tile functions mostly do not require the dependent libraries. If we choose to gate, we should also make sure that we gate any spatial joins behind the flag. I'd like some input on three possibilities:

1. Do not gate: Everyone who wants to use Velox will have to include the dependencies, and have access to geospatial capabilities.
2. Gate all geo: Gate all geospatial functions behind the flag.
3. Gate only functions requiring the libraries: The Geometry type, BingTile type, and most BingTile functions would always be available, but almost all Geometry functions and a couple BingTile functions will not be available if VELOX_ENABLE_GEO is not set.

[jagill]

cc @wraymo @aditi-pandit @pedroerp @czentgr @majetideepak @jaystarshot @amitkdutta @bikramSingh91 @Sullivan-Patrick

[czentgr]

@jagill Thanks for this write up.
I'm leaning towards the third option of gating only functions that require the library. The only question is then how end-users will understand what is and isn't supported in a given build. That being said, I would think that the OSS versioned Prestissimo/PrestoC++ containers would always be enabled for full Geometry support. So far there haven't been different types of containers based on the supported features.
For Velox there isn't an official release so it (yet) so users would have to build their own version and hence know what is and isn't supported. So perhaps the question of where the line is isn't that great.
IMO, we shouldn't artificially restrict features if we don't have to.

[wraymo]

@jagill Thanks for following up! Personally, I prefer the third option—that’s also the approach I took in the serialization PR.

[pedroerp]

Thanks for starting this discussion. I'm ok with either (2) or (3).

I have a slight preference for (2) as it could a bit unintuitive for users to see geo functions when VELOX_ENABLE_GEO is not set, but if it's just a couple of small compilation units it should also be fine.

[aditi-pandit]

Thanks for this discussion.

Gating Geospatial features behind a flag is fine.

From a Presto stand-point we could do the equivalent of what the Presto Java features would be +/- the presto-geospatial-toolkit module. That is all geo-spatial features I think.

I'm not strongly opposed to 3, but think it would be tough to explain the subset of functionality to the users. The code organization may require some thinking as well... we would have to separate the registration functions, PlanNode to operator conversions. This doesn't sound very appealing to me. Its simpler to have docs and users understand that its one complete set of functionality or nothing.

[majetideepak]

I prefer 2 as well. It gets confusing if some of the functionality is missing.