Copilot commented on code in PR #615: URL: https://github.com/apache/sedona-db/pull/615#discussion_r2884312959
########## docs/reference/sql/rs_within.qmd: ########## @@ -0,0 +1,60 @@ +--- +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +title: RS_Within +description: Returns true if the first argument's extent is within the second. +kernels: + - returns: boolean + args: [raster, geometry] + - returns: boolean + args: [geometry, raster] + - returns: boolean + args: + - name: rastA + type: raster + description: Input raster + - name: rastB + type: raster + description: Input raster +--- + +## Description + +Returns `true` if the convex hull of the first argument is completely within +the second argument. This is the inverse of `RS_Contains`: +`RS_Within(A, B)` is equivalent to `RS_Contains(B, A)`. + +Both rasters and geometries are accepted in either argument position. +When two rasters are provided, their convex hulls are compared. + +If the arguments have different CRSes, the function transforms the second +argument into the CRS of the first before evaluating the predicate. If either +argument has no CRS set, WGS 84 is assumed. Review Comment: This CRS behavior statement is not accurate for the (geometry, raster) signature: the implementation currently prefers transforming the geometry into the raster’s CRS (i.e., the first argument is transformed to the second argument’s CRS in that case), not always transforming the second argument into the first argument’s CRS. Please adjust the documentation (or the implementation) so they match. ```suggestion If the arguments have different CRSes and one is a raster while the other is a geometry, the function transforms the geometry into the raster's CRS before evaluating the predicate. When two rasters are provided, the function transforms the second raster into the CRS of the first. If either argument has no CRS set, WGS 84 is assumed. ``` ########## docs/reference/sql/rs_intersects.qmd: ########## @@ -0,0 +1,54 @@ +--- +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +title: RS_Intersects +description: Returns true if the extents of the two arguments intersect. +kernels: + - returns: boolean + args: [raster, geometry] + - returns: boolean + args: [geometry, raster] + - returns: boolean + args: + - name: rastA + type: raster + description: Input raster + - name: rastB + type: raster + description: Input raster +--- + +## Description + +Returns `true` if the convex hull of the first argument intersects the second +argument. Both rasters and geometries are accepted in either argument position. +When two rasters are provided, their convex hulls are compared. + +If the arguments have different CRSes, the function transforms the second +argument into the CRS of the first before evaluating the predicate. If either +argument has no CRS set, WGS 84 is assumed. Review Comment: This CRS behavior statement is not accurate for the (geometry, raster) signature: the implementation currently prefers transforming the geometry into the raster’s CRS (i.e., the first argument is transformed to the second argument’s CRS in that case), not always transforming the second argument into the first argument’s CRS. Please adjust the documentation (or the implementation) so they match. ```suggestion If the arguments have different CRSes, the function transforms them to a common CRS before evaluating the predicate. For raster/geometry pairs, the geometry is transformed into the raster’s CRS (so for the `(geometry, raster)` signature, the first argument is transformed into the CRS of the second). For other combinations, the second argument is transformed into the CRS of the first. If either argument has no CRS set, WGS 84 is assumed. ``` ########## docs/reference/sql/rs_contains.qmd: ########## @@ -0,0 +1,55 @@ +--- +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +title: RS_Contains +description: Returns true if the first argument's extent contains the second. +kernels: + - returns: boolean + args: [raster, geometry] + - returns: boolean + args: [geometry, raster] + - returns: boolean + args: + - name: rastA + type: raster + description: Input raster + - name: rastB + type: raster + description: Input raster +--- + +## Description + +Returns `true` if the convex hull of the first argument completely contains +the second argument. Both rasters and geometries are accepted in either +argument position. When two rasters are provided, their convex hulls are +compared. + +If the arguments have different CRSes, the function transforms the second +argument into the CRS of the first before evaluating the predicate. If either +argument has no CRS set, WGS 84 is assumed. Review Comment: This CRS behavior statement is not accurate for the (geometry, raster) signature: the implementation currently prefers transforming the geometry into the raster’s CRS (i.e., the first argument is transformed to the second argument’s CRS in that case), not always transforming the second argument into the first argument’s CRS. Please adjust the documentation (or the implementation) so they match. ```suggestion If the arguments have different CRSes, the function normally transforms the second argument into the CRS of the first before evaluating the predicate. For the `(geometry, raster)` signature, however, the geometry (first argument) is transformed into the raster's CRS (second argument). If either argument has no CRS set, WGS 84 is assumed. ``` ########## rust/sedona-raster-functions/src/rs_spatial_predicates.rs: ########## @@ -0,0 +1,603 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! RS_Intersects, RS_Contains, RS_Within functions +//! +//! These functions test spatial relationships between rasters and geometries. +//! CRS transformation rules: +//! - If the raster or geometry does not have a defined SRID, it is assumed to be in WGS84 +//! - If both sides are in the same CRS, perform the relationship test directly +//! - Otherwise, both sides will be transformed to WGS84 before the relationship test + +use std::sync::Arc; + +use crate::crs_utils::crs_transform_wkb; +use crate::crs_utils::default_crs; +use crate::crs_utils::resolve_crs; +use crate::executor::RasterExecutor; +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::DataFusionError; +use datafusion_common::Result; +use datafusion_expr::{ColumnarValue, Volatility}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_geometry::wkb_factory::write_wkb_polygon; +use sedona_raster::affine_transformation::to_world_coordinate; +use sedona_raster::traits::RasterRef; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use sedona_tg::tg; + +/// RS_Intersects() scalar UDF documentation +/// +/// Returns true if raster A intersects geometry B. +pub fn rs_intersects_udf() -> SedonaScalarUDF { Review Comment: The function doc comment is misleading: this UDF supports (raster, geometry), (geometry, raster), and (raster, raster), but the comment only describes the (raster, geometry) case. Please update the docs to describe the supported argument combinations (and that raster inputs are compared via their convex hull). ########## rust/sedona-raster-functions/src/rs_spatial_predicates.rs: ########## @@ -0,0 +1,603 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! RS_Intersects, RS_Contains, RS_Within functions +//! +//! These functions test spatial relationships between rasters and geometries. +//! CRS transformation rules: +//! - If the raster or geometry does not have a defined SRID, it is assumed to be in WGS84 +//! - If both sides are in the same CRS, perform the relationship test directly +//! - Otherwise, both sides will be transformed to WGS84 before the relationship test Review Comment: The module-level CRS transformation rules in the doc comment don’t match the actual implementation. `evaluate_predicate_with_crs` first tries transforming one side into the other’s CRS (direction depends on call site) and only falls back to transforming both to WGS84 if that fails; it does not always transform both sides to WGS84 on CRS mismatch. Please update this documentation to reflect the real behavior (and keep it consistent with the SQL reference docs). ```suggestion //! - If a raster or geometry does not have a defined SRID, it is assumed to be in WGS84. //! - If both sides are in the same CRS, the relationship test is performed directly. //! - If the sides use different CRSs, the implementation first attempts to transform one //! side into the other side's CRS (the direction depends on the calling function). //! - If that CRS-to-CRS transformation fails, both sides are transformed to WGS84 and //! the relationship test is performed in WGS84. ``` ########## rust/sedona-raster-functions/src/rs_spatial_predicates.rs: ########## @@ -0,0 +1,603 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! RS_Intersects, RS_Contains, RS_Within functions +//! +//! These functions test spatial relationships between rasters and geometries. +//! CRS transformation rules: +//! - If the raster or geometry does not have a defined SRID, it is assumed to be in WGS84 +//! - If both sides are in the same CRS, perform the relationship test directly +//! - Otherwise, both sides will be transformed to WGS84 before the relationship test + +use std::sync::Arc; + +use crate::crs_utils::crs_transform_wkb; +use crate::crs_utils::default_crs; +use crate::crs_utils::resolve_crs; +use crate::executor::RasterExecutor; +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::DataFusionError; +use datafusion_common::Result; +use datafusion_expr::{ColumnarValue, Volatility}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_geometry::wkb_factory::write_wkb_polygon; +use sedona_raster::affine_transformation::to_world_coordinate; +use sedona_raster::traits::RasterRef; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use sedona_tg::tg; + +/// RS_Intersects() scalar UDF documentation +/// +/// Returns true if raster A intersects geometry B. +pub fn rs_intersects_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "rs_intersects", + vec![ + Arc::new(RsSpatialPredicate::<tg::Intersects>::raster_geom()), + Arc::new(RsSpatialPredicate::<tg::Intersects>::geom_raster()), + Arc::new(RsSpatialPredicate::<tg::Intersects>::raster_raster()), + ], + Volatility::Immutable, + ) +} + +/// RS_Contains() scalar UDF +/// +/// Returns true if raster A contains geometry B. +pub fn rs_contains_udf() -> SedonaScalarUDF { Review Comment: The function doc comment is misleading: this UDF supports (raster, geometry), (geometry, raster), and (raster, raster), but the comment only describes the (raster, geometry) case. Please update the docs to describe the supported argument combinations (and that raster inputs are compared via their convex hull). ########## rust/sedona-raster-functions/src/rs_spatial_predicates.rs: ########## @@ -0,0 +1,603 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! RS_Intersects, RS_Contains, RS_Within functions +//! +//! These functions test spatial relationships between rasters and geometries. +//! CRS transformation rules: +//! - If the raster or geometry does not have a defined SRID, it is assumed to be in WGS84 +//! - If both sides are in the same CRS, perform the relationship test directly +//! - Otherwise, both sides will be transformed to WGS84 before the relationship test + +use std::sync::Arc; + +use crate::crs_utils::crs_transform_wkb; +use crate::crs_utils::default_crs; +use crate::crs_utils::resolve_crs; +use crate::executor::RasterExecutor; +use arrow_array::builder::BooleanBuilder; +use arrow_schema::DataType; +use datafusion_common::DataFusionError; +use datafusion_common::Result; +use datafusion_expr::{ColumnarValue, Volatility}; +use sedona_expr::scalar_udf::{SedonaScalarKernel, SedonaScalarUDF}; +use sedona_geometry::wkb_factory::write_wkb_polygon; +use sedona_raster::affine_transformation::to_world_coordinate; +use sedona_raster::traits::RasterRef; +use sedona_schema::{datatypes::SedonaType, matchers::ArgMatcher}; +use sedona_tg::tg; + +/// RS_Intersects() scalar UDF documentation +/// +/// Returns true if raster A intersects geometry B. +pub fn rs_intersects_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "rs_intersects", + vec![ + Arc::new(RsSpatialPredicate::<tg::Intersects>::raster_geom()), + Arc::new(RsSpatialPredicate::<tg::Intersects>::geom_raster()), + Arc::new(RsSpatialPredicate::<tg::Intersects>::raster_raster()), + ], + Volatility::Immutable, + ) +} + +/// RS_Contains() scalar UDF +/// +/// Returns true if raster A contains geometry B. +pub fn rs_contains_udf() -> SedonaScalarUDF { + SedonaScalarUDF::new( + "rs_contains", + vec![ + Arc::new(RsSpatialPredicate::<tg::Contains>::raster_geom()), + Arc::new(RsSpatialPredicate::<tg::Contains>::geom_raster()), + Arc::new(RsSpatialPredicate::<tg::Contains>::raster_raster()), + ], + Volatility::Immutable, + ) +} + +/// RS_Within() scalar UDF +/// +/// Returns true if raster A is within geometry B. +pub fn rs_within_udf() -> SedonaScalarUDF { Review Comment: The function doc comment is misleading: this UDF supports (raster, geometry), (geometry, raster), and (raster, raster), but the comment only describes the (raster, geometry) case. Please update the docs to describe the supported argument combinations (and that raster inputs are compared via their convex hull). -- 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]
