You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2025/10/14 15:03:47 UTC
[PR] chore(c/sedona-s2geography): Remove build time bindgen dependency [sedona-db]
paleolimbot opened a new pull request, #221:
URL: https://github.com/apache/sedona-db/pull/221
In #218 the build time bindgen dependency is causing an issue. We've removed this dependency for the default Python build, but cargo build will still require this (even though cargo build -p specific-package is still fine).
Eventually we should possibly also move towards not building s2geography from within a Rust crate at all (https://github.com/apache/sedona-db/issues/109), but in the meantime this should remove at least one thing blocking the usual build.
--
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] chore(c/sedona-s2geography): Remove build time bindgen dependency [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #221:
URL: https://github.com/apache/sedona-db/pull/221#discussion_r2429637829
##########
c/sedona-s2geography/src/geography_glue_bindgen.rs:
##########
@@ -14,9 +14,80 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#![allow(non_upper_case_globals)]
-#![allow(non_camel_case_types)]
-#![allow(non_snake_case)]
-#![allow(dead_code)]
-include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+use std::os::raw::{c_char, c_int, c_void};
+
+#[repr(C)]
+pub struct ArrowSchema {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+pub struct ArrowArray {
+ _private: [u8; 0],
+}
Review Comment:
Using zero-sized arrays with `_private: [u8; 0]` for opaque types is not the idiomatic approach in Rust. Consider using `std::ffi::c_void` or defining these as opaque structs with `#[repr(C)] pub struct ArrowSchema;` instead.
```suggestion
pub struct ArrowSchema;
#[repr(C)]
pub struct ArrowArray;
```
##########
c/sedona-s2geography/src/geography_glue_bindgen.rs:
##########
@@ -14,9 +14,80 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#![allow(non_upper_case_globals)]
-#![allow(non_camel_case_types)]
-#![allow(non_snake_case)]
-#![allow(dead_code)]
-include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+use std::os::raw::{c_char, c_int, c_void};
+
+#[repr(C)]
+pub struct ArrowSchema {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+pub struct ArrowArray {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct SedonaGeographyArrowUdf {
+ pub init: Option<
+ unsafe extern "C" fn(
+ self_: *mut SedonaGeographyArrowUdf,
+ arg_schema: *mut ArrowSchema,
+ options: *const c_char,
+ out: *mut ArrowSchema,
+ ) -> c_int,
+ >,
+ pub execute: Option<
+ unsafe extern "C" fn(
+ self_: *mut SedonaGeographyArrowUdf,
+ args: *mut *mut ArrowArray,
+ n_args: i64,
+ out: *mut ArrowArray,
+ ) -> c_int,
+ >,
+ pub get_last_error:
+ Option<unsafe extern "C" fn(self_: *mut SedonaGeographyArrowUdf) -> *const c_char>,
+ pub release: Option<unsafe extern "C" fn(self_: *mut SedonaGeographyArrowUdf)>,
+ pub private_data: *mut c_void,
+}
+
+macro_rules! declare_s2_c_udfs {
+ ($($name:ident),*) => {
+ $(
+ paste::item! {
+ pub fn [<SedonaGeographyInitUdf $name>](out: *mut SedonaGeographyArrowUdf);
Review Comment:
The macro-generated function declarations are missing the `unsafe extern \"C\"` qualifier. C function declarations in unsafe extern blocks should be consistent with their actual signatures.
```suggestion
pub unsafe extern "C" fn [<SedonaGeographyInitUdf $name>](out: *mut SedonaGeographyArrowUdf);
```
--
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] chore(c/sedona-s2geography): Remove build time bindgen dependency [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #221:
URL: https://github.com/apache/sedona-db/pull/221#discussion_r2429648821
##########
c/sedona-s2geography/src/geography_glue_bindgen.rs:
##########
@@ -14,9 +14,80 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#![allow(non_upper_case_globals)]
-#![allow(non_camel_case_types)]
-#![allow(non_snake_case)]
-#![allow(dead_code)]
-include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+use std::os::raw::{c_char, c_int, c_void};
+
+#[repr(C)]
+pub struct ArrowSchema {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+pub struct ArrowArray {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct SedonaGeographyArrowUdf {
+ pub init: Option<
+ unsafe extern "C" fn(
+ self_: *mut SedonaGeographyArrowUdf,
+ arg_schema: *mut ArrowSchema,
+ options: *const c_char,
+ out: *mut ArrowSchema,
+ ) -> c_int,
+ >,
+ pub execute: Option<
+ unsafe extern "C" fn(
+ self_: *mut SedonaGeographyArrowUdf,
+ args: *mut *mut ArrowArray,
+ n_args: i64,
+ out: *mut ArrowArray,
+ ) -> c_int,
+ >,
+ pub get_last_error:
+ Option<unsafe extern "C" fn(self_: *mut SedonaGeographyArrowUdf) -> *const c_char>,
+ pub release: Option<unsafe extern "C" fn(self_: *mut SedonaGeographyArrowUdf)>,
+ pub private_data: *mut c_void,
+}
+
+macro_rules! declare_s2_c_udfs {
+ ($($name:ident),*) => {
+ $(
+ paste::item! {
+ pub fn [<SedonaGeographyInitUdf $name>](out: *mut SedonaGeographyArrowUdf);
Review Comment:
We only use the macro within an unsafe block
--
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] chore(c/sedona-s2geography): Remove build time bindgen dependency [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #221:
URL: https://github.com/apache/sedona-db/pull/221#discussion_r2429650917
##########
c/sedona-s2geography/src/geography_glue_bindgen.rs:
##########
@@ -14,9 +14,80 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-#![allow(non_upper_case_globals)]
-#![allow(non_camel_case_types)]
-#![allow(non_snake_case)]
-#![allow(dead_code)]
-include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+use std::os::raw::{c_char, c_int, c_void};
+
+#[repr(C)]
+pub struct ArrowSchema {
+ _private: [u8; 0],
+}
+
+#[repr(C)]
+pub struct ArrowArray {
+ _private: [u8; 0],
+}
Review Comment:
I tried this but it is not FFI-safe (at least one member is required)
--
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] chore(c/sedona-s2geography): Remove build time bindgen dependency [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #221:
URL: https://github.com/apache/sedona-db/pull/221
--
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