Kontinuation commented on code in PR #695: URL: https://github.com/apache/sedona-db/pull/695#discussion_r2936559753
########## c/sedona-gdal/src/vsi.rs: ########## @@ -0,0 +1,209 @@ +// 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. + +//! Ported (and contains copied code) from georust/gdal: +//! <https://github.com/georust/gdal/blob/v0.19.0/src/vsi.rs>. +//! Original code is licensed under MIT. +//! +//! GDAL Virtual File System (VSI) wrappers. + +use std::ffi::CString; + +use crate::errors::{GdalError, Result}; +use crate::gdal_api::{call_gdal_api, GdalApi}; + +/// Creates a new VSI in-memory file from a given buffer. +/// +/// The data is copied into GDAL-allocated memory (via `VSIMalloc`) so that +/// GDAL can safely free it with `VSIFree` when ownership is taken, without +/// crossing allocator boundaries back into Rust. +pub fn create_mem_file(api: &'static GdalApi, file_name: &str, data: &[u8]) -> Result<()> { + let c_file_name = CString::new(file_name)?; + let len = data.len(); + let len_i64 = i64::try_from(len)?; + + let gdal_buf = if len == 0 { + std::ptr::null_mut() + } else { + // Allocate via GDAL's allocator so GDAL can safely free it. + let gdal_buf = unsafe { call_gdal_api!(api, VSIMalloc, len) } as *mut u8; + if gdal_buf.is_null() { + return Err(GdalError::NullPointer { + method_name: "VSIMalloc", + msg: format!("failed to allocate {len} bytes"), + }); + } + + // Copy data into GDAL-allocated buffer. + unsafe { + std::ptr::copy_nonoverlapping(data.as_ptr(), gdal_buf, len); + } + gdal_buf + }; + + let handle = unsafe { + call_gdal_api!( + api, + VSIFileFromMemBuffer, + c_file_name.as_ptr(), + gdal_buf, + len_i64, + 1 // bTakeOwnership = true — GDAL will VSIFree gdal_buf + ) + }; + + if handle.is_null() { + // GDAL did not take ownership, so we must free. + if !gdal_buf.is_null() { + unsafe { call_gdal_api!(api, VSIFree, gdal_buf as *mut std::ffi::c_void) }; + } + return Err(GdalError::NullPointer { + method_name: "VSIFileFromMemBuffer", + msg: String::new(), + }); + } + + unsafe { + call_gdal_api!(api, VSIFCloseL, handle); + } + + Ok(()) +} + +/// Unlink (delete) a VSI in-memory file. +pub fn unlink_mem_file(api: &'static GdalApi, file_name: &str) -> Result<()> { + let c_file_name = CString::new(file_name)?; + + let rv = unsafe { call_gdal_api!(api, VSIUnlink, c_file_name.as_ptr()) }; + + if rv != 0 { + return Err(GdalError::UnlinkMemFile { + file_name: file_name.to_string(), + }); + } + + Ok(()) +} + +/// Copies the bytes of the VSI in-memory file, taking ownership and freeing the GDAL memory. +pub fn get_vsi_mem_file_bytes_owned(api: &'static GdalApi, file_name: &str) -> Result<Vec<u8>> { Review Comment: Yes. `pub struct VSIBuffer` seems to be a better API. I have added `get_vsi_mem_file_buffer_owned` that returns `Result<VSIBuffer>`. ########## c/sedona-gdal/src/spatial_ref.rs: ########## @@ -0,0 +1,172 @@ +// 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. + +//! Ported (and contains copied code) from georust/gdal: +//! <https://github.com/georust/gdal/blob/v0.19.0/src/spatial_ref/srs.rs>. +//! Original code is licensed under MIT. + +use std::ffi::{CStr, CString}; +use std::ptr; + +use crate::errors::{GdalError, Result}; +use crate::gdal_api::{call_gdal_api, GdalApi}; +use crate::gdal_dyn_bindgen::OGRERR_NONE; +use crate::gdal_dyn_bindgen::*; + +/// An OGR spatial reference system. +pub struct SpatialRef { + api: &'static GdalApi, + c_srs: OGRSpatialReferenceH, +} + +// SAFETY: `SpatialRef` has unique ownership of its GDAL handle and only moves that +// ownership between threads. The handle is released exactly once on drop, and this +// wrapper does not provide shared concurrent access, so `Send` is sound while `Sync` +// remains intentionally unimplemented. +unsafe impl Send for SpatialRef {} + +impl Drop for SpatialRef { + fn drop(&mut self) { + if !self.c_srs.is_null() { + unsafe { call_gdal_api!(self.api, OSRRelease, self.c_srs) }; + } + } +} + +impl SpatialRef { + /// Create a new SpatialRef from a WKT string. + pub fn from_wkt(api: &'static GdalApi, wkt: &str) -> Result<Self> { + let c_wkt = CString::new(wkt)?; + let c_srs = unsafe { call_gdal_api!(api, OSRNewSpatialReference, c_wkt.as_ptr()) }; Review Comment: I have added another constructor `SpatialRef::from_definition` that calls `OSRSetFromUserInput` under the hood. The function is also brought from georust/gdal. Actually we do not need to construct SpatialRef objects directly when implementing RS functions. We call [`GDALSetProjection`](https://gdal.org/en/stable/api/raster_c_api.html#_CPPv417GDALSetProjection12GDALDatasetHPKc), which calls `SetFromUserInput` internally. It also forces GIS traditional axis order by [calling `oSRS.SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER)`](https://github.com/OSGeo/gdal/blob/19348d2770d32597d6d3875b20ec48b41fe33f1d/gcore/gdaldataset.cpp#L1509-L1525) before loading the projection, so EPSG:4326 will have lon-lat axis order just as OGC:CRS84. I have added `axis_mapping_strategy`, `set_axis_mapping_strategy`, `data_axis_to_srs_axis_mapping` and `epsg_treats_as_lat_long` to SpatialRef for feature completeness. SpatialRef does not implicitly enforce axis order, users are free to set the axis order by calling `set_axis_mapping_strategy` themselves. ########## c/sedona-gdal/src/geo_transform.rs: ########## @@ -0,0 +1,152 @@ +// 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. + +//! Ported (and contains copied code) from georust/gdal: +//! <https://github.com/georust/gdal/blob/v0.19.0/src/geo_transform.rs>. +//! Original code is licensed under MIT. +//! +//! GeoTransform type and extension trait. +//! +//! The [`apply`](GeoTransformEx::apply) and [`invert`](GeoTransformEx::invert) +//! methods are pure-Rust reimplementations of GDAL's `GDALApplyGeoTransform` +//! and `GDALInvGeoTransform` (from `alg/gdaltransformer.cpp`). No FFI call or +//! thread-local state is needed. + +use crate::errors; +use crate::errors::GdalError; + +/// An affine geo-transform: six coefficients mapping pixel/line to projection coordinates. +/// +/// - `[0]`: x-coordinate of the upper-left corner of the upper-left pixel. +/// - `[1]`: W-E pixel resolution (pixel width). +/// - `[2]`: row rotation (typically zero). +/// - `[3]`: y-coordinate of the upper-left corner of the upper-left pixel. +/// - `[4]`: column rotation (typically zero). +/// - `[5]`: N-S pixel resolution (pixel height, negative for North-up). +pub type GeoTransform = [f64; 6]; + +/// Extension methods on [`GeoTransform`]. +pub trait GeoTransformEx { + /// Apply the geo-transform to a pixel/line coordinate, returning (geo_x, geo_y). + fn apply(&self, x: f64, y: f64) -> (f64, f64); + + /// Invert this geo-transform, returning the inverse coefficients for + /// computing (geo_x, geo_y) -> (x, y) transformations. + fn invert(&self) -> errors::Result<GeoTransform>; +} + +impl GeoTransformEx for GeoTransform { + /// Pure-Rust equivalent of GDAL's `GDALApplyGeoTransform`. + fn apply(&self, x: f64, y: f64) -> (f64, f64) { + let geo_x = self[0] + x * self[1] + y * self[2]; + let geo_y = self[3] + x * self[4] + y * self[5]; + (geo_x, geo_y) + } Review Comment: I have evaluated the approach of using `glam` to implement `GeoTransformEx` and found that it is less maintainable than the current approach. 1. We need to carefully map geo transform parameters to the matrix elements and translation vector of DAffine2, which still need all the tests to verify that we are implement it correctly. 2. The code for detecting close-to-zero determinant values cannot be eliminated after switching to glam, since glam does not expose an interface for computing inverse affine transformation while reporting non-inversible affine transformation as error. The code is still verbose after the refactoring. 3. These functions were directly translated from `GDALApplyGeoTransform` and `GDALInvGeoTransform` in GDAL source code, so the maintainer could easily verify that the behavior is identical to the original GDAL code. The reason why we don't reuse the code in [`affine_transformation.rs`](https://github.com/apache/sedona-db/blob/cff68404ede9732e19c9cba5845aff9d09cdefc7/rust/sedona-raster/src/affine_transformation.rs) is because we want c/sedona-gdal to be a standalone module. Another approach is to define fn apply as `apply(&self, api: &'static GdalApi, x: f64, y: f64)` and call into the `GDALApplyGeoTransform` of dynamically loaded GDAL. This requires the caller to pass in an awkward `api` parameter for this simple function, and the function is stable and simple enough to be replicated in Rust to avoid another level of indirection. -- 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]
