Kontinuation commented on code in PR #697: URL: https://github.com/apache/sedona-db/pull/697#discussion_r3020033249
########## 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: I am leaving this as-is. `version_info()` is intended for GDAL-defined request keys such as `RELEASE_NAME` and `VERSION_NUM`, so an interior NUL here is invalid programmer input and results in a panic rather than an unsoundness issue. I do not think that warrants widening the facade API to `Result<String>` or silently truncating the 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: Addressed in 44091077. I tightened the unsafe docs for the MEM builder to cover alignment requirements, dataset lifetime, and the possibility of GDAL writing through attached DATAPOINTER bands. I also made the external-band pointer contract explicit by switching this API from `*const u8` to `*mut u8`. ########## 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: Addressed docs-wise in 44091077. I removed the stronger claim that callers do not need to match on band type and reworded this to describe the three GDAL nodata representations more directly. I am not adding validation here: we already have a mixed-type test (`test_mem_builder_mixed_owned_and_external_bands`) that uses `Nodata::U64(255)` with a `UInt8` band and round-trips successfully, so this behavior is intentional rather than an unchecked mismatch bug. -- 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]
