Copilot commented on code in PR #708: URL: https://github.com/apache/sedona-db/pull/708#discussion_r3104440026
########## c/sedona-s2geography/src/geography.rs: ########## @@ -0,0 +1,196 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::marker::PhantomData; +use std::ptr; + +use crate::s2geog_call; +use crate::s2geog_check; +use crate::s2geography_c_bindgen::*; +use crate::utils::S2GeogCError; + +/// Safe wrapper around an unbound S2Geog geography object +/// +/// The Geography is a wrapper around a traditional geometry optmizied for +/// a spherical interpretation of edges using the S2 geometry library. The +/// geography caches some internal scratch space and may be reused when +/// looping over many geographies derived from the same lifetime. +/// +/// Some Geography objects are tied to the lifetime of their input (e.g., +/// when reading WKB); however, others own their coordinates (e.g., when +/// reading WKT). +/// +/// Geographies are thread safe. The internal index is built on the fly when +/// required in a thread safe way (although it may result in faster evaluation +/// to force a build early in a situation where it is known that a lot of +/// evaluations are about to happen). +pub struct Geography<'a> { + ptr: *mut S2Geog, + _marker: PhantomData<&'a [u8]>, +} + +impl<'a> Geography<'a> { + /// Create a new empty geography + pub fn new() -> Self { + let mut ptr: *mut S2Geog = ptr::null_mut(); + unsafe { s2geog_check!(S2GeogCreate(&mut ptr)) }.unwrap(); + Self { + ptr, + _marker: PhantomData, + } + } + + /// Force build an index on this geography + /// + /// This operation forces an index build, which may be expensive but can result in + /// faster evaluation when passed to some operations (e.g., predicates). + pub fn prepare(&mut self) -> Result<(), S2GeogCError> { + unsafe { s2geog_call!(S2GeogForcePrepare(self.ptr)) } + } + + /// Estmate the memory used by this geography + pub fn mem_used(&self) -> usize { + unsafe { S2GeogMemUsed(self.ptr) } + } + + pub(crate) fn as_ptr(&self) -> *const S2Geog { + self.ptr + } +} + +impl<'a> Default for Geography<'a> { + fn default() -> Self { + Self::new() + } +} + +impl<'a> Drop for Geography<'a> { + fn drop(&mut self) { + if !self.ptr.is_null() { + unsafe { + S2GeogDestroy(self.ptr); + } + } + } +} + +// Safety: Geography contains only a pointer to C++ data that is thread-safe +// when accessed through const methods +unsafe impl<'a> Send for Geography<'a> {} +unsafe impl<'a> Sync for Geography<'a> {} + +/// Factory for creating Geography objects from various formats +pub struct GeographyFactory { + ptr: *mut S2GeogFactory, +} + +impl GeographyFactory { + /// Create a new geography factory + pub fn new() -> Self { + let mut ptr: *mut S2GeogFactory = ptr::null_mut(); + unsafe { s2geog_check!(S2GeogFactoryCreate(&mut ptr)) }.unwrap(); + Self { ptr } + } + + /// Create a geography from WKB bytes + pub fn from_wkb<'a>(&mut self, wkb: &'a [u8]) -> Result<Geography<'a>, S2GeogCError> { + let mut geog = Geography::new(); + self.init_from_wkb(wkb, &mut geog)?; + Ok(geog) + } + + /// Create a bound geography from WKB bytes + /// + /// The returned geography is bound to the lifetime of the WKB buffer, + /// ensuring the buffer is not dropped while the geography is in use. + /// This is necessary because the underlying C++ implementation may + /// reference the original WKB data. + pub fn from_wkb_bound<'a>(&mut self, wkb: &'a [u8]) -> Result<Geography<'a>, S2GeogCError> { + let mut geog = Geography::new(); + self.init_from_wkb(wkb, &mut geog)?; + Ok(geog) + } + + /// Create a geography from WKT string + pub fn from_wkt(&mut self, wkt: &str) -> Result<Geography<'static>, S2GeogCError> { + let mut geog = Geography::new(); + self.init_from_wkt(wkt, &mut geog)?; + Ok(geog) + } + + /// Internal wrappers around the actual init. These is the function that should be used Review Comment: Grammar issue in doc comment: "These is the function" should be "This is the function". ```suggestion /// Internal wrappers around the actual init. This is the function that should be used ``` ########## c/sedona-s2geography/src/geography.rs: ########## @@ -0,0 +1,196 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::marker::PhantomData; +use std::ptr; + +use crate::s2geog_call; +use crate::s2geog_check; +use crate::s2geography_c_bindgen::*; +use crate::utils::S2GeogCError; + +/// Safe wrapper around an unbound S2Geog geography object +/// +/// The Geography is a wrapper around a traditional geometry optmizied for +/// a spherical interpretation of edges using the S2 geometry library. The +/// geography caches some internal scratch space and may be reused when +/// looping over many geographies derived from the same lifetime. +/// +/// Some Geography objects are tied to the lifetime of their input (e.g., +/// when reading WKB); however, others own their coordinates (e.g., when +/// reading WKT). +/// +/// Geographies are thread safe. The internal index is built on the fly when +/// required in a thread safe way (although it may result in faster evaluation +/// to force a build early in a situation where it is known that a lot of +/// evaluations are about to happen). +pub struct Geography<'a> { + ptr: *mut S2Geog, + _marker: PhantomData<&'a [u8]>, +} + +impl<'a> Geography<'a> { + /// Create a new empty geography + pub fn new() -> Self { + let mut ptr: *mut S2Geog = ptr::null_mut(); + unsafe { s2geog_check!(S2GeogCreate(&mut ptr)) }.unwrap(); + Self { + ptr, + _marker: PhantomData, + } + } + + /// Force build an index on this geography + /// + /// This operation forces an index build, which may be expensive but can result in + /// faster evaluation when passed to some operations (e.g., predicates). + pub fn prepare(&mut self) -> Result<(), S2GeogCError> { + unsafe { s2geog_call!(S2GeogForcePrepare(self.ptr)) } + } + + /// Estmate the memory used by this geography + pub fn mem_used(&self) -> usize { + unsafe { S2GeogMemUsed(self.ptr) } + } + + pub(crate) fn as_ptr(&self) -> *const S2Geog { + self.ptr + } +} + +impl<'a> Default for Geography<'a> { + fn default() -> Self { + Self::new() + } +} + +impl<'a> Drop for Geography<'a> { + fn drop(&mut self) { + if !self.ptr.is_null() { + unsafe { + S2GeogDestroy(self.ptr); + } + } + } +} + +// Safety: Geography contains only a pointer to C++ data that is thread-safe +// when accessed through const methods +unsafe impl<'a> Send for Geography<'a> {} +unsafe impl<'a> Sync for Geography<'a> {} + +/// Factory for creating Geography objects from various formats +pub struct GeographyFactory { + ptr: *mut S2GeogFactory, +} + +impl GeographyFactory { + /// Create a new geography factory + pub fn new() -> Self { + let mut ptr: *mut S2GeogFactory = ptr::null_mut(); + unsafe { s2geog_check!(S2GeogFactoryCreate(&mut ptr)) }.unwrap(); + Self { ptr } + } + + /// Create a geography from WKB bytes + pub fn from_wkb<'a>(&mut self, wkb: &'a [u8]) -> Result<Geography<'a>, S2GeogCError> { + let mut geog = Geography::new(); + self.init_from_wkb(wkb, &mut geog)?; + Ok(geog) + } + + /// Create a bound geography from WKB bytes + /// + /// The returned geography is bound to the lifetime of the WKB buffer, + /// ensuring the buffer is not dropped while the geography is in use. + /// This is necessary because the underlying C++ implementation may + /// reference the original WKB data. + pub fn from_wkb_bound<'a>(&mut self, wkb: &'a [u8]) -> Result<Geography<'a>, S2GeogCError> { + let mut geog = Geography::new(); + self.init_from_wkb(wkb, &mut geog)?; + Ok(geog) + } Review Comment: `from_wkb_bound()` appears to be identical to `from_wkb()` (same signature, same implementation, and both return `Geography<'a>` tied to the input buffer). This duplication is likely to confuse API users about ownership/lifetime semantics. Consider removing one of the methods, or changing `from_wkb()`/`from_wkb_bound()` so they have clearly different behavior (e.g., owning vs non-owning). ########## c/sedona-s2geography/src/geography.rs: ########## @@ -0,0 +1,196 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::marker::PhantomData; +use std::ptr; + +use crate::s2geog_call; +use crate::s2geog_check; +use crate::s2geography_c_bindgen::*; +use crate::utils::S2GeogCError; + +/// Safe wrapper around an unbound S2Geog geography object +/// +/// The Geography is a wrapper around a traditional geometry optmizied for +/// a spherical interpretation of edges using the S2 geometry library. The +/// geography caches some internal scratch space and may be reused when +/// looping over many geographies derived from the same lifetime. +/// +/// Some Geography objects are tied to the lifetime of their input (e.g., +/// when reading WKB); however, others own their coordinates (e.g., when +/// reading WKT). +/// +/// Geographies are thread safe. The internal index is built on the fly when +/// required in a thread safe way (although it may result in faster evaluation +/// to force a build early in a situation where it is known that a lot of +/// evaluations are about to happen). +pub struct Geography<'a> { + ptr: *mut S2Geog, + _marker: PhantomData<&'a [u8]>, +} + +impl<'a> Geography<'a> { + /// Create a new empty geography + pub fn new() -> Self { + let mut ptr: *mut S2Geog = ptr::null_mut(); + unsafe { s2geog_check!(S2GeogCreate(&mut ptr)) }.unwrap(); + Self { + ptr, + _marker: PhantomData, + } + } + + /// Force build an index on this geography + /// + /// This operation forces an index build, which may be expensive but can result in + /// faster evaluation when passed to some operations (e.g., predicates). + pub fn prepare(&mut self) -> Result<(), S2GeogCError> { + unsafe { s2geog_call!(S2GeogForcePrepare(self.ptr)) } + } + + /// Estmate the memory used by this geography Review Comment: Typo in doc comment: "Estmate" should be "Estimate". ```suggestion /// Estimate the memory used by this geography ``` ########## c/sedona-s2geography/src/geography.rs: ########## @@ -0,0 +1,196 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::marker::PhantomData; +use std::ptr; + +use crate::s2geog_call; +use crate::s2geog_check; +use crate::s2geography_c_bindgen::*; +use crate::utils::S2GeogCError; + +/// Safe wrapper around an unbound S2Geog geography object +/// +/// The Geography is a wrapper around a traditional geometry optmizied for Review Comment: Typo in doc comment: "optmizied" should be "optimized". ```suggestion /// The Geography is a wrapper around a traditional geometry optimized for ``` ########## c/sedona-s2geography/src/kernels.rs: ########## @@ -0,0 +1,398 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::sync::Arc; + +use datafusion_common::Result; +use sedona_common::sedona_internal_datafusion_err; +use sedona_expr::scalar_udf::ScalarKernelRef; +use sedona_extension::{extension::SedonaCScalarKernel, scalar_kernel::ImportedScalarKernel}; + +use crate::{s2geog_check, s2geography_c_bindgen::*}; + +pub fn s2_scalar_kernels() -> Result<Vec<(String, ScalarKernelRef)>> { + let mut ffi_scalar_kernels = Vec::<SedonaCScalarKernel>::new(); + ffi_scalar_kernels.resize_with(unsafe { S2GeogNumKernels() }, Default::default); + + unsafe { + s2geog_check!(S2GeogInitKernels( + ffi_scalar_kernels.as_mut_ptr() as _, + size_of::<SedonaCScalarKernel>() * ffi_scalar_kernels.len(), Review Comment: `size_of::<SedonaCScalarKernel>()` is used here without importing `std::mem::size_of` (or qualifying it), which will fail to compile. Import `std::mem::size_of` at the top of the module or change this call to `std::mem::size_of::<...>()`. ```suggestion std::mem::size_of::<SedonaCScalarKernel>() * ffi_scalar_kernels.len(), ``` ########## c/sedona-s2geography/src/utils.rs: ########## @@ -0,0 +1,243 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::s2geography_c_bindgen::*; +use std::cell::RefCell; +use std::ptr; +use thiserror::Error; + +/// Compute an S2 Cell identifier from a longitude/latitude pair +/// +/// If either longitude or latitude are NaN (e.g., an empty point), +/// the sentinel cell (`u64::MAX`) is returned. Lon/Lat pairs are +/// normalized such that invalid lon/lat pairs will still compute +/// a result (even though that result may be difficult to interpret). +pub fn s2_cell_id_from_lnglat(lnglat: (f64, f64)) -> u64 { + let vertex = S2GeogVertex { + v: [lnglat.0, lnglat.1, 0.0, 0.0], + }; + unsafe { S2GeogLngLatToCellId(&vertex) } +} + +/// Dependency versions for underlying libraries +pub struct Versions {} + +impl Versions { + /// Return the statically linked s2 version as a string + pub fn s2geometry() -> String { + unsafe { + let raw_c_str = S2GeogS2GeometryVersion(); + let c_str = std::ffi::CStr::from_ptr(raw_c_str); + c_str.to_string_lossy().into_owned() + } + } + + /// Return the linked Abseil version as a string + /// + /// Depending on build-time settings, this may have been statically + /// or dynamically linked. + pub fn abseil() -> String { + unsafe { + let raw_c_str = S2GeogAbseilVersion(); + let c_str = std::ffi::CStr::from_ptr(raw_c_str); + c_str.to_string_lossy().into_owned() + } + } + + /// Return the linked OpenSSL version as a string + /// + /// Depending on build-time settings, this may have been statically + /// or dynamically linked. + pub fn openssl() -> String { + unsafe { + let raw_c_str = S2GeogOpenSSLVersion(); + let c_str = std::ffi::CStr::from_ptr(raw_c_str); + c_str.to_string_lossy().into_owned() + } + } + + /// Return the linked nanoarrow version as a string + pub fn nanoarrow() -> String { + unsafe { + let raw_c_str = S2GeogNanoarrowVersion(); + let c_str = std::ffi::CStr::from_ptr(raw_c_str); + c_str.to_string_lossy().into_owned() + } + } + + /// Return the linked geoarrow version as a string + pub fn geoarrow() -> String { + unsafe { + let raw_c_str = S2GeogGeoArrowVersion(); + let c_str = std::ffi::CStr::from_ptr(raw_c_str); + c_str.to_string_lossy().into_owned() + } + } +} + +/// Error returned by s2geography C API functions +#[derive(Debug, Error)] +#[error("{message} (code: {code})")] +pub struct S2GeogCError { + /// The errno-compatible error code + pub code: i32, + /// The error message from the C API + pub message: String, +} + +impl S2GeogCError { + /// Create a new error with the given code and message + pub fn new(code: i32, message: String) -> Self { + Self { code, message } + } + + /// Create an error with just a code (no message available) + pub fn from_code(func_name: &str, code: i32) -> Self { + Self { + code, + message: format!("{} failed", func_name), + } + } +} + +/// Call an s2geography C function that returns an error code and accepts an error pointer. +/// +/// Uses a thread-local error object to avoid having to allocate one on each stack. +#[macro_export] +macro_rules! s2geog_call { + ($func:ident ( $($arg:expr),* $(,)? )) => {{ + $crate::utils::S2GEOG_ERROR.with_borrow_mut(|err| { + let code = $crate::s2geography_c_bindgen::$func($($arg,)* err.as_mut_ptr()); + if code == $crate::s2geography_c_bindgen::S2GEOGRAPHY_OK { + Ok(()) + } else { + let msg = err.message(); + if msg.is_empty() { + Err($crate::utils::S2GeogCError::from_code(stringify!($func), code)) + } else { + Err($crate::utils::S2GeogCError::new(code, msg)) + } + } + }) + }}; +} + +/// Call an s2geography C function that returns an error code (no error pointer). +/// +/// Use this for functions like `S2GeogInitKernels` that only return a code. +#[macro_export] +macro_rules! s2geog_check { + ($func:ident ( $($arg:expr),* $(,)? )) => {{ + let code = $crate::s2geography_c_bindgen::$func($($arg,)*); + if code == $crate::s2geography_c_bindgen::S2GEOGRAPHY_OK { + Ok(()) + } else { + Err($crate::utils::S2GeogCError::from_code(stringify!($func), code)) + } + }}; +} + +thread_local! { + /// Thread-local error object reused across calls + pub static S2GEOG_ERROR: RefCell<S2GeogErrorGuard> = RefCell::new(S2GeogErrorGuard::new()); +} + +/// Safe wrapper around an S2GeogError that ensures proper cleanup +pub struct S2GeogErrorGuard { + ptr: *mut S2GeogError, +} + +impl S2GeogErrorGuard { + /// Create a new error guard with an allocated error object + pub fn new() -> Self { + let mut ptr: *mut S2GeogError = ptr::null_mut(); + unsafe { + S2GeogErrorCreate(&mut ptr); + } + Self { ptr } + } + + /// Get the raw pointer for passing to C functions + pub fn as_mut_ptr(&mut self) -> *mut S2GeogError { + self.ptr + } + + /// Get the error message, or an empty string if no error + pub fn message(&self) -> String { + if self.ptr.is_null() { + return String::new(); + } + unsafe { + let c_str = S2GeogErrorGetMessage(self.ptr); + if c_str.is_null() { + String::new() + } else { + std::ffi::CStr::from_ptr(c_str) + .to_string_lossy() + .into_owned() + } + } + } +} + +impl Default for S2GeogErrorGuard { + fn default() -> Self { + Self::new() + } +} + +impl Drop for S2GeogErrorGuard { + fn drop(&mut self) { + if !self.ptr.is_null() { + unsafe { + S2GeogErrorDestroy(self.ptr); + } + } + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_s2_cell_id_from_lnglat() { + // Check a single, finite cell + assert_eq!(s2_cell_id_from_lnglat((0.0, 0.0)), 1152921504606846977); + + // Emptyish cases should return the sentinel cell + assert_eq!(s2_cell_id_from_lnglat((f64::NAN, 0.0)), u64::MAX); + assert_eq!(s2_cell_id_from_lnglat((0.0, f64::NAN)), u64::MAX); + assert_eq!(s2_cell_id_from_lnglat((f64::NAN, f64::NAN)), u64::MAX); + + // These should both return something (even if what it returns is difficult + // to interpret) + assert_ne!(s2_cell_id_from_lnglat((181.0, 0.0)), u64::MAX); + assert_ne!(s2_cell_id_from_lnglat((0.0, 91.0)), u64::MAX); + } + + #[test] + fn test_versions() { + assert_eq!(Versions::s2geometry(), "0.13.1"); Review Comment: This test hard-codes the exact S2 version string ("0.13.1"). Since the build can use either the vendored s2geometry submodule or a system/vcpkg-provided `s2` (via `find_package(s2)`), the actual version may legitimately differ and cause spurious failures for developers/CI environments. Consider loosening this assertion (e.g., non-empty, or matching major/minor only) or conditioning it on the build mode. ```suggestion assert!(Versions::s2geometry().contains(".")); ``` -- 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]
