petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463159374
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
Review Comment:
Just providing more motivation on *why* multiple kernels are preferred.
If we discover the `geo` implementation of a basic `st_buffer(geom, dist)`
(no extra params) is faster (), we could support that kernel for
`st_buffer(geom, dist)`, while simultaneously supporting the geos kernel that
you're adding here.
In the code below, we register all of the functions from different
libraries. If a kernel exists for two libraries, the earlier one will be
overwritten. e.g `geo` overwrites `geos` at the moment.
https://github.com/apache/sedona-db/blob/b8ac5efbe358ebb9344cf9177496447e233d2253/rust/sedona/src/context.rs#L128-L134
If it turns out the `geo` implementation is faster than `geos`
([issue](https://github.com/apache/sedona-db/issues/232)), adding these new
params as a separate kernel allows us to keep a `geo` kernel for
`st_buffer(geom, dist)` while also keeping a `geos` kernel for `st_geom(geom,
dist, params)`. `geo` [doesn't seem to
support](https://github.com/georust/geo/blob/7263968eb533028fd9be81eaeb9ca455e8a2aee6/geo/src/algorithm/buffer.rs#L141-L145)
all of the params either way, so this `geos` implementation will be helpful
for us regardless.
idk off the top of my head if things are hooked up so it would work that
way, but that's the idea anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
