You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "joonaspessi (via GitHub)" <gi...@apache.org> on 2025/11/08 13:45:47 UTC
[PR] feat: st_polygonize [sedona-db]
joonaspessi opened a new pull request, #286:
URL: https://github.com/apache/sedona-db/pull/286
Implements `ST_Polygonize` aggregate function following PostGIS behavior. Creates polygons from a collection of linestrings that form closed rings using the GEOS polygonize algorithm.
This is the first GEOS-based aggregate function in the codebase and asking for opinions about the implementation as I'm quite unsure if this is the correct approach.
## How to support `geometry[]` array input?
PostGIS supports two function signatures:
```sql
-- Aggregate form (implemented)
SELECT ST_Polygonize(geom) FROM table;
-- Array form (NOT implemented)
SELECT ST_Polygonize(ARRAY[geom1, geom2, geom3]);
```
Any ideas how to implement this with DafaFusion and keep support for both input styles?
Should we separate the "array form" to another PR?
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2507541841
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
Review Comment:
Totally optional, but it might simplify our `state()` implementation (and make it more similar to `ST_Collect()`) to use `Vec<u8>` here (in other words, accumulate a geometrycollection).
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
Review Comment:
We should probably use a `GEOSWkbFactory` to create all of these (it's faster than `new_from_wkb()`)
https://github.com/apache/sedona-db/blob/main/c/sedona-geos/src/wkb_to_geos.rs#L24-L29
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = [self.input_type.clone()];
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(&arg_types, &args);
+
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ self.geometries.push(item.buf().to_vec());
Review Comment:
This is a similar idea to ST_Collect()...it may help here (or improvements you find here might be useful there!)
https://github.com/apache/sedona-db/blob/94ce7db5287b39218b803eae057a4eb92a50c397/rust/sedona-functions/src/st_collect.rs#L98-L105
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2516960993
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,457 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
Review Comment:
Thanks for the suggestion. Should be fixed now
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2508024169
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
Review Comment:
Tried to think if this would work out. Using single buffer like in `ST_Collect`, would it require then to keep track of offsets and then re-slice the buffer so that we can pass them individually `geos::Geometry::polygonize(&[geom1, geom2, ...])`. If I got it right, it feels little complicated
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2515508535
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,459 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
Review Comment:
This is OK! A heap allocation per output item is better than a heap allocation per input item 🙂
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2513135146
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,459 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
Review Comment:
Couldn't get rid of this :(
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2507029173
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = [self.input_type.clone()];
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(&arg_types, &args);
+
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ self.geometries.push(item.buf().to_vec());
Review Comment:
I dug in and found a way to avoid some of these `to_vec()` calls that copy the buffer. It's scattered across the file (and thus hard to convey here in a concise way), so I'm tried something new today and submitted a PR on your fork: https://github.com/joonaspessi/sedona-db/pull/1. Merge if it looks good to you.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #286:
URL: https://github.com/apache/sedona-db/pull/286#issuecomment-3520352420
Yes, I am aware. I plan to change them back to `agg`...
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2507015053
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = [self.input_type.clone()];
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(&arg_types, &args);
Review Comment:
```suggestion
let arg_types = std::slice::from_ref(&self.input_type);
let args = [ColumnarValue::Array(values[0].clone())];
let executor = sedona_functions::executor::WkbExecutor::new(arg_types, &args);
```
We can avoid this clone.
##########
python/sedonadb/tests/functions/test_aggregate.py:
##########
@@ -115,3 +115,63 @@ def test_st_collect_zero_input(eng):
) AS t(geom) WHERE false""",
None,
)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_basic_triangle(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (10 0, 10 10)'),
+ ('LINESTRING (10 10, 0 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 10 10, 10 0)))",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_with_nulls(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ (NULL),
+ ('LINESTRING (10 0, 10 10)'),
+ (NULL),
+ ('LINESTRING (10 10, 0 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 10 10, 10 0)))",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_no_polygons_formed(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (20 0, 30 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION EMPTY",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_multiple_polygons(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (10 0, 5 10)'),
+ ('LINESTRING (5 10, 0 0)'),
+ ('LINESTRING (20 0, 30 0)'),
+ ('LINESTRING (30 0, 25 10)'),
+ ('LINESTRING (25 10, 20 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 5 10, 10 0)), POLYGON ((30 0, 20 0, 25 10, 30 0)))",
+ )
Review Comment:
I think we should also add these cases too.
```sql
# a single polygon
select st_astext(st_polygonize(array[st_geomfromtext('POLYGON ((10 0, 0 0, 10 10, 10 0))')]));
-- GEOMETRYCOLLECTION(POLYGON((10 0,0 0,10 10,10 0)))
one of each of the other geom types (e.g points, multipoint, etc) and linestring empty
-- GEOMETRYCOLLECTION EMPTY
# A single linestring that's really a LinearRing
select st_astext(st_polygonize(array[st_geomfromtext('linestring(0 0, 0 1, 1 1, 1 0, 0 0)')]));
-- GEOMETRYCOLLECTION(POLYGON((0 0,0 1,1 1,1 0,0 0)))
```
These are using Postgis array syntax. Since we don't have array syntax yet, we could do something like this to avoid having to write a bunch of those polygon:
```python
@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
@pytest.mark.parametrize(
("geom", "expected"),
def test_st_polygonize_single_geom(eng, geom, expected): # feel free to come up with a better name
eng = eng.create_or_skip()
eng.assert_query_result(
f"""SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
VALUES
('{geom}')
) AS t(geom)""",
expected,
```
Optional: It would be nice to have a few of these cases on the Rust level. If someone makes changes during development, it's nice to have `cargo test` catch buggy changes rather than waiting to occasionally `pytest`.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "yutannihilation (via GitHub)" <gi...@apache.org>.
yutannihilation commented on PR #286:
URL: https://github.com/apache/sedona-db/pull/286#issuecomment-3520186122
Sorry to nitpick. I think Sedona uses `_Aggr` instead of `_Agg` (although I feel `_Agg` is more familiar...)
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2515009837
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,457 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
Review Comment:
```suggestion
return sedona_internal_err!("No input arrays provided to accumulator in update_batch");
```
nit: We try to avoid using `Datafusion::Internal` because it throws a misleading error message. See here for more detail: https://github.com/apache/sedona-db/pull/2. This is a handy macro we typically use instead.
Here's the import `use sedona_common::sedona_internal_err;`
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,457 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = std::slice::from_ref(&self.input_type);
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(arg_types, &args);
+
+ let item_ref = self.item.as_mut().unwrap();
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ item_ref.extend_from_slice(item.buf());
+ self.count += 1;
+ }
+ Ok(())
+ })?;
+
+ Ok(())
+ }
+
+ fn evaluate(&mut self) -> Result<ScalarValue> {
+ let wkb = self.make_wkb_result()?;
+ Ok(ScalarValue::Binary(wkb))
+ }
+
+ fn size(&self) -> usize {
+ let item_capacity = self.item.as_ref().map(|e| e.capacity()).unwrap_or(0);
+ size_of::<PolygonizeAccumulator>() + item_capacity
+ }
+
+ fn state(&mut self) -> Result<Vec<ScalarValue>> {
+ let serialized_count = ScalarValue::UInt64(Some(self.count as u64));
+ let serialized_item = ScalarValue::Binary(self.item.take());
+
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+ self.item = Some(item);
+ self.count = 0;
+
+ Ok(vec![serialized_count, serialized_item])
+ }
+
+ fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> {
+ if states.len() != 2 {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected number of state fields for st_polygonize() (expected 2, got {})",
+ states.len()
+ )));
+ }
+
+ let item_ref = self.item.as_mut().ok_or_else(|| {
+ DataFusionError::Internal("Unexpected internal state in ST_Polygonize()".to_string())
Review Comment:
```suggestion
sedona_internal_err!("Unexpected internal state in ST_Polygonize()")
```
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,457 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{cast::AsArray, types::UInt64Type, Array, ArrayRef};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geo_traits::Dimensions;
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_geometry::wkb_factory::write_wkb_geometrycollection_header;
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::read_wkb;
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![
+ Arc::new(Field::new("count", DataType::UInt64, false)),
+ Arc::new(Field::new("item", DataType::Binary, true)),
+ ])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ item: Option<Vec<u8>>,
+ count: usize,
+}
+
+const WKB_HEADER_SIZE: usize = 1 + 4 + 4;
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+
+ Self {
+ input_type,
+ item: Some(item),
+ count: 0,
+ }
+ }
+
+ fn make_wkb_result(&mut self) -> Result<Option<Vec<u8>>> {
+ if self.count == 0 {
+ return Ok(None);
+ }
+
+ let collection_wkb = self.item.as_mut().unwrap();
+ let mut header = Vec::new();
+ write_wkb_geometrycollection_header(&mut header, Dimensions::Xy, self.count)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to write header: {e}")))?;
+ collection_wkb[0..WKB_HEADER_SIZE].copy_from_slice(&header);
+
+ let wkb = read_wkb(collection_wkb)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to read WKB: {e}")))?;
+
+ let factory = GEOSWkbFactory::new();
+ let collection = factory.create(&wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })?;
+
+ let num_geoms = collection.get_num_geometries().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get number of geometries: {e}"))
+ })?;
+
+ let mut geos_geoms = Vec::with_capacity(num_geoms);
+ for i in 0..num_geoms {
+ let geom = collection.get_geometry_n(i).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to get geometry {}: {e}", i))
+ })?;
+ // Clone is necessary: get_geometry_n() returns ConstGeometry<'_> which doesn't
+ // implement Borrow<Geometry>. The GEOS polygonize() function signature requires
+ // T: Borrow<Geometry>, so we must clone to get owned Geometry instances.
+ geos_geoms.push(geom.clone());
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = std::slice::from_ref(&self.input_type);
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(arg_types, &args);
+
+ let item_ref = self.item.as_mut().unwrap();
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ item_ref.extend_from_slice(item.buf());
+ self.count += 1;
+ }
+ Ok(())
+ })?;
+
+ Ok(())
+ }
+
+ fn evaluate(&mut self) -> Result<ScalarValue> {
+ let wkb = self.make_wkb_result()?;
+ Ok(ScalarValue::Binary(wkb))
+ }
+
+ fn size(&self) -> usize {
+ let item_capacity = self.item.as_ref().map(|e| e.capacity()).unwrap_or(0);
+ size_of::<PolygonizeAccumulator>() + item_capacity
+ }
+
+ fn state(&mut self) -> Result<Vec<ScalarValue>> {
+ let serialized_count = ScalarValue::UInt64(Some(self.count as u64));
+ let serialized_item = ScalarValue::Binary(self.item.take());
+
+ let mut item = Vec::new();
+ write_wkb_geometrycollection_header(&mut item, Dimensions::Xy, 0)
+ .expect("Failed to write initial GeometryCollection header");
+ self.item = Some(item);
+ self.count = 0;
+
+ Ok(vec![serialized_count, serialized_item])
+ }
+
+ fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> {
+ if states.len() != 2 {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected number of state fields for st_polygonize() (expected 2, got {})",
+ states.len()
+ )));
Review Comment:
```suggestion
return sedona_internal_err!(
"Unexpected number of state fields for st_polygonize() (expected 2, got {:?})",
states.len()
);
```
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2507632934
##########
python/sedonadb/tests/functions/test_aggregate.py:
##########
@@ -115,3 +115,63 @@ def test_st_collect_zero_input(eng):
) AS t(geom) WHERE false""",
None,
)
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_basic_triangle(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (10 0, 10 10)'),
+ ('LINESTRING (10 10, 0 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 10 10, 10 0)))",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_with_nulls(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ (NULL),
+ ('LINESTRING (10 0, 10 10)'),
+ (NULL),
+ ('LINESTRING (10 10, 0 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 10 10, 10 0)))",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_no_polygons_formed(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (20 0, 30 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION EMPTY",
+ )
+
+
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+def test_st_polygonize_multiple_polygons(eng):
+ eng = eng.create_or_skip()
+ eng.assert_query_result(
+ """SELECT ST_Polygonize(ST_GeomFromText(geom)) FROM (
+ VALUES
+ ('LINESTRING (0 0, 10 0)'),
+ ('LINESTRING (10 0, 5 10)'),
+ ('LINESTRING (5 10, 0 0)'),
+ ('LINESTRING (20 0, 30 0)'),
+ ('LINESTRING (30 0, 25 10)'),
+ ('LINESTRING (25 10, 20 0)')
+ ) AS t(geom)""",
+ "GEOMETRYCOLLECTION (POLYGON ((10 0, 0 0, 5 10, 10 0)), POLYGON ((30 0, 20 0, 25 10, 30 0)))",
+ )
Review Comment:
Thanks for the idea of having test cases for single geometries. Tests are now updated for both rust and python suites.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2508022475
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = [self.input_type.clone()];
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(&arg_types, &args);
+
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ self.geometries.push(item.buf().to_vec());
Review Comment:
Thanks for creating the PR, merged it into the branch and I hope that most of the unnecessary copy operations are now gone.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2507029173
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb in &self.geometries {
+ let geom = geos::Geometry::new_from_wkb(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert WKB to GEOS: {e}"))
+ })?;
+ geos_geoms.push(geom);
+ }
+
+ let result = geos::Geometry::polygonize(&geos_geoms)
+ .map_err(|e| DataFusionError::Execution(format!("Failed to polygonize: {e}")))?;
+
+ let wkb = result.to_wkb().map_err(|e| {
+ DataFusionError::Execution(format!("Failed to convert result to WKB: {e}"))
+ })?;
+
+ Ok(Some(wkb.into()))
+ }
+}
+
+impl Accumulator for PolygonizeAccumulator {
+ fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
+ if values.is_empty() {
+ return Err(DataFusionError::Internal(
+ "No input arrays provided to accumulator in update_batch".to_string(),
+ ));
+ }
+
+ let arg_types = [self.input_type.clone()];
+ let args = [ColumnarValue::Array(values[0].clone())];
+ let executor = sedona_functions::executor::WkbExecutor::new(&arg_types, &args);
+
+ executor.execute_wkb_void(|maybe_item| {
+ if let Some(item) = maybe_item {
+ self.geometries.push(item.buf().to_vec());
Review Comment:
I dug in and found a way to avoid some of these `to_vec()` calls that copy the buffer. It's scattered across the file (and thus hard to convey here in a concise way), so I'm tried something new today and submitted a PR on your fork: https://github.com/joonaspessi/sedona-db/pull/1. Merge if it looks good to you, and it should reflect here in your PR.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2511752765
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,439 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{
+ builder::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::{read_wkb, Wkb};
+
+thread_local! {
+ static GEOS_WKB_FACTORY: GEOSWkbFactory = GEOSWkbFactory::new();
+}
+
+fn wkb_to_geos_geometry(wkb: &Wkb) -> Result<geos::Geometry> {
+ GEOS_WKB_FACTORY.with(|factory| {
+ factory.create(wkb).map_err(|e| {
+ DataFusionError::Execution(format!("Failed to create geometry from WKB: {e}"))
+ })
+ })
+}
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Arc<[u8]>>,
+}
+
+impl PolygonizeAccumulator {
+ pub fn new(input_type: SedonaType) -> Self {
+ Self {
+ input_type,
+ geometries: Vec::new(),
+ }
+ }
+
+ fn make_wkb_result(&self) -> Result<Option<Vec<u8>>> {
+ if self.geometries.is_empty() {
+ return Ok(None);
+ }
+
+ let mut geos_geoms = Vec::with_capacity(self.geometries.len());
+ for wkb_bytes in &self.geometries {
Review Comment:
Right before this loop is where you could declare the function-level `GEOSWkbFactory`. If we run into performance issues with this function we can reevlauate whether a thread local is appropriate.
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,439 @@
+// 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 crate::wkb_to_geos::GEOSWkbFactory;
+use arrow_array::{
+ builder::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+use wkb::reader::{read_wkb, Wkb};
+
+thread_local! {
+ static GEOS_WKB_FACTORY: GEOSWkbFactory = GEOSWkbFactory::new();
+}
Review Comment:
I don't think you need a thread local factory...we do that in at least one performance-critical place in SedonaDB but here I think it is sufficient to declare it once as a function-level variable before you loop over WKB geometries.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #286:
URL: https://github.com/apache/sedona-db/pull/286
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #286:
URL: https://github.com/apache/sedona-db/pull/286#issuecomment-3518558034
We should rename this to `ST_Polygonize_Agg`. Sedona, Snowflake, Databricks all use `_agg` to separate their Spatial SQL aggregate functions from scalar functions.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "yutannihilation (via GitHub)" <gi...@apache.org>.
yutannihilation commented on PR #286:
URL: https://github.com/apache/sedona-db/pull/286#issuecomment-3520458394
Oh, cool. Good to know!
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: st_polygonize [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #286:
URL: https://github.com/apache/sedona-db/pull/286#discussion_r2511798982
##########
c/sedona-geos/src/st_polygonize.rs:
##########
@@ -0,0 +1,311 @@
+// 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::OffsetBufferBuilder, cast::as_list_array, Array, ArrayRef, BinaryArray,
+};
+use arrow_schema::{DataType, Field, FieldRef};
+use datafusion_common::{cast::as_binary_array, error::Result, DataFusionError, ScalarValue};
+use datafusion_expr::{Accumulator, ColumnarValue};
+use geos::Geom;
+use sedona_expr::aggregate_udf::{SedonaAccumulator, SedonaAccumulatorRef};
+use sedona_schema::{
+ datatypes::{SedonaType, WKB_GEOMETRY},
+ matchers::ArgMatcher,
+};
+
+/// ST_Polygonize() aggregate implementation using GEOS
+pub fn st_polygonize_impl() -> SedonaAccumulatorRef {
+ Arc::new(STPolygonize {})
+}
+
+#[derive(Debug)]
+struct STPolygonize {}
+
+impl SedonaAccumulator for STPolygonize {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(vec![ArgMatcher::is_geometry()], WKB_GEOMETRY);
+ matcher.match_args(args)
+ }
+
+ fn accumulator(
+ &self,
+ args: &[SedonaType],
+ _output_type: &SedonaType,
+ ) -> Result<Box<dyn Accumulator>> {
+ Ok(Box::new(PolygonizeAccumulator::new(args[0].clone())))
+ }
+
+ fn state_fields(&self, _args: &[SedonaType]) -> Result<Vec<FieldRef>> {
+ Ok(vec![Arc::new(Field::new(
+ "geometries",
+ DataType::List(Arc::new(Field::new("item", DataType::Binary, true))),
+ false,
+ ))])
+ }
+}
+
+#[derive(Debug)]
+struct PolygonizeAccumulator {
+ input_type: SedonaType,
+ geometries: Vec<Vec<u8>>,
Review Comment:
You'd have to write it as a valid GEOMETRYCOLLECTION (like ST_Collect()), use GEOS to parse the geometrycollection (once), then pass its children to `polygonize()`. It should be faster for a few reasons (e.g., a `Vec<Vec<u8>>` will perform heap allocations for each element, whereas `Vec<u8>` can reuse the same heap allocation for multiple elements). Optional to handle that here but it would be nice as a precedent for future aggregate function writers.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/sedona-geos): Add ST_Polygonize() aggregate version [sedona-db]
Posted by "joonaspessi (via GitHub)" <gi...@apache.org>.
joonaspessi commented on PR #286:
URL: https://github.com/apache/sedona-db/pull/286#issuecomment-3520182301
@jiayuasu As suggested, renamed to `ST_Polygonize_Agg`. Maybe this makes most sense for both user and implementation perspective.
--
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-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org