Copilot commented on code in PR #697: URL: https://github.com/apache/sedona-db/pull/697#discussion_r3019833921
########## c/sedona-gdal/src/gdal.rs: ########## @@ -0,0 +1,237 @@ +// 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. + +//! High-level convenience wrapper around [`GdalApi`]. +//! +//! [`Gdal`] bundles a `&'static GdalApi` reference and exposes ergonomic +//! methods that delegate to the lower-level constructors and free functions +//! scattered across the crate, eliminating the need to pass `api` explicitly +//! at every call site. + +use crate::config; +use crate::dataset::Dataset; +use crate::driver::{Driver, DriverManager}; +use crate::errors::Result; +use crate::gdal_api::GdalApi; +use crate::gdal_dyn_bindgen::{GDALOpenFlags, OGRFieldType}; +use crate::mem::create_mem_dataset; +use crate::raster::polygonize::{polygonize, PolygonizeOptions}; +use crate::raster::rasterband::RasterBand; +use crate::raster::rasterize::{rasterize, RasterizeOptions}; +use crate::raster::rasterize_affine::rasterize_affine; +use crate::raster::types::DatasetOptions; +use crate::raster::types::GdalDataType; +use crate::spatial_ref::SpatialRef; +use crate::vector::feature::FieldDefn; +use crate::vector::geometry::Geometry; +use crate::vector::layer::Layer; +use crate::vrt::VrtDataset; +use crate::vsi; + +/// High-level convenience wrapper around [`GdalApi`]. +/// +/// Stores a `&'static GdalApi` reference and provides ergonomic methods that +/// delegate to the various constructors and free functions in the crate. +pub struct Gdal { + api: &'static GdalApi, +} + +impl Gdal { + /// Create a `Gdal` wrapper for the given API reference. + pub(crate) fn new(api: &'static GdalApi) -> Self { + Self { api } + } + + // -- Info ---------------------------------------------------------------- + + /// Return the name of the loaded GDAL library. + pub fn name(&self) -> &str { + self.api.name() + } + + /// Fetch GDAL version information for a standard request key. + pub fn version_info(&self, request: &str) -> String { + self.api.version_info(request) Review Comment: `Gdal::version_info` is exposed as a safe, infallible API, but it delegates to `GdalApi::version_info` which does `CString::new(request).unwrap()` and will panic on interior NULs. Consider changing this facade method to return `Result<String>` (or otherwise avoid the `unwrap`) so the high-level API doesn't introduce a surprising panic path. ```suggestion let cleaned_request = match request.find('\0') { Some(idx) => &request[..idx], None => request, }; self.api.version_info(cleaned_request) ``` ########## c/sedona-gdal/src/mem.rs: ########## @@ -0,0 +1,439 @@ +// 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. + +//! High-level builder for creating in-memory (MEM) GDAL datasets. +//! +//! [`MemDatasetBuilder`] provides a fluent, type-safe API for constructing GDAL MEM +//! datasets with zero-copy band attachment, optional geo-transform, projection, and +//! per-band nodata values. + +use crate::dataset::Dataset; +use crate::errors::Result; +use crate::gdal::Gdal; +use crate::gdal_api::{call_gdal_api, GdalApi}; +use crate::gdal_dyn_bindgen::CE_Failure; +use crate::raster::types::GdalDataType; + +/// Nodata value for a raster band. +/// +/// GDAL has three separate APIs for setting nodata depending on the band data type: +/// - [`f64`] for most types (UInt8 through Float64, excluding Int64/UInt64) +/// - [`i64`] for Int64 bands +/// - [`u64`] for UInt64 bands +/// +/// This enum encapsulates all three variants so callers don't need to match on +/// the band type when setting nodata. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum Nodata { + F64(f64), + I64(i64), + U64(u64), +} + +/// A band specification for [`MemDatasetBuilder`]. +struct MemBand { + data_type: GdalDataType, + data_ptr: *const u8, + pixel_offset: Option<i64>, + line_offset: Option<i64>, + nodata: Option<Nodata>, +} + +/// A builder for constructing in-memory (MEM) GDAL datasets. +/// +/// This creates datasets using `MEMDataset::Create` (bypassing GDAL's open-dataset-list +/// mutex for better concurrency) and attaches bands via `GDALAddBand` with `DATAPOINTER` +/// options for zero-copy operation. +/// +/// # Safety +/// +/// All `add_band*` methods are `unsafe` because the caller must ensure that the +/// provided data pointers remain valid for the lifetime of the built [`Dataset`]. +pub struct MemDatasetBuilder { + width: usize, + height: usize, + n_owned_bands: usize, + owned_bands_data_type: Option<GdalDataType>, + bands: Vec<MemBand>, + geo_transform: Option<[f64; 6]>, + projection: Option<String>, +} + +impl MemDatasetBuilder { + /// Create a builder for a MEM dataset with the given dimensions. + pub fn new(width: usize, height: usize) -> Self { + Self { + width, + height, + n_owned_bands: 0, + owned_bands_data_type: None, + bands: Vec::new(), + geo_transform: None, + projection: None, + } + } + + /// Create a builder for a MEM dataset with GDAL-owned bands. + pub fn new_with_owned_bands( + width: usize, + height: usize, + n_owned_bands: usize, + owned_bands_data_type: GdalDataType, + ) -> Self { + Self { + width, + height, + n_owned_bands, + owned_bands_data_type: Some(owned_bands_data_type), + bands: Vec::new(), + geo_transform: None, + projection: None, + } + } + + /// Create a MEM dataset with GDAL-owned bands. + /// This is a safe shortcut for `new_with_owned_bands(...).build(gdal)`. + pub fn create( + gdal: &Gdal, + width: usize, + height: usize, + n_owned_bands: usize, + owned_bands_data_type: GdalDataType, + ) -> Result<Dataset> { + // SAFETY: `new_with_owned_bands` creates a builder with zero external bands, + // so no data pointers need to outlive the dataset. + unsafe { + Self::new_with_owned_bands(width, height, n_owned_bands, owned_bands_data_type) + .build(gdal) + } + } + + /// Add a zero-copy band from a raw data pointer. + /// Use default contiguous row-major pixel and line offsets. + /// + /// # Safety + /// + /// The caller must ensure `data_ptr` points to a valid buffer of at least + /// `height * width * data_type.byte_size()` bytes, and that the buffer + /// outlives the built [`Dataset`]. + pub unsafe fn add_band(self, data_type: GdalDataType, data_ptr: *const u8) -> Self { Review Comment: The safety contract for `add_band` only mentions buffer length/outliving the dataset. Because GDAL may read/write through `DATAPOINTER`, the docs should also spell out that the memory must be correctly aligned for `data_type` and (if the dataset may be written to) must be writable; otherwise safe Rust callers could accidentally create UB even though the function is already `unsafe`. ########## c/sedona-gdal/src/mem.rs: ########## @@ -0,0 +1,439 @@ +// 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. + +//! High-level builder for creating in-memory (MEM) GDAL datasets. +//! +//! [`MemDatasetBuilder`] provides a fluent, type-safe API for constructing GDAL MEM +//! datasets with zero-copy band attachment, optional geo-transform, projection, and +//! per-band nodata values. + +use crate::dataset::Dataset; +use crate::errors::Result; +use crate::gdal::Gdal; +use crate::gdal_api::{call_gdal_api, GdalApi}; +use crate::gdal_dyn_bindgen::CE_Failure; +use crate::raster::types::GdalDataType; + +/// Nodata value for a raster band. +/// +/// GDAL has three separate APIs for setting nodata depending on the band data type: +/// - [`f64`] for most types (UInt8 through Float64, excluding Int64/UInt64) +/// - [`i64`] for Int64 bands +/// - [`u64`] for UInt64 bands +/// +/// This enum encapsulates all three variants so callers don't need to match on +/// the band type when setting nodata. Review Comment: The `Nodata` docs say callers don't need to match on the band type when setting nodata, but callers still have to choose the correct enum variant (and the builder doesn't validate that the variant matches the band data type). Consider clarifying the docs to avoid implying stronger type-safety than is currently enforced, or add a validation step that returns a clear error on mismatches. ```suggestion /// This enum groups all three representations behind a single type so callers /// can pass a `Nodata` value without dealing with the raw GDAL APIs directly. /// /// **Important:** Callers are still responsible for choosing the variant that /// matches the band data type (for example, `Nodata::F64` for `Float32` /// bands, `Nodata::I64` for `Int64` bands, etc.). The builder does not /// currently enforce at compile time that the chosen variant matches /// `GdalDataType`, and a mismatch may result in runtime errors from GDAL. ``` -- 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]
