Copilot commented on code in PR #638:
URL: https://github.com/apache/sedona-db/pull/638#discussion_r2824401652
##########
rust/sedona-spatial-join/tests/spatial_join_integration.rs:
##########
@@ -514,6 +521,16 @@ async fn test_geography_join_is_not_optimized() ->
Result<()> {
let options = SpatialJoinOptions::default();
let ctx = setup_context(Some(options), 10)?;
+ let stub_impl = SimpleSedonaScalarKernel::new_ref(
+ ArgMatcher::new(
+ vec![ArgMatcher::is_geography(), ArgMatcher::is_geography()],
+ SedonaType::Arrow(DataType::Boolean),
+ ),
+ Arc::new(|_arg_types, _args| plan_err!("execution!")),
+ );
+ let st_intersects_udf = SedonaScalarUDF::from_impl("st_intersects",
stub_impl);
+ ctx.register_udf(st_intersects_udf.into());
Review Comment:
The stubbed ST_Intersects implementation returns a Plan error with message
"execution!", which is hard to diagnose if it ever triggers. Use a more
descriptive message (e.g., indicating this stub should never execute and exists
only to satisfy planning/type resolution).
##########
rust/sedona-geo/src/st_line_interpolate_point.rs:
##########
@@ -108,16 +108,17 @@ fn invoke_scalar(geom: &geo_types::Geometry<f64>, ratio:
f64) -> Result<(f64, f6
mod tests {
use rstest::rstest;
use sedona_expr::scalar_udf::SedonaScalarUDF;
- use sedona_functions::register::stubs::st_area_udf;
use sedona_schema::datatypes::{WKB_GEOMETRY, WKB_GEOMETRY_ITEM_CRS,
WKB_VIEW_GEOMETRY};
use sedona_testing::{compare::assert_scalar_equal_wkb_geometry,
testers::ScalarUdfTester};
use super::*;
#[rstest]
fn udf(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)] sedona_type: SedonaType)
{
- let mut udf = st_area_udf();
- udf.add_kernels(st_line_interpolate_point_impl());
+ let udf = SedonaScalarUDF::from_impl(
+ "st_line_interpolate_point",
+ st_line_interpolate_point_impl(),
+ );
Review Comment:
The UDF name used in this test ("st_line_interpolate_point") doesn't match
the actual registered function name ("st_lineinterpolatepoint", see
sedona-geo/src/register.rs and the other test below). This inconsistency can
cause confusion and makes it harder to ensure we're testing the public SQL/API
surface. Use a consistent/registered name here.
##########
rust/sedona-geo/src/st_intersection_agg.rs:
##########
@@ -232,8 +232,7 @@ mod test {
#[rstest]
fn polygon_polygon_cases(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)]
sedona_type: SedonaType) {
- let mut udaf = st_intersection_agg_udf();
- udaf.add_kernel(st_intersection_agg_impl());
+ let udaf = SedonaAggregateUDF::from_impl("st_union",
st_intersection_agg_impl());
Review Comment:
In these tests the UDAF is created with the name "st_union", but this file
implements ST_Intersection_Agg and the function is registered elsewhere as
"st_intersection_agg". Using the wrong name can make metadata/registration
tests misleading and can hide name-related issues. Use the correct function
name here.
```suggestion
let udaf = SedonaAggregateUDF::from_impl("st_intersection_agg",
st_intersection_agg_impl());
```
##########
rust/sedona-geo/src/st_union_agg.rs:
##########
@@ -226,8 +226,7 @@ mod test {
#[rstest]
fn polygon_polygon_cases(#[values(WKB_GEOMETRY, WKB_VIEW_GEOMETRY)]
sedona_type: SedonaType) {
- let mut udaf = st_union_agg_udf();
- udaf.add_kernel(st_union_agg_impl());
+ let udaf = SedonaAggregateUDF::from_impl("st_union",
st_union_agg_impl());
Review Comment:
This union aggregate is registered under the name "st_union_agg" (see
sedona-geo/src/register.rs), but the test constructs the UDAF as "st_union".
Please use the correct function name so the test matches the actual exported
API.
##########
rust/sedona-expr/src/scalar_udf.rs:
##########
@@ -426,34 +409,17 @@ mod tests {
tester.assert_return_type(DataType::Utf8);
}
- #[test]
- fn stub() {
- let stub = SedonaScalarUDF::new_stub(
- "stubby",
- ArgMatcher::new(vec![], SedonaType::Arrow(DataType::Boolean)),
- Volatility::Immutable,
- );
- let tester = ScalarUdfTester::new(stub.into(), vec![]);
- tester.assert_return_type(DataType::Boolean);
-
- let err = tester.invoke_arrays(vec![]).unwrap_err();
- assert_eq!(
- err.message(),
- "Implementation for stubby([]) was not registered"
- );
- }
-
#[test]
fn crs_propagation() {
let geom_lnglat = SedonaType::Wkb(Edges::Planar, lnglat());
- let predicate_stub = SedonaScalarUDF::new_stub(
- "stubby",
+ let predicate_stub_impl = SimpleSedonaScalarKernel::new_ref(
ArgMatcher::new(
vec![ArgMatcher::is_geometry(), ArgMatcher::is_geometry()],
SedonaType::Arrow(DataType::Boolean),
),
- Volatility::Immutable,
+ Arc::new(|_arg_types, args| Ok(args[0].clone())),
Review Comment:
This test kernel declares a Boolean return type but the implementation
returns args[0] (a geometry ColumnarValue). That violates the kernel contract
and could lead to confusing failures if the test ever invokes the UDF. Return a
Boolean value (e.g., a constant true/false or null) instead.
```suggestion
Arc::new(|_arg_types, _args| {
Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some(true))))
}),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
