You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "Abeeujah (via GitHub)" <gi...@apache.org> on 2025/10/24 17:09:56 UTC
[PR] Plumb remaining parameters for ST_Buffer [sedona-db]
Abeeujah opened a new pull request, #241:
URL: https://github.com/apache/sedona-db/pull/241
closes #223
- `geos` doesn't have a method that computes `buffer` with `useSpheroid` so I didn't implement that yet
- The `testers.rs` isn't really clear to me how to proceed with four args so I haven't been thorough with the unit tests
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2470568862
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
These test cases fail because of the following as identified
**GEOS Version Incompatibility:**
- PostGIS uses GEOS 3.9.0 (includes geometric artifacts in single-sided buffers)
- SedonaDB uses GEOS 3.13.1 (artifacts removed - more geometrically correct)
**Key Finding**
In GEOS 3.12.0 (May 2023), major improvements were made to single-sided buffers:
- [Commit 091f6d99](https://github.com/libgeos/geos/commit/091f6d99): "Only keep biggest polygon from single-sided buffer"
- This removes spurious artifacts that were incorrectly included in buffer results
- **Result:** GEOS 3.13 produces smaller, more accurate areas than GEOS 3.9
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462389751
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -32,6 +32,19 @@ use sedona_schema::{
use crate::executor::GeosExecutor;
/// ST_Buffer() implementation using the geos crate
+///
+/// Supports three signatures:
+/// - ST_Buffer(geometry: Geometry, distance: Double)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean, bufferStyleParameters: String)
Review Comment:
Yes, I think we want PostGIS signatures for now. PostGIS and Sedona Spark implement the spheroidal buffer by reprojecting, doing a buffer, then projecting back, but PostGIS does this as `ST_Buffer(<GEOGRAPHY>)` instead of geometry + useSpheroid.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2473696486
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -592,4 +606,93 @@ mod tests {
assert!(result_buffer.equals_exact(&expected_buffer, 0.1).unwrap());
}
+
+ #[test]
+ fn test_side_right_geos_3_13() {
+ let wkt = "LINESTRING(50 50, 150 150, 150 50)";
+ let line = geos::Geometry::new_from_wkt(wkt).unwrap();
+ let distance = 100.0;
+
+ // Test single-sided buffer (GEOS 3.13+ removes artifacts, giving 12713.61)
+ // PostGIS with GEOS 3.9 returns 16285.08 due to including geometric artifacts
+ // GEOS 3.12+ improvements: https://github.com/libgeos/geos/commit/091f6d99
+ let params_single = BufferParams::builder().single_sided(true).build().unwrap();
+
+ let buffer_right = line.buffer_with_params(-distance, ¶ms_single).unwrap();
+ let area_right = buffer_right.area().unwrap();
+
+ // Expected area with GEOS 3.13 (improved algorithm without artifacts)
+ assert!(
+ (area_right - 12713.605978550266).abs() < 0.1,
+ "Expected GEOS 3.13+ area ~12713.61, got {}",
+ area_right
+ );
+ }
+
+ #[test]
+ fn test_empty_and_invalid_input() {
+ assert_eq!(
+ parse_buffer_side_style(None),
+ (false, false),
+ "Should return (false, false) for None."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("")),
+ (false, false),
+ "Should return (false, false) for an empty string."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("mitre_limit=5.0")),
+ (false, false),
+ "Should return (false, false) for an invalid key."
+ );
+ }
+
+ #[test]
+ fn test_single_side_and_case_insensitivity() {
+ assert_eq!(
+ parse_buffer_side_style(Some("side=left")),
+ (true, false),
+ "Should detect 'left'."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("side=RIGHT")),
+ (false, true),
+ "Should detect 'RIGHT' case-insensitively."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("SiDe=LeFt")),
+ (true, false),
+ "Should handle mixed case key and value."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("join=mitre SIDE=RIGHT mitre_limit=5.0")),
+ (false, true),
+ "Should ignore other params and detect 'RIGHT'."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("side=center")),
+ (false, false),
+ "Should ignore invalid side values."
+ );
+ }
+
+ #[test]
+ fn test_both_sides_present() {
+ assert_eq!(
+ parse_buffer_side_style(Some("side=left side=right")),
Review Comment:
Thank 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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2467845022
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -163,4 +302,198 @@ mod tests {
let envelope_result = envelope_tester.invoke_array(buffer_result).unwrap();
assert_array_equal(&envelope_result, &expected_envelope);
}
+
+ #[rstest]
+ fn udf_with_buffer_params(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ // Envelope checks result in different values for different GEOS versions.
+ // This test at least ensures that the buffer parameters are plugged in.
+ let buffer_result_flat = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 2.0, "endcap=flat".to_string())
+ .unwrap();
+
+ let buffer_result_square = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 1.0, "endcap=square".to_string())
+ .unwrap();
+
+ assert_ne!(buffer_result_flat, buffer_result_square);
Review Comment:
I pushed this to try to get CI passing since I wasn't sure myself exactly how to do it without trying a few things...I think this is OK given the integration tests. Buffer is just very hard one to test.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3454568738
don't merge yet
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2470570732
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,191 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
+ (Some(wkb), Some(distance)) => {
+ builder.append_value(invoke_scalar(&wkb, distance, ¶ms)?);
+ }
+ _ => builder.append_null(),
+ }
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+}
+
fn invoke_scalar(
geos_geom: &geos::Geometry,
distance: f64,
params: &BufferParams,
- writer: &mut impl std::io::Write,
-) -> Result<()> {
+) -> Result<Vec<u8>> {
let geometry = geos_geom
.buffer_with_params(distance, params)
.map_err(|e| DataFusionError::External(Box::new(e)))?;
let wkb = geometry
.to_wkb()
.map_err(|e| DataFusionError::Execution(format!("Failed to convert to wkb: {e}")))?;
- writer.write_all(wkb.as_ref())?;
- Ok(())
+ Ok(wkb.into())
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {arg:?}",
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {param}",
+ )));
+ };
+
+ if key.eq_ignore_ascii_case("endcap") {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ } else if key.eq_ignore_ascii_case("join") {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ } else if key.eq_ignore_ascii_case("side") {
+ params_builder = params_builder.single_sided(is_single_sided(value)?);
Review Comment:
This has now been added
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2473531575
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -592,4 +606,93 @@ mod tests {
assert!(result_buffer.equals_exact(&expected_buffer, 0.1).unwrap());
}
+
+ #[test]
+ fn test_side_right_geos_3_13() {
+ let wkt = "LINESTRING(50 50, 150 150, 150 50)";
+ let line = geos::Geometry::new_from_wkt(wkt).unwrap();
+ let distance = 100.0;
+
+ // Test single-sided buffer (GEOS 3.13+ removes artifacts, giving 12713.61)
+ // PostGIS with GEOS 3.9 returns 16285.08 due to including geometric artifacts
+ // GEOS 3.12+ improvements: https://github.com/libgeos/geos/commit/091f6d99
+ let params_single = BufferParams::builder().single_sided(true).build().unwrap();
+
+ let buffer_right = line.buffer_with_params(-distance, ¶ms_single).unwrap();
+ let area_right = buffer_right.area().unwrap();
+
+ // Expected area with GEOS 3.13 (improved algorithm without artifacts)
+ assert!(
+ (area_right - 12713.605978550266).abs() < 0.1,
+ "Expected GEOS 3.13+ area ~12713.61, got {}",
+ area_right
+ );
+ }
+
+ #[test]
+ fn test_empty_and_invalid_input() {
+ assert_eq!(
+ parse_buffer_side_style(None),
+ (false, false),
+ "Should return (false, false) for None."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("")),
+ (false, false),
+ "Should return (false, false) for an empty string."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("mitre_limit=5.0")),
+ (false, false),
+ "Should return (false, false) for an invalid key."
+ );
+ }
+
+ #[test]
+ fn test_single_side_and_case_insensitivity() {
+ assert_eq!(
+ parse_buffer_side_style(Some("side=left")),
+ (true, false),
+ "Should detect 'left'."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("side=RIGHT")),
+ (false, true),
+ "Should detect 'RIGHT' case-insensitively."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("SiDe=LeFt")),
+ (true, false),
+ "Should handle mixed case key and value."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("join=mitre SIDE=RIGHT mitre_limit=5.0")),
+ (false, true),
+ "Should ignore other params and detect 'RIGHT'."
+ );
+ assert_eq!(
+ parse_buffer_side_style(Some("side=center")),
+ (false, false),
+ "Should ignore invalid side values."
+ );
+ }
+
+ #[test]
+ fn test_both_sides_present() {
+ assert_eq!(
+ parse_buffer_side_style(Some("side=left side=right")),
Review Comment:
nice test case!
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463155943
##########
c/sedona-geos/src/register.rs:
##########
@@ -37,6 +44,7 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
vec, but when I tried changing the string, both kernels work simultaneously as desired. 🤷
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2465866103
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -163,4 +313,211 @@ mod tests {
let envelope_result = envelope_tester.invoke_array(buffer_result).unwrap();
assert_array_equal(&envelope_result, &expected_envelope);
}
+
+ #[rstest]
+ fn udf_with_buffer_params(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+
+ let buffer_result = tester
+ .invoke_scalar_scalar_scalar(
+ "LINESTRING (0 0, 10 0)",
+ 2.0,
+ "endcap=flat join=mitre quad_segs=2".to_string(),
+ )
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result).unwrap();
+ let expected_envelope = "POLYGON((0 -2, 0 2, 10 2, 10 -2, 0 -2))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+
+ let buffer_result = tester
+ .invoke_scalar_scalar_scalar(
+ "LINESTRING (0 0, 10 0)",
+ 1.0,
+ "endcap=square join=bevel".to_string(),
+ )
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result).unwrap();
+ let expected_envelope = "POLYGON((-1 -1, -1 1, 11 1, 11 -1, -1 -1))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+ }
+
+ #[rstest]
+ fn udf_with_quad_segs(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+ let input_wkt = "POINT (5 5)";
+ let buffer_dist = 3.0;
+
+ let buffer_result_default = tester
+ .invoke_scalar_scalar_scalar(input_wkt, buffer_dist, "endcap=round".to_string())
+ .unwrap();
+ let envelope_result_default = envelope_tester
+ .invoke_scalar(buffer_result_default)
+ .unwrap();
+
+ let expected_envelope = "POLYGON((2 2, 2 8, 8 8, 8 2, 2 2))";
+ tester.assert_scalar_result_equals(envelope_result_default, expected_envelope);
+
+ let buffer_result_low_segs = tester
+ .invoke_scalar_scalar_scalar(
+ input_wkt,
+ buffer_dist,
+ "quad_segs=1 endcap=round".to_string(),
+ )
+ .unwrap();
+ let envelope_result_low_segs = envelope_tester
+ .invoke_scalar(buffer_result_low_segs)
+ .unwrap();
+ tester.assert_scalar_result_equals(envelope_result_low_segs, expected_envelope);
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_endcap() {
+ let err = parse_buffer_params(Some("endcap=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'invalid'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_join() {
+ let err = parse_buffer_params(Some("join=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_side() {
+ let err = parse_buffer_params(Some("side=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid side: 'invalid'. Valid options: both, left, right"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mitre_limit() {
+ let err = parse_buffer_params(Some("mitre_limit=not_a_number"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'not_a_number'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_miter_limit() {
+ let err = parse_buffer_params(Some("miter_limit=abc")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'abc'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quad_segs() {
+ let err = parse_buffer_params(Some("quad_segs=not_an_int"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'not_an_int'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quadrant_segments() {
+ let err = parse_buffer_params(Some("quadrant_segments=xyz"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'xyz'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_multiple_invalid_params() {
+ // Test that the first invalid parameter is caught
+ let err = parse_buffer_params(Some("endcap=wrong join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'wrong'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mixed_with_valid() {
+ // Test invalid parameter after valid ones
+ let err = parse_buffer_params(Some("endcap=round join=invalid"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_param_name() {
+ let err = parse_buffer_params(Some("unknown_param=value"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid buffer parameter: unknown_param (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_missing_value() {
+ let err = parse_buffer_params(Some("endcap=round bare_param join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Missing value for buffer parameter: bare_param"
+ );
+ }
Review Comment:
Thank you for testing all of these!
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2471687603
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,216 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
);
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ let is_single_sided = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("side="))
+ .unwrap_or(false);
+
+ let is_left = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("left"))
+ .unwrap_or(false);
+
+ let is_right = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("right"))
+ .unwrap_or(false);
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
+ (Some(wkb), Some(mut distance)) => {
+ if is_single_sided && ((is_left && distance < 0.0) || (is_right && distance > 0.0))
Review Comment:
```suggestion
if (is_left && distance < 0.0) || (is_right && distance > 0.0)
```
`is_single_sided` here is unnecessary since it's guaranteed to be true if `is_left` or `is_right` here is true.
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
Wow. You did an amazing job investigating. Glad to here you figured out what the other mystery was. I see [here](https://hub.docker.com/r/postgis/postgis) that there are indeed Docker images that support `geos` 13. I gave `postgis/postgis:18-3.6` a try locally, and the tests pass. It's the only one that falls under the ([recommended debian section](https://hub.docker.com/r/postgis/postgis#debian-based-recommended)). How about you just update it [here](https://github.com/apache/sedona-db/blob/0551583a675da1cf5cc61cb7c7a61352051f1fc6/compose.yml#L20).
```
image: postgis/postgis:18-3.6
```
WDYT @paleolimbot, is this a reasonable change? It ensures our `geos` version matches the container's. They say "This Docker-PostGIS image has a cautious release cycle to guarantee high stability", so it seems safe to use to me.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,216 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
);
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ let is_single_sided = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("side="))
+ .unwrap_or(false);
+
Review Comment:
```suggestion
```
This condition for `is_single_sided` isn't quite right. If we put `side=both`, this would incorrectly return true. Looking a few lines below. There's only one place where this variable is used, and it's unnecessary (the rest of the condition guarantees correctness for us), so let's remove this variable.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,216 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
);
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ let is_single_sided = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("side="))
+ .unwrap_or(false);
+
+ let is_left = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("left"))
+ .unwrap_or(false);
+
+ let is_right = buffer_style_params
+ .as_ref()
+ .map(|s| s.eq_ignore_ascii_case("right"))
+ .unwrap_or(false);
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
+ (Some(wkb), Some(mut distance)) => {
+ if is_single_sided && ((is_left && distance < 0.0) || (is_right && distance > 0.0))
+ {
+ distance = -distance;
+ }
+ builder.append_value(invoke_scalar(&wkb, distance, ¶ms)?);
+ }
+ _ => builder.append_null(),
+ }
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+}
+
fn invoke_scalar(
geos_geom: &geos::Geometry,
distance: f64,
params: &BufferParams,
- writer: &mut impl std::io::Write,
-) -> Result<()> {
+) -> Result<Vec<u8>> {
let geometry = geos_geom
.buffer_with_params(distance, params)
.map_err(|e| DataFusionError::External(Box::new(e)))?;
let wkb = geometry
.to_wkb()
.map_err(|e| DataFusionError::Execution(format!("Failed to convert to wkb: {e}")))?;
- writer.write_all(wkb.as_ref())?;
- Ok(())
Review Comment:
While it looks odd, we actually want to keep the pattern here where we write the bytes directly into the `writer` buffer instead of returning it from the function. It's faster this way. We follow this pattern for any function that returns a geometry object. See my other comment above in the file to see the other half of this to reverse. I originally did the same thing when I first wrote this function, but dewey caught me.
Here's what the caller code should look like. I'm referencing the existing code (before this PR) here.
https://github.com/apache/sedona-db/blob/0551583a675da1cf5cc61cb7c7a61352051f1fc6/c/sedona-geos/src/st_buffer.rs#L88-L89
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462420329
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
+ }
+
+ let mut matchers = vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()];
- matcher.match_args(args)
+ if args.len() >= 3 {
+ matchers.push(ArgMatcher::is_boolean());
+ }
+ if args.len() == 4 {
+ matchers.push(ArgMatcher::is_string());
+ }
+
+ ArgMatcher::new(matchers, WKB_GEOMETRY).match_args(args)
}
fn invoke_batch(
&self,
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ // Extract Args
+ let distance: Option<f64> = extract_optional_f64(&args[1])?;
Review Comment:
For the distance parameter, I think that it is worth the effort to allow this to be an array (and I have confidence in your ability to do so 🙂 ). The pattern we use for this in other places is something like:
```rust
let distance_value = args[1].cast_to(&DataType::Float64)?.to_array(executor.num_iterations());
let distance_array = if let ColumnarValue::Array(array_ref) = distance_value {
as_float64_array(array_ref)?
} else {
return sedona_internal_err!("Unexpected scalar value");
};
let mut distance_iter = distance_array.iter();
// ...
executor.execute_wkb_void(|wkb| {
match (wkb, distance_iter.next().unwrap() {
(Some(wkb_item), Some(distance_item)) => { invoke_scalar(...) }
_ => { /* append null */ }
}
})
```
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
Review Comment:
The kernel system is really intended have multiple `SedonaScalarKernel`s that handle the matching of the various numbers/types of arguments. For example:
```rust
struct STBuffer{};
impl SedonaScalarKernel for STBufferStyle {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
let matcher = ArgMatcher::new(
vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
WKB_GEOMETRY,
);
matcher.match_args(args)
}
fn invoke_batch(...) { invoke_batch_impl(...) }
}
struct STBufferStyle{};
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
let matcher = ArgMatcher::new(
vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric(), ArgMatcher::is_string()],
WKB_GEOMETRY,
);
matcher.match_args(args)
}
fn invoke_batch(...) { invoke_batch_impl(...) }
}
fn invoke_batch_impl(...) { ... }
```
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +124,136 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_f64(arg: &ColumnarValue) -> Result<Option<f64>> {
+ let casted = arg.cast_to(&DataType::Float64, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(f64::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar distance, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_bool(arg: Option<&ColumnarValue>) -> Result<Option<bool>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Boolean, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(bool::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar useSpheroid parameter, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
Review Comment:
Given the complexity here, I think it is worth writing dedicated tests for this function below, including the error messages. (In many places we do `let err = parse_buffer_params(...).unwrap_err(); assert_eq!(err.message(), "...")`).
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463156963
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -163,4 +300,46 @@ mod tests {
let envelope_result = envelope_tester.invoke_array(buffer_result).unwrap();
assert_array_equal(&envelope_result, &expected_envelope);
}
+
+ #[rstest]
+ fn udf_with_buffer_params(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer_style", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let buffer_result = tester
+ .invoke(vec![
+ ColumnarValue::Scalar(ScalarValue::Binary(Some(sedona_testing::create::make_wkb(
+ "LINESTRING (0 0, 10 0)",
+ )))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1.0))),
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(
+ "endcap=flat join=mitre quad_segs=2".to_string(),
+ ))),
+ ])
+ .unwrap();
+
+ assert!(matches!(buffer_result, ColumnarValue::Scalar(_)));
Review Comment:
I think the envelope tester (like the one used in the above example) would work here to give a reasonable "is it plugged in" check. I think the Python integration tests use `ST_Equals()` or maybe compare `ST_Area()` which should work here, too.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
);
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
Review Comment:
This looks good...thanks!
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463155943
##########
c/sedona-geos/src/register.rs:
##########
@@ -37,6 +44,7 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
vec, but when I tried changing the string, both kernels work simultaneously as desired. 🤷
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462978538
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +124,136 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_f64(arg: &ColumnarValue) -> Result<Option<f64>> {
+ let casted = arg.cast_to(&DataType::Float64, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(f64::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar distance, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_bool(arg: Option<&ColumnarValue>) -> Result<Option<bool>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Boolean, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(bool::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar useSpheroid parameter, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
Review Comment:
Sure, I would include tests for the 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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2465785102
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
Review Comment:
I'm the one that suggested the unwrap here...it's true that we usually prefer returning an internal error even for things we think are impossible, but here I think unwrap is OK choice because it's in a tight loop and we really did just initialize the array.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463159066
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -32,6 +32,19 @@ use sedona_schema::{
use crate::executor::GeosExecutor;
/// ST_Buffer() implementation using the geos crate
+///
+/// Supports three signatures:
+/// - ST_Buffer(geometry: Geometry, distance: Double)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean, bufferStyleParameters: String)
Review Comment:
Sorry, that's my bad 😬 . I updated the issue now. Hopefully you didn't spend too long figure out how to get `useSpheroid` to work.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3445541227
> The testers.rs isn't really clear to me how to proceed with four args so I haven't been thorough with the unit tests
Doesn't seem like this will be needed.
Let's also add lots more Python integration tests, so we can compare directly to PostGIS. Makes sense to create a new test function like `test_st_buffer_with_params` or something.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3447949345
Looking good so far to me. The biggest thing we're missing is Python integration tests. In case you write these up and find the current logic is flawed in *some* way (e.g we forgot/overlooked something), I recommend referencing [Sedona's parseBufferParams() function](https://github.com/apache/sedona/blob/d6ea87cf9f2a27adee00ff0fb41ee92002b9a834/common/src/main/java/org/apache/sedona/common/Functions.java#L311) and my previous [PR](https://github.com/apache/sedona/pull/2184) to fix it. I recall PostGIS's docs don't do the best job of explaining how default params are handled when certain values are specified. For example, I think we're missing [this logic](https://github.com/apache/sedona/blob/d6ea87cf9f2a27adee00ff0fb41ee92002b9a834/common/src/main/java/org/apache/sedona/common/Functions.java#L393-L402) at the moment (use python tests to check).
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2465477174
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
Review Comment:
Theoretically, it's infallible, cause there would always be an item for every iteration, the array was initialized to be the size of the executors number of iteration, that's why unwrap is called there.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2474546630
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
Thank you for investigating! We will have to remember to keep this up-to-date with PostGIS/Postgres releases but I think that is OK and helps predictability.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2467958615
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
This one test case with `side` is too simple that it's not catching the edge case behavior.
Here are 4 cases I want to add. You're welcome to modify the exact test cases, but I think we should follow the comments.
```python
# non-default side and should implicitly use square endcap
(
"LINESTRING (50 50, 150 150, 150 50)",
100,
"'side=right'",
<TODO>
),
# non-default side should implicitly use square endcap
(
"POLYGON ((50 50, 50 150, 150 150, 150 50, 50 50))",
20,
"'side=left'",
<TODO>,
),
# non-default side and non-default flat endcap should not use square endcap
# Specifying flat here doesn't actually make a difference. I found it hard to find a good test case here
(
"POLYGON ((50 50, 50 150, 150 150, 150 50, 50 50))",
20,
"'side=right endcap=flat'",
<TODO>,
),
# explicitly specifying default side=both should not set endcap=square
(
"LINESTRING (50 50, 150 150, 150 50)",
100,
"'side=both'",
<TODO>,
),
```
Here are the results I got, the current ode currently fails the first 3, while it passes the 4th. Currently, around ~2x off from the correct answer on the wrong ones.
1. Sedona: 35847.55494469865. PostGIS: 16285.07633336958
2. Sedona: 10000.0 PostGIS: 19248.578060903223
3. Sedona: 10000.0 PostGIS: 3600.0
4. Sedona: 69888.08929186598 PostGIS: 69888.089291866
After implementing the changes, if you're not careful, you might fail the 3rd or 4th cases for different reasons. Just make sure you follow the comments I left there. It's not hard, but it's easy to miss. This is the exact bug that the PR I mentioned above was fixing, so consider checking out the java implementation there.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,191 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
+ (Some(wkb), Some(distance)) => {
+ builder.append_value(invoke_scalar(&wkb, distance, ¶ms)?);
+ }
+ _ => builder.append_null(),
+ }
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+}
+
fn invoke_scalar(
geos_geom: &geos::Geometry,
distance: f64,
params: &BufferParams,
- writer: &mut impl std::io::Write,
-) -> Result<()> {
+) -> Result<Vec<u8>> {
let geometry = geos_geom
.buffer_with_params(distance, params)
.map_err(|e| DataFusionError::External(Box::new(e)))?;
let wkb = geometry
.to_wkb()
.map_err(|e| DataFusionError::Execution(format!("Failed to convert to wkb: {e}")))?;
- writer.write_all(wkb.as_ref())?;
- Ok(())
+ Ok(wkb.into())
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {arg:?}",
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {param}",
+ )));
+ };
+
+ if key.eq_ignore_ascii_case("endcap") {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ } else if key.eq_ignore_ascii_case("join") {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ } else if key.eq_ignore_ascii_case("side") {
+ params_builder = params_builder.single_sided(is_single_sided(value)?);
Review Comment:
We're still missing this logic:
https://github.com/apache/sedona/blame/d6ea87cf9f2a27adee00ff0fb41ee92002b9a834/common/src/main/java/org/apache/sedona/common/Functions.java#L399-L402
Here's a better docstring to follow because the PostGIS docs hasn't been updated yet.
- `side=both|left|right` : Defaults to `both`. Setting `left` or `right` enables a single-sided buffer operation on the geometry, with the buffered side aligned according to the direction of the line. This functionality is specific to LINESTRING geometry and has no impact on POINT or POLYGON geometries. By default, square end caps are applied when `left` or `right` are specified.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -163,4 +302,203 @@ mod tests {
let envelope_result = envelope_tester.invoke_array(buffer_result).unwrap();
assert_array_equal(&envelope_result, &expected_envelope);
}
+
+ #[rstest]
+ fn udf_with_buffer_params(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+
+ let buffer_result_flat = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 2.0, "endcap=flat".to_string())
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result_flat).unwrap();
+ let expected_envelope = "POLYGON((0 -2, 0 2, 10 2, 10 -2, 0 -2))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+
+ let buffer_result_square = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 1.0, "endcap=square".to_string())
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result_square).unwrap();
+ let expected_envelope = "POLYGON((-1 -1, -1 1, 11 1, 11 -1, -1 -1))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+ }
+
+ #[rstest]
+ fn udf_with_quad_segs(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+ let input_wkt = "POINT (5 5)";
+ let buffer_dist = 3.0;
+
+ let buffer_result_default = tester
+ .invoke_scalar_scalar_scalar(input_wkt, buffer_dist, "endcap=round".to_string())
+ .unwrap();
+ let envelope_result_default = envelope_tester
+ .invoke_scalar(buffer_result_default)
+ .unwrap();
+
+ let expected_envelope = "POLYGON((2 2, 2 8, 8 8, 8 2, 2 2))";
+ tester.assert_scalar_result_equals(envelope_result_default, expected_envelope);
+
+ let buffer_result_low_segs = tester
+ .invoke_scalar_scalar_scalar(
+ input_wkt,
+ buffer_dist,
+ "quad_segs=1 endcap=round".to_string(),
+ )
+ .unwrap();
+ let envelope_result_low_segs = envelope_tester
+ .invoke_scalar(buffer_result_low_segs)
+ .unwrap();
+ tester.assert_scalar_result_equals(envelope_result_low_segs, expected_envelope);
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_endcap() {
+ let err = parse_buffer_params(Some("endcap=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'invalid'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_join() {
+ let err = parse_buffer_params(Some("join=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_side() {
+ let err = parse_buffer_params(Some("side=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid side: 'invalid'. Valid options: both, left, right"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mitre_limit() {
+ let err = parse_buffer_params(Some("mitre_limit=not_a_number"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'not_a_number'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_miter_limit() {
+ let err = parse_buffer_params(Some("miter_limit=abc")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'abc'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quad_segs() {
+ let err = parse_buffer_params(Some("quad_segs=not_an_int"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'not_an_int'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quadrant_segments() {
+ let err = parse_buffer_params(Some("quadrant_segments=xyz"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'xyz'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_multiple_invalid_params() {
+ // Test that the first invalid parameter is caught
+ let err = parse_buffer_params(Some("endcap=wrong join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'wrong'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mixed_with_valid() {
+ // Test invalid parameter after valid ones
+ let err = parse_buffer_params(Some("endcap=round join=invalid"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_param_name() {
+ let err = parse_buffer_params(Some("unknown_param=value"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid buffer parameter: unknown_param (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_missing_value() {
+ let err = parse_buffer_params(Some("endcap=round bare_param join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Missing value for buffer parameter: bare_param"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_duplicate_params_no_error() {
+ let result = parse_buffer_params(Some("endcap=round endcap=flat"));
+ assert!(result.is_ok());
+ }
+
+ #[test]
+ fn test_parse_buffer_params_quad_segs_out_of_range() {
+ let result = parse_buffer_params(Some("quad_segs=-5"));
+ assert!(result.is_ok());
+ }
Review Comment:
It would be nice to write tests that directly check the values set for the params object (rather than testing the resulting geometries). Particularly, it would test the cases I mentioned in the Python tests. Here they are again for convenience.
```
1. non-default side and should implicitly use square endcap
2. non-default side should implicitly use square endcap
3. non-default side and non-default flat endcap should not use square endcap
4. explicitly specifying default side=both should not set endcap=square
```
I know this PR already feels long, so if you're not up for it, feel free to create a new issue for you or someone to do later. As long as the correct behavior is implemented and we have some python tests for it, I'm good to merge it.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3446991845
Hi @petern48 @paleolimbot Please guys do let me know if this is in line with the suggestions dropped, So I can proceed or course correct, The reviews has been great so far!
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2471669644
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
Wow. You did an amazing job investigating. Glad to here you figured out what the other mystery was. I see [here](https://hub.docker.com/r/postgis/postgis) that there are indeed Docker images that support `geos` 13. I gave `postgis/postgis:18-3.6` a try locally, and the tests pass. It's the only one that falls under the ([recommended debian section](https://hub.docker.com/r/postgis/postgis#debian-based-recommended)). How about you just update it [here](https://github.com/apache/sedona-db/blob/0551583a675da1cf5cc61cb7c7a61352051f1fc6/compose.yml#L20). WDYT @paleolimbot, is this a reasonable change? It ensures our `geos` version matches the container's.
```
image: postgis/postgis:18-3.6
```
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2464631356
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
Review Comment:
I wonder whether the second struct is really needed.
Could you use an optional ArgMatcher (https://github.com/apache/sedona-db/blob/8e6234c7401fc72895752558cc6aeaedd1ff470a/rust/sedona-schema/src/matchers.rs#L202) for the third parameter and keep all the logic in STBuffer ?!
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
Review Comment:
Is it safe to call `.unwrap()` here ?
I.e. do you want to panic when there is a problem or an Err could be returned and let the caller decide what to do with it?
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
+ "endcap" => {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ }
+ "join" => {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ }
+ "side" => {
+ params_builder = params_builder.single_sided(parse_side(value)?);
+ }
+ "mitre_limit" | "miter_limit" => {
+ let limit: f64 = parse_number(value, "mitre_limit")?;
+ params_builder = params_builder.mitre_limit(limit);
+ }
+ "quad_segs" | "quadrant_segments" => {
+ let segs = parse_number(value, "quadrant_segments")?;
+ params_builder = params_builder.quadrant_segments(segs);
+ }
+ _ => {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
+ key.to_lowercase()
Review Comment:
IMO it would be clearer for the developer to see the original key here instead of its lowercase version.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
+ "endcap" => {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ }
+ "join" => {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ }
+ "side" => {
+ params_builder = params_builder.single_sided(parse_side(value)?);
+ }
+ "mitre_limit" | "miter_limit" => {
+ let limit: f64 = parse_number(value, "mitre_limit")?;
+ params_builder = params_builder.mitre_limit(limit);
+ }
+ "quad_segs" | "quadrant_segments" => {
+ let segs = parse_number(value, "quadrant_segments")?;
+ params_builder = params_builder.quadrant_segments(segs);
+ }
+ _ => {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
+ key.to_lowercase()
+ )));
+ }
+ }
+ }
+
+ params_builder
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)))
+}
+
+fn parse_cap_style(value: &str) -> Result<CapStyle> {
+ match value.to_lowercase().as_str() {
+ "round" => Ok(CapStyle::Round),
+ "flat" | "butt" => Ok(CapStyle::Flat),
+ "square" => Ok(CapStyle::Square),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid endcap style: '{}'. Valid options: round, flat, butt, square",
+ value
+ ))),
+ }
+}
+
+fn parse_join_style(value: &str) -> Result<JoinStyle> {
+ match value.to_lowercase().as_str() {
Review Comment:
nit: this will cause allocations.
You may want to use eq_ignore_ascii_case() instead.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
+ "endcap" => {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ }
+ "join" => {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ }
+ "side" => {
+ params_builder = params_builder.single_sided(parse_side(value)?);
+ }
+ "mitre_limit" | "miter_limit" => {
+ let limit: f64 = parse_number(value, "mitre_limit")?;
+ params_builder = params_builder.mitre_limit(limit);
+ }
+ "quad_segs" | "quadrant_segments" => {
+ let segs = parse_number(value, "quadrant_segments")?;
+ params_builder = params_builder.quadrant_segments(segs);
+ }
+ _ => {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
+ key.to_lowercase()
+ )));
+ }
+ }
+ }
+
+ params_builder
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)))
+}
+
+fn parse_cap_style(value: &str) -> Result<CapStyle> {
+ match value.to_lowercase().as_str() {
+ "round" => Ok(CapStyle::Round),
+ "flat" | "butt" => Ok(CapStyle::Flat),
+ "square" => Ok(CapStyle::Square),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid endcap style: '{}'. Valid options: round, flat, butt, square",
+ value
+ ))),
+ }
+}
+
+fn parse_join_style(value: &str) -> Result<JoinStyle> {
+ match value.to_lowercase().as_str() {
+ "round" => Ok(JoinStyle::Round),
+ "mitre" | "miter" => Ok(JoinStyle::Mitre),
+ "bevel" => Ok(JoinStyle::Bevel),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid join style: '{}'. Valid options: round, mitre, miter, bevel",
+ value
+ ))),
+ }
+}
+
+fn parse_side(value: &str) -> Result<bool> {
Review Comment:
```suggestion
fn is_single_sided(value: &str) -> Result<bool> {
```
nit: I think `is_single_sided` would be a better name here.
`parse_side` would be more appropriate if it was returning a `Side` enum (with variants BOTH, LEFT and RIGHT) and this enum could have a method `is_single_sided`
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
+ "endcap" => {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ }
+ "join" => {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ }
+ "side" => {
+ params_builder = params_builder.single_sided(parse_side(value)?);
+ }
+ "mitre_limit" | "miter_limit" => {
+ let limit: f64 = parse_number(value, "mitre_limit")?;
+ params_builder = params_builder.mitre_limit(limit);
+ }
+ "quad_segs" | "quadrant_segments" => {
+ let segs = parse_number(value, "quadrant_segments")?;
+ params_builder = params_builder.quadrant_segments(segs);
+ }
+ _ => {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
+ key.to_lowercase()
+ )));
+ }
+ }
+ }
+
+ params_builder
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)))
+}
+
+fn parse_cap_style(value: &str) -> Result<CapStyle> {
+ match value.to_lowercase().as_str() {
Review Comment:
nit: this will cause allocations.
You may want to use eq_ignore_ascii_case() instead.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
Review Comment:
nit: this will cause allocations.
You may want to use eq_ignore_ascii_case() instead.
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +151,115 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {}",
+ param
+ )));
+ };
+
+ match key.to_lowercase().as_str() {
+ "endcap" => {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ }
+ "join" => {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ }
+ "side" => {
+ params_builder = params_builder.single_sided(parse_side(value)?);
+ }
+ "mitre_limit" | "miter_limit" => {
+ let limit: f64 = parse_number(value, "mitre_limit")?;
+ params_builder = params_builder.mitre_limit(limit);
+ }
+ "quad_segs" | "quadrant_segments" => {
+ let segs = parse_number(value, "quadrant_segments")?;
+ params_builder = params_builder.quadrant_segments(segs);
+ }
+ _ => {
+ return Err(DataFusionError::Execution(format!(
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
Review Comment:
```suggestion
"Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')",
```
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3452402800
A bit weird cause these pass locally, could be due to the append_value, and geos not being same version, I got 3.13 locally and I believe CI uses 3.14;
I just modified the invoke_scalar to return the bytes, and then write it using the binary builder can you check again?
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462977143
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -32,6 +32,19 @@ use sedona_schema::{
use crate::executor::GeosExecutor;
/// ST_Buffer() implementation using the geos crate
+///
+/// Supports three signatures:
+/// - ST_Buffer(geometry: Geometry, distance: Double)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean, bufferStyleParameters: String)
Review Comment:
I think, seeing the Sedona reference in the issue description made me think it was the more favored one, but I just updated it to use the postgis signature, please do check and let me know what you think.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463155943
##########
c/sedona-geos/src/register.rs:
##########
@@ -37,6 +44,7 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
vec, but when I tried changing the string, both kernels work simultaneously as desired. 🤷
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2471669644
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
Wow. You did an amazing job investigating. Glad to here you figured out what the other mystery was. I see [here](https://hub.docker.com/r/postgis/postgis) that there are indeed Docker images that support `geos` 13. I gave `postgis/postgis:18-3.6` a try locally, and the tests pass. It's the only one that falls under the ([recommended debian section](https://hub.docker.com/r/postgis/postgis#debian-based-recommended)). How about you just update it [here](https://github.com/apache/sedona-db/blob/0551583a675da1cf5cc61cb7c7a61352051f1fc6/compose.yml#L20).
```
image: postgis/postgis:18-3.6
```
WDYT @paleolimbot, is this a reasonable change? It ensures our `geos` version matches the container's. They say "This Docker-PostGIS image has a cautious release cycle to guarantee high stability", so it seems safe to use to me.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2470572923
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -163,4 +302,203 @@ mod tests {
let envelope_result = envelope_tester.invoke_array(buffer_result).unwrap();
assert_array_equal(&envelope_result, &expected_envelope);
}
+
+ #[rstest]
+ fn udf_with_buffer_params(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+
+ let buffer_result_flat = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 2.0, "endcap=flat".to_string())
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result_flat).unwrap();
+ let expected_envelope = "POLYGON((0 -2, 0 2, 10 2, 10 -2, 0 -2))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+
+ let buffer_result_square = tester
+ .invoke_scalar_scalar_scalar("LINESTRING (0 0, 10 0)", 1.0, "endcap=square".to_string())
+ .unwrap();
+ let envelope_result = envelope_tester.invoke_scalar(buffer_result_square).unwrap();
+ let expected_envelope = "POLYGON((-1 -1, -1 1, 11 1, 11 -1, -1 -1))";
+ tester.assert_scalar_result_equals(envelope_result, expected_envelope);
+ }
+
+ #[rstest]
+ fn udf_with_quad_segs(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType) {
+ let udf = SedonaScalarUDF::from_kernel("st_buffer", st_buffer_style_impl());
+ let tester = ScalarUdfTester::new(
+ udf.into(),
+ vec![
+ sedona_type.clone(),
+ SedonaType::Arrow(DataType::Float64),
+ SedonaType::Arrow(DataType::Utf8),
+ ],
+ );
+ tester.assert_return_type(WKB_GEOMETRY);
+
+ let envelope_udf = sedona_functions::st_envelope::st_envelope_udf();
+ let envelope_tester = ScalarUdfTester::new(envelope_udf.into(), vec![WKB_GEOMETRY]);
+ let input_wkt = "POINT (5 5)";
+ let buffer_dist = 3.0;
+
+ let buffer_result_default = tester
+ .invoke_scalar_scalar_scalar(input_wkt, buffer_dist, "endcap=round".to_string())
+ .unwrap();
+ let envelope_result_default = envelope_tester
+ .invoke_scalar(buffer_result_default)
+ .unwrap();
+
+ let expected_envelope = "POLYGON((2 2, 2 8, 8 8, 8 2, 2 2))";
+ tester.assert_scalar_result_equals(envelope_result_default, expected_envelope);
+
+ let buffer_result_low_segs = tester
+ .invoke_scalar_scalar_scalar(
+ input_wkt,
+ buffer_dist,
+ "quad_segs=1 endcap=round".to_string(),
+ )
+ .unwrap();
+ let envelope_result_low_segs = envelope_tester
+ .invoke_scalar(buffer_result_low_segs)
+ .unwrap();
+ tester.assert_scalar_result_equals(envelope_result_low_segs, expected_envelope);
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_endcap() {
+ let err = parse_buffer_params(Some("endcap=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'invalid'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_join() {
+ let err = parse_buffer_params(Some("join=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_side() {
+ let err = parse_buffer_params(Some("side=invalid")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid side: 'invalid'. Valid options: both, left, right"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mitre_limit() {
+ let err = parse_buffer_params(Some("mitre_limit=not_a_number"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'not_a_number'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_miter_limit() {
+ let err = parse_buffer_params(Some("miter_limit=abc")).err().unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid mitre_limit value: 'abc'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quad_segs() {
+ let err = parse_buffer_params(Some("quad_segs=not_an_int"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'not_an_int'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_quadrant_segments() {
+ let err = parse_buffer_params(Some("quadrant_segments=xyz"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid quadrant_segments value: 'xyz'. Expected a valid number"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_multiple_invalid_params() {
+ // Test that the first invalid parameter is caught
+ let err = parse_buffer_params(Some("endcap=wrong join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid endcap style: 'wrong'. Valid options: round, flat, butt, square"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_mixed_with_valid() {
+ // Test invalid parameter after valid ones
+ let err = parse_buffer_params(Some("endcap=round join=invalid"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid join style: 'invalid'. Valid options: round, mitre, miter, bevel"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_invalid_param_name() {
+ let err = parse_buffer_params(Some("unknown_param=value"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Invalid buffer parameter: unknown_param (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_missing_value() {
+ let err = parse_buffer_params(Some("endcap=round bare_param join=mitre"))
+ .err()
+ .unwrap();
+ assert_eq!(
+ err.message(),
+ "Missing value for buffer parameter: bare_param"
+ );
+ }
+
+ #[test]
+ fn test_parse_buffer_params_duplicate_params_no_error() {
+ let result = parse_buffer_params(Some("endcap=round endcap=flat"));
+ assert!(result.is_ok());
+ }
+
+ #[test]
+ fn test_parse_buffer_params_quad_segs_out_of_range() {
+ let result = parse_buffer_params(Some("quad_segs=-5"));
+ assert!(result.is_ok());
+ }
Review Comment:
BufferParams don't expose getter methods, nor implement traits that makes them easily testable, so I went ahead with testing behaviors instead of the values
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2467958615
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
This one test case with `side` is too simple that it's not catching the edge case behavior.
Here are 4 cases I want to add. You're welcome to modify the exact test cases, but I think we should follow the comments.
```python
# non-default side and should implicitly use square endcap
(
"LINESTRING (50 50, 150 150, 150 50)",
100,
"'side=right'",
<TODO>
),
# non-default side should implicitly use square endcap
(
"POLYGON ((50 50, 50 150, 150 150, 150 50, 50 50))",
20,
"'side=left'",
<TODO>,
),
# non-default side and non-default flat endcap should not use square endcap
# Specifying flat here doesn't actually make a difference. I found it hard to find a good test case here
(
"POLYGON ((50 50, 50 150, 150 150, 150 50, 50 50))",
20,
"'side=right endcap=flat'",
<TODO>,
),
# explicitly specifying default side=both should not set endcap=square
(
"LINESTRING (50 50, 150 150, 150 50)",
100,
"'side=both'",
<TODO>,
),
```
Here are the results I got, the current ode currently fails the first 3, while it passes the 4th. Currently, around ~2x off from the correct answer on the wrong ones.
1. Sedona: 35847.55494469865. PostGIS: 16285.07633336958
2. Sedona: 10000.0 PostGIS: 19248.578060903223
3. Sedona: 10000.0 PostGIS: 3600.0
4. Sedona: 69888.08929186598 PostGIS: 69888.089291866
The 3rd or 4th cases tests cases that are easy to overlook while implementing the logic (each for different reasons). Just make sure you follow the comments I left there. It's not hard, but it's easy to miss. This is the exact bug that the PR I mentioned above was fixing, so consider checking out the java implementation there.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2465803431
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
Review Comment:
It's a great point that we could use an optional matcher...I kind of like the one kernel per combination of arguments but if the other way can be done cleanly it's OK too.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463159374
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
Review Comment:
Just providing more motivation on *why* multiple kernels are preferred.
If we discover the `geo` implementation of a basic `st_buffer(geom, dist)` (no extra params) is faster (), we could support that kernel for `st_buffer(geom, dist)`, while simultaneously supporting the geos kernel that you're adding here.
In the code below, we register all of the functions from different libraries. If a kernel exists for two libraries, the earlier one will be overwritten. e.g `geo` overwrites `geos` at the moment.
https://github.com/apache/sedona-db/blob/b8ac5efbe358ebb9344cf9177496447e233d2253/rust/sedona/src/context.rs#L128-L134
If it turns out the `geo` implementation is faster than `geos` ([issue](https://github.com/apache/sedona-db/issues/232)), adding these new params as a separate kernel allows us to keep a `geo` kernel for `st_buffer(geom, dist)` while also keeping a `geos` kernel for `st_geom(geom, dist, params)`. `geo` [doesn't seem to support](https://github.com/georust/geo/blob/7263968eb533028fd9be81eaeb9ca455e8a2aee6/geo/src/algorithm/buffer.rs#L141-L145) all of the params either way, so this `geos` implementation will be helpful for us regardless.
idk off the top of my head if things are hooked up so it would work that way, but that's the idea anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462977688
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
Review Comment:
So if I get you correctly, two kernels, I get to register the new one as well?
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462374867
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -32,6 +32,19 @@ use sedona_schema::{
use crate::executor::GeosExecutor;
/// ST_Buffer() implementation using the geos crate
+///
+/// Supports three signatures:
+/// - ST_Buffer(geometry: Geometry, distance: Double)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean)
+/// - ST_Buffer(geometry: Geometry, distance: Double, useSpheroid: Boolean, bufferStyleParameters: String)
Review Comment:
I see you're following Sedona's buffer signatures.
I think we want to follow PostGIS's signatures, which don't include useSpheroid. @paleolimbot Confirm?
https://postgis.net/docs/ST_Buffer.html
We already have this one:
`ST_Buffer(geometry g1, float radius_of_buffer);`
This is the one I had in mind in the issue
`ST_Buffer(geometry g1, float radius_of_buffer, text buffer_style_parameters = '');`
PostGIS also supports the below signature. To my understanding, it's just the quad_segs argument in buffer style params, so it's not new functionality. You're welcome to implement this too, though I mainly had the previous one in mind.
`ST_Buffer(geometry g1, float radius_of_buffer, integer num_seg_quarter_circle);`
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #241:
URL: https://github.com/apache/sedona-db/pull/241
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2468034766
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,65 +67,191 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
- let executor = GeosExecutor::new(arg_types, args);
- let mut builder = BinaryBuilder::with_capacity(
- executor.num_iterations(),
- WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
- );
- executor.execute_wkb_void(|wkb| {
- match (wkb, distance) {
- (Some(wkb), Some(distance)) => {
- invoke_scalar(&wkb, distance, ¶ms, &mut builder)?;
- builder.append_value([]);
- }
- _ => builder.append_null(),
- }
+impl SedonaScalarKernel for STBufferStyle {
+ fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
+ let matcher = ArgMatcher::new(
+ vec![
+ ArgMatcher::is_geometry(),
+ ArgMatcher::is_numeric(),
+ ArgMatcher::is_string(),
+ ],
+ WKB_GEOMETRY,
+ );
- Ok(())
- })?;
+ matcher.match_args(args)
+ }
- executor.finish(Arc::new(builder.finish()))
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ invoke_batch_impl(arg_types, args)
}
}
+fn invoke_batch_impl(arg_types: &[SedonaType], args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ let executor = GeosExecutor::new(arg_types, args);
+ let mut builder = BinaryBuilder::with_capacity(
+ executor.num_iterations(),
+ WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
+ );
+
+ // Extract Args
+ let distance_value = args[1]
+ .cast_to(&DataType::Float64, None)?
+ .to_array(executor.num_iterations())?;
+ let distance_array = as_float64_array(&distance_value)?;
+ let mut distance_iter = distance_array.iter();
+
+ let buffer_style_params = extract_optional_string(args.get(2))?;
+
+ // Build BufferParams based on style parameters
+ let params = parse_buffer_params(buffer_style_params.as_deref())?;
+
+ executor.execute_wkb_void(|wkb| {
+ match (wkb, distance_iter.next().unwrap()) {
+ (Some(wkb), Some(distance)) => {
+ builder.append_value(invoke_scalar(&wkb, distance, ¶ms)?);
+ }
+ _ => builder.append_null(),
+ }
+ Ok(())
+ })?;
+
+ executor.finish(Arc::new(builder.finish()))
+}
+
fn invoke_scalar(
geos_geom: &geos::Geometry,
distance: f64,
params: &BufferParams,
- writer: &mut impl std::io::Write,
-) -> Result<()> {
+) -> Result<Vec<u8>> {
let geometry = geos_geom
.buffer_with_params(distance, params)
.map_err(|e| DataFusionError::External(Box::new(e)))?;
let wkb = geometry
.to_wkb()
.map_err(|e| DataFusionError::Execution(format!("Failed to convert to wkb: {e}")))?;
- writer.write_all(wkb.as_ref())?;
- Ok(())
+ Ok(wkb.into())
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {arg:?}",
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
+ let Some(params_str) = params_str else {
+ return BufferParams::builder()
+ .build()
+ .map_err(|e| DataFusionError::External(Box::new(e)));
+ };
+
+ let mut params_builder = BufferParams::builder();
+
+ for param in params_str.split_whitespace() {
+ let Some((key, value)) = param.split_once('=') else {
+ return Err(DataFusionError::Execution(format!(
+ "Missing value for buffer parameter: {param}",
+ )));
+ };
+
+ if key.eq_ignore_ascii_case("endcap") {
+ params_builder = params_builder.end_cap_style(parse_cap_style(value)?);
+ } else if key.eq_ignore_ascii_case("join") {
+ params_builder = params_builder.join_style(parse_join_style(value)?);
+ } else if key.eq_ignore_ascii_case("side") {
+ params_builder = params_builder.single_sided(is_single_sided(value)?);
Review Comment:
We're still missing this logic:
https://github.com/apache/sedona/blame/d6ea87cf9f2a27adee00ff0fb41ee92002b9a834/common/src/main/java/org/apache/sedona/common/Functions.java#L399-L402
Here's a better docstring to follow because the PostGIS docs hasn't been updated yet.
- `side=both|left|right` : Defaults to `both`. Setting `left` or `right` enables a single-sided buffer operation on the geometry, with the buffered side aligned according to the direction of the line. This functionality is specific to LINESTRING geometry and has no impact on POINT or POLYGON geometries. By default, square end caps are applied when `left` or `right` are specified.
Also, when I was experimenting with the python tests, they didn't behave as I thought they would. While I expected `side=right` to fail, I thought `side=right endcap=square` would pass, but the result was the same. I think it's possible there's another bug somewhere. Though I wouldn't worry about this for now. Just implement the logic for `side`, and we'll see if there's still strange behavior.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2471669644
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -176,6 +176,115 @@ def test_st_buffer(eng, geom, dist, expected_area):
)
+@pytest.mark.parametrize("eng", [SedonaDB, PostGIS])
+@pytest.mark.parametrize(
+ ("geom", "dist", "buffer_style_parameters", "expected_area"),
+ [
+ (None, None, None, None),
+ ("POINT(100 90)", 50, "'quad_segs=8'", 7803.612880645131),
+ (
+ "LINESTRING(50 50,150 150,150 50)",
+ 10,
+ "'endcap=round join=round'",
+ 5016.204476944362,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 2,
+ "'join=miter'",
+ 196.0,
+ ),
+ (
+ "LINESTRING(0 0, 10 0)",
+ 5,
+ "'endcap=square'",
+ 200.0,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=4'",
+ 306.1467458920718,
+ ),
+ (
+ "POINT(0 0)",
+ 10,
+ "'quad_segs=16'",
+ 313.654849054594,
+ ),
+ (
+ "LINESTRING(0 0, 100 0, 100 100)",
+ 5,
+ "'join=bevel'",
+ 2065.536128806451,
+ ),
+ (
+ "LINESTRING(0 0, 50 0)",
+ 10,
+ "'endcap=flat'",
+ 1000.0,
+ ),
+ (
+ "POLYGON((0 0, 0 20, 20 20, 20 0, 0 0))",
+ -2,
+ "'join=round'",
+ 256.0,
+ ),
+ (
+ "POLYGON((0 0, 0 100, 100 100, 100 0, 0 0), (20 20, 20 80, 80 80, 80 20, 20 20))",
+ 5,
+ "'join=round quad_segs=4'",
+ 9576.536686473019,
+ ),
+ (
+ "MULTIPOINT((10 10), (30 30))",
+ 5,
+ "'quad_segs=8'",
+ 156.0722576129026,
+ ),
+ (
+ "GEOMETRYCOLLECTION(POINT(10 10), LINESTRING(50 50, 60 60))",
+ 3,
+ "'endcap=round join=round'",
+ 141.0388264830308,
+ ),
+ (
+ "POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))",
+ 0,
+ "'join=miter'",
+ 100.0,
+ ),
+ (
+ "POINT(0 0)",
+ 0.1,
+ "'quad_segs=8'",
+ 0.031214451522580514,
+ ),
+ (
+ "LINESTRING(0 0, 50 0, 50 50)",
+ 10,
+ "'join=miter miter_limit=2'",
+ 2312.1445152258043,
+ ),
+ (
+ "LINESTRING(0 0, 0 100)",
+ 10,
+ "'side=left'",
+ 1000.0,
+ ),
Review Comment:
Wow. You did an amazing job investigating. Glad to here you figured out what the other mystery was. I see [here](https://hub.docker.com/r/postgis/postgis) that there are indeed Docker images that support `geos` 13. I gave `postgis/postgis:18-3.6` a try locally, and the tests pass. It's the only one that falls under the ([recommended debian section](https://hub.docker.com/r/postgis/postgis#debian-based-recommended)). How about you just update it [here](https://github.com/apache/sedona-db/blob/0551583a675da1cf5cc61cb7c7a61352051f1fc6/compose.yml#L20).
```
image: postgis/postgis:18-3.6
```
WDYT @paleolimbot, is this a reasonable change? It ensures our `geos` version matches the container's. They say "This Docker-PostGIS image has a cautious release cycle to guarantee high stability", so it seems safe to use to me.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3462123615
Great job from you all with the review so far!!!
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2462978252
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -41,41 +54,37 @@ struct STBuffer {}
impl SedonaScalarKernel for STBuffer {
fn return_type(&self, args: &[SedonaType]) -> Result<Option<SedonaType>> {
- let matcher = ArgMatcher::new(
- vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()],
- WKB_GEOMETRY,
- );
+ if !(2..=4).contains(&args.len()) {
+ return Err(DataFusionError::Plan(format!(
+ "ST_Buffer expects 2-4 arguments, got {}",
+ args.len()
+ )));
+ }
+
+ let mut matchers = vec![ArgMatcher::is_geometry(), ArgMatcher::is_numeric()];
- matcher.match_args(args)
+ if args.len() >= 3 {
+ matchers.push(ArgMatcher::is_boolean());
+ }
+ if args.len() == 4 {
+ matchers.push(ArgMatcher::is_string());
+ }
+
+ ArgMatcher::new(matchers, WKB_GEOMETRY).match_args(args)
}
fn invoke_batch(
&self,
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ // Extract Args
+ let distance: Option<f64> = extract_optional_f64(&args[1])?;
Review Comment:
I did convert it to an array :sweat_smile: thanks for the vote of confidence, please do let me know if the implementation is going as expected
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #241:
URL: https://github.com/apache/sedona-db/pull/241#issuecomment-3452000365
I see:
```
failures:
---- st_buffer::tests::udf_with_buffer_params::sedona_type_1_WKB_GEOMETRY stdout ----
thread 'st_buffer::tests::udf_with_buffer_params::sedona_type_1_WKB_GEOMETRY' panicked at rust/sedona-testing/src/compare.rs:169:9:
error: test failed, to rerun pass `-p sedona-geos --lib`
Arrow ScalarValues not equal:
actual is Binary("1,3,0,0,0,1,0,0,0,5,0,0,0,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,1,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,1,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191"), expected Binary("1,3,0,0,0,1,0,0,0,5,0,0,0,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- st_buffer::tests::udf_with_buffer_params::sedona_type_2_WKB_VIEW_GEOMETRY stdout ----
thread 'st_buffer::tests::udf_with_buffer_params::sedona_type_2_WKB_VIEW_GEOMETRY' panicked at rust/sedona-testing/src/compare.rs:169:9:
Arrow ScalarValues not equal:
actual is Binary("1,3,0,0,0,1,0,0,0,5,0,0,0,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,1,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,1,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191"), expected Binary("1,3,0,0,0,1,0,0,0,5,0,0,0,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,63,0,0,0,0,0,0,38,64,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191,0,0,0,0,0,0,240,191")
failures:
st_buffer::tests::udf_with_buffer_params::sedona_type_1_WKB_GEOMETRY
st_buffer::tests::udf_with_buffer_params::sedona_type_2_WKB_VIEW_GEOMETRY
```
...which unfortunately is not a very useful failure (sorry), but probably means that test needs to be adjusted to be more lenient somehow.
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "Abeeujah (via GitHub)" <gi...@apache.org>.
Abeeujah commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2465486523
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -54,50 +67,73 @@ impl SedonaScalarKernel for STBuffer {
arg_types: &[SedonaType],
args: &[ColumnarValue],
) -> Result<ColumnarValue> {
- // Default params
- let params_builder = BufferParams::builder();
+ invoke_batch_impl(arg_types, args)
+ }
+}
- let params = params_builder
- .build()
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
-
- // Extract the constant scalar value before looping over the input geometries
- let distance: Option<f64>;
- let arg1 = args[1].cast_to(&DataType::Float64, None)?;
- if let ColumnarValue::Scalar(scalar_arg) = &arg1 {
- if scalar_arg.is_null() {
- distance = None;
- } else {
- distance = Some(f64::try_from(scalar_arg.clone())?);
- }
- } else {
- return Err(DataFusionError::Execution(format!(
- "Invalid distance: {:?}",
- args[1]
- )));
- }
+pub fn st_buffer_style_impl() -> ScalarKernelRef {
+ Arc::new(STBufferStyle {})
+}
+#[derive(Debug)]
+struct STBufferStyle {}
Review Comment:
I did something similar initially, but this current approach is preferred, as reviewed above in https://github.com/apache/sedona-db/pull/241#discussion_r2462399367
--
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] Plumb remaining parameters for ST_Buffer [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #241:
URL: https://github.com/apache/sedona-db/pull/241#discussion_r2463155943
##########
c/sedona-geos/src/register.rs:
##########
@@ -37,6 +44,7 @@ pub fn scalar_kernels() -> Vec<(&'static str, ScalarKernelRef)> {
vec![
("st_area", st_area_impl()),
("st_buffer", st_buffer_impl()),
+ ("st_buffer_style", st_buffer_style_impl()),
Review Comment:
```suggestion
("st_buffer", st_buffer_style_impl()),
```
This string here represents the actual string we call in SQL, so we actually want this to be `"st_buffer"` and not `"st_buffer_style"`. With this current code, we have to instead call it like below.
```
select st_buffer_style(st_geomfromtext('point (0 0)'), 1, '');
```
##########
c/sedona-geos/src/st_buffer.rs:
##########
@@ -115,6 +124,136 @@ fn invoke_scalar(
Ok(())
}
+fn extract_optional_f64(arg: &ColumnarValue) -> Result<Option<f64>> {
+ let casted = arg.cast_to(&DataType::Float64, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(f64::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar distance, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_bool(arg: Option<&ColumnarValue>) -> Result<Option<bool>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Boolean, None)?;
+ match &casted {
+ ColumnarValue::Scalar(scalar) if !scalar.is_null() => {
+ Ok(Some(bool::try_from(scalar.clone())?))
+ }
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar useSpheroid parameter, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn extract_optional_string(arg: Option<&ColumnarValue>) -> Result<Option<String>> {
+ let Some(arg) = arg else { return Ok(None) };
+ let casted = arg.cast_to(&DataType::Utf8, None)?;
+ match &casted {
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some(s)) | ScalarValue::LargeUtf8(Some(s))) => {
+ Ok(Some(s.clone()))
+ }
+ ColumnarValue::Scalar(scalar) if scalar.is_null() => Ok(None),
+ ColumnarValue::Scalar(_) => Ok(None),
+ _ => Err(DataFusionError::Execution(format!(
+ "Expected scalar bufferStyleParameters, got: {:?}",
+ arg
+ ))),
+ }
+}
+
+fn parse_buffer_params(params_str: Option<&str>) -> Result<BufferParams> {
Review Comment:
Yes, let's definitely add negative tests. Some ideas: invalid keys (e.g `key1=2`), invalid value types (e.g `mitre_limit=abcd`).
Add a test for empty string (which should work). We should have some at the Python level, as well in case we add new kernels from another library (though idk how likely that is, considering `geo` doesn't support all params).
--
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