paleolimbot commented on code in PR #33:
URL: https://github.com/apache/sedona-db/pull/33#discussion_r2326121945
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -493,18 +493,19 @@ def test_st_isempty(eng, geom, expected):
("geom", "expected"),
[
(None, None),
- ("POINT EMPTY", 0),
+ # ("POINT EMPTY", 0), # Not supported by geo implementation - throws
ArrowInvalid error
Review Comment:
We still have to handle these cases in the implementations (in this case,
before they get passed to `to_geo()`).
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -493,18 +493,19 @@ def test_st_isempty(eng, geom, expected):
("geom", "expected"),
[
(None, None),
- ("POINT EMPTY", 0),
+ # ("POINT EMPTY", 0), # Not supported by geo implementation - throws
ArrowInvalid error
("LINESTRING EMPTY", 0),
("POINT (0 0)", 0),
("LINESTRING (0 0, 0 1)", 1),
("MULTIPOINT ((0 0), (1 1))", 0),
("MULTILINESTRING ((0 0, 1 1), (1 1, 2 2))", 2.8284271247461903),
- # Polygons contribute 0 because perimeters aren't included in the
length calculation
- ("POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))", 0),
- ("MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)), ((0 0, 1 0, 1 1, 0 1, 0
0)))", 0),
+ # Geo implementation calculates polygon perimeters as length (differs
from OGC standard)
+ # This provides more comprehensive length calculation including
polygon boundaries
+ ("POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))", 4.0),
+ ("MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)), ((0 0, 1 0, 1 1, 0 1, 0
0)))", 8.0),
Review Comment:
We still need to these to implemented correctly (this is the challenge with
using the geo library for these functions)
##########
rust/sedona-geo/src/st_centroid.rs:
##########
@@ -0,0 +1,155 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::Arc;
+
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::error::Result;
+use datafusion_expr::ColumnarValue;
+use sedona_expr::scalar_udf::{ArgMatcher, ScalarKernelRef, SedonaScalarKernel};
+use sedona_functions::executor::WkbExecutor;
+use sedona_schema::datatypes::{SedonaType, WKB_GEOMETRY};
+use wkb::reader::Wkb;
+
+use crate::centroid::extract_centroid_2d;
+
+/// ST_Centroid() implementation using centroid extraction
+pub fn st_centroid_impl() -> ScalarKernelRef {
+ Arc::new(STCentroid {})
+}
+
+#[derive(Debug)]
+struct STCentroid {}
+
+impl SedonaScalarKernel for STCentroid {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = WkbExecutor::new(arg_types, args);
+ let mut builder =
BinaryBuilder::with_capacity(executor.num_iterations(), 1024);
+ executor.execute_wkb_void(|maybe_wkb| {
+ match maybe_wkb {
+ Some(wkb) => {
+ let centroid_wkb = invoke_scalar(&wkb)?;
+ builder.append_value(¢roid_wkb);
+ }
+ _ => builder.append_null(),
+ }
+
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+ }
+}
+
+fn invoke_scalar(wkb: &Wkb) -> Result<Vec<u8>> {
+ let (x, y) = extract_centroid_2d(wkb)?;
+
+ // Create WKB for POINT geometry
+ if x.is_nan() || y.is_nan() {
+ // Return POINT EMPTY
+ Ok(create_empty_point_wkb())
+ } else {
+ Ok(create_point_wkb(x, y))
+ }
+}
+
+fn create_point_wkb(x: f64, y: f64) -> Vec<u8> {
+ let mut wkb = Vec::with_capacity(21);
+ // Little endian
+ wkb.push(0x01);
+ // Point geometry type (1)
+ wkb.extend_from_slice(&1u32.to_le_bytes());
+ // X coordinate
+ wkb.extend_from_slice(&x.to_le_bytes());
+ // Y coordinate
+ wkb.extend_from_slice(&y.to_le_bytes());
+ wkb
+}
Review Comment:
I believe there's a factory function for this in sedona-geometry
##########
rust/sedona-geo/src/st_centroid.rs:
##########
@@ -0,0 +1,155 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::Arc;
+
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::error::Result;
+use datafusion_expr::ColumnarValue;
+use sedona_expr::scalar_udf::{ArgMatcher, ScalarKernelRef, SedonaScalarKernel};
+use sedona_functions::executor::WkbExecutor;
+use sedona_schema::datatypes::{SedonaType, WKB_GEOMETRY};
+use wkb::reader::Wkb;
+
+use crate::centroid::extract_centroid_2d;
+
+/// ST_Centroid() implementation using centroid extraction
+pub fn st_centroid_impl() -> ScalarKernelRef {
+ Arc::new(STCentroid {})
+}
+
+#[derive(Debug)]
+struct STCentroid {}
+
+impl SedonaScalarKernel for STCentroid {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = WkbExecutor::new(arg_types, args);
+ let mut builder =
BinaryBuilder::with_capacity(executor.num_iterations(), 1024);
+ executor.execute_wkb_void(|maybe_wkb| {
+ match maybe_wkb {
+ Some(wkb) => {
+ let centroid_wkb = invoke_scalar(&wkb)?;
+ builder.append_value(¢roid_wkb);
+ }
+ _ => builder.append_null(),
+ }
+
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+ }
+}
+
+fn invoke_scalar(wkb: &Wkb) -> Result<Vec<u8>> {
+ let (x, y) = extract_centroid_2d(wkb)?;
+
+ // Create WKB for POINT geometry
+ if x.is_nan() || y.is_nan() {
+ // Return POINT EMPTY
+ Ok(create_empty_point_wkb())
+ } else {
+ Ok(create_point_wkb(x, y))
+ }
+}
+
+fn create_point_wkb(x: f64, y: f64) -> Vec<u8> {
+ let mut wkb = Vec::with_capacity(21);
+ // Little endian
+ wkb.push(0x01);
+ // Point geometry type (1)
+ wkb.extend_from_slice(&1u32.to_le_bytes());
+ // X coordinate
+ wkb.extend_from_slice(&x.to_le_bytes());
+ // Y coordinate
+ wkb.extend_from_slice(&y.to_le_bytes());
+ wkb
+}
+
+fn create_empty_point_wkb() -> Vec<u8> {
+ let mut wkb = Vec::with_capacity(21);
+ // Little endian
+ wkb.push(0x01);
+ // Point geometry type (1)
+ wkb.extend_from_slice(&1u32.to_le_bytes());
+ // NaN coordinates for empty point
+ wkb.extend_from_slice(&f64::NAN.to_le_bytes());
+ wkb.extend_from_slice(&f64::NAN.to_le_bytes());
+ wkb
+}
Review Comment:
We have a file sedona-testing/src/fixtures.rs where you can put the rendered
version of this!
##########
.gitignore:
##########
@@ -36,3 +36,7 @@ coverage/
# documentation
site/
+
+# Python cache files
+__pycache__/
+benchmarks/__pycache__/
Review Comment:
nit: I think this works:
```suggestion
__pycache__
```
##########
rust/sedona-geo/src/st_centroid.rs:
##########
@@ -0,0 +1,155 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::Arc;
+
+use arrow_array::builder::BinaryBuilder;
+use datafusion_common::error::Result;
+use datafusion_expr::ColumnarValue;
+use sedona_expr::scalar_udf::{ArgMatcher, ScalarKernelRef, SedonaScalarKernel};
+use sedona_functions::executor::WkbExecutor;
+use sedona_schema::datatypes::{SedonaType, WKB_GEOMETRY};
+use wkb::reader::Wkb;
+
+use crate::centroid::extract_centroid_2d;
+
+/// ST_Centroid() implementation using centroid extraction
+pub fn st_centroid_impl() -> ScalarKernelRef {
+ Arc::new(STCentroid {})
+}
+
+#[derive(Debug)]
+struct STCentroid {}
+
+impl SedonaScalarKernel for STCentroid {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()],
WKB_GEOMETRY);
+
+ matcher.match_args(args)
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let executor = WkbExecutor::new(arg_types, args);
+ let mut builder =
BinaryBuilder::with_capacity(executor.num_iterations(), 1024);
+ executor.execute_wkb_void(|maybe_wkb| {
+ match maybe_wkb {
+ Some(wkb) => {
+ let centroid_wkb = invoke_scalar(&wkb)?;
+ builder.append_value(¢roid_wkb);
+ }
+ _ => builder.append_null(),
+ }
+
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+ }
+}
+
+fn invoke_scalar(wkb: &Wkb) -> Result<Vec<u8>> {
+ let (x, y) = extract_centroid_2d(wkb)?;
Review Comment:
One of the challeneges with centroid in particular is behaviour in 3D (the Z
and/or M values is interpolated and possibly weighted by the length of the
segment if I remember correctly, and I have no idea what happens for a Polygon).
--
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: issues-unsubscr...@sedona.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
