paleolimbot commented on code in PR #696:
URL: https://github.com/apache/sedona-db/pull/696#discussion_r2913546722


##########
c/sedona-gdal/src/config.rs:
##########
@@ -0,0 +1,42 @@
+// 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/config.rs>.
+//! Original code is licensed under MIT.
+//!
+//! GDAL configuration option wrappers.
+
+use std::ffi::CString;
+
+use crate::errors::Result;
+use crate::gdal_api::{call_gdal_api, GdalApi};
+
+/// Set a GDAL library configuration option with **thread-local** scope.
+pub fn set_thread_local_config_option(api: &'static GdalApi, key: &str, value: 
&str) -> Result<()> {

Review Comment:
   Perhaps not here, but it might be worth exposing this as 
`with_thread_local_config_option(FnMut...)` that takes care of resetting the 
previous value. At the very least returning the previous value so it can be 
reset may be a good feature of this particular function:
   
   
https://github.com/geopandas/pyogrio/blob/5f298ba31a22654c8fdfa061342df99c2f66b12b/pyogrio/_io.pyx#L202-L233



##########
c/sedona-gdal/src/raster.rs:
##########
@@ -0,0 +1,22 @@
+// 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.
+
+pub mod types;
+
+pub use types::{
+    Buffer, DatasetOptions, GdalDataType, GdalType, RasterCreationOptions, 
ResampleAlg,
+};

Review Comment:
   It seems odd to have these aliases...should the aliases be removed or 
`types` be private?



##########
c/sedona-gdal/src/raster/types.rs:
##########
@@ -0,0 +1,212 @@
+// 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/raster/types.rs>.
+//! Original code is licensed under MIT.
+
+use crate::gdal_dyn_bindgen::{
+    self, GDALDataType, GDALOpenFlags, GDALRIOResampleAlg, GDAL_OF_READONLY, 
GDAL_OF_VERBOSE_ERROR,
+};
+
+/// A Rust-friendly enum mirroring the georust/gdal `GdalDataType` names.
+///
+/// This maps 1-to-1 with [`GDALDataType`] but uses Rust-idiomatic names like 
`UInt8`
+/// instead of `GDT_Byte`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub enum GdalDataType {
+    Unknown,
+    UInt8,
+    Int8,
+    UInt16,
+    Int16,
+    UInt32,
+    Int32,
+    UInt64,
+    Int64,
+    Float32,
+    Float64,
+}
+
+impl GdalDataType {
+    /// Convert from the C-level `GDALDataType` enum.
+    ///
+    /// Returns `None` for complex types and `GDT_TypeCount`.
+    pub fn from_c(c_type: GDALDataType) -> Option<Self> {
+        match c_type {
+            GDALDataType::GDT_Unknown => Some(Self::Unknown),
+            GDALDataType::GDT_Byte => Some(Self::UInt8),
+            GDALDataType::GDT_Int8 => Some(Self::Int8),
+            GDALDataType::GDT_UInt16 => Some(Self::UInt16),
+            GDALDataType::GDT_Int16 => Some(Self::Int16),
+            GDALDataType::GDT_UInt32 => Some(Self::UInt32),
+            GDALDataType::GDT_Int32 => Some(Self::Int32),
+            GDALDataType::GDT_UInt64 => Some(Self::UInt64),
+            GDALDataType::GDT_Int64 => Some(Self::Int64),
+            GDALDataType::GDT_Float32 => Some(Self::Float32),
+            GDALDataType::GDT_Float64 => Some(Self::Float64),
+            _ => None, // Complex types, Float16, TypeCount
+        }
+    }
+
+    /// Convert to the C-level `GDALDataType` enum.
+    pub fn to_c(self) -> GDALDataType {
+        match self {
+            Self::Unknown => GDALDataType::GDT_Unknown,
+            Self::UInt8 => GDALDataType::GDT_Byte,
+            Self::Int8 => GDALDataType::GDT_Int8,
+            Self::UInt16 => GDALDataType::GDT_UInt16,
+            Self::Int16 => GDALDataType::GDT_Int16,
+            Self::UInt32 => GDALDataType::GDT_UInt32,
+            Self::Int32 => GDALDataType::GDT_Int32,
+            Self::UInt64 => GDALDataType::GDT_UInt64,
+            Self::Int64 => GDALDataType::GDT_Int64,
+            Self::Float32 => GDALDataType::GDT_Float32,
+            Self::Float64 => GDALDataType::GDT_Float64,
+        }
+    }
+
+    /// Return the ordinal value compatible with the C API (same as 
`self.to_c() as i32`).
+    pub fn ordinal(self) -> i32 {
+        self.to_c() as i32
+    }
+
+    /// Return the byte size of this data type (0 for Unknown).
+    pub fn byte_size(self) -> usize {
+        match self {
+            Self::Unknown => 0,
+            Self::UInt8 | Self::Int8 => 1,
+            Self::UInt16 | Self::Int16 => 2,
+            Self::UInt32 | Self::Int32 | Self::Float32 => 4,
+            Self::UInt64 | Self::Int64 | Self::Float64 => 8,
+        }
+    }
+}
+
+/// Trait mapping Rust primitive types to GDAL data types.
+pub trait GdalType {
+    fn gdal_ordinal() -> GDALDataType;
+}
+
+macro_rules! impl_gdal_type {
+    ($($ty:ty => $variant:ident),+ $(,)?) => {
+        $(
+            impl GdalType for $ty {
+                fn gdal_ordinal() -> GDALDataType {
+                    GDALDataType::$variant
+                }
+            }
+        )+
+    };
+}
+
+impl_gdal_type! {
+    u8 => GDT_Byte,
+    i8 => GDT_Int8,
+    u16 => GDT_UInt16,
+    i16 => GDT_Int16,
+    u32 => GDT_UInt32,
+    i32 => GDT_Int32,
+    u64 => GDT_UInt64,
+    i64 => GDT_Int64,
+    f32 => GDT_Float32,
+    f64 => GDT_Float64,
+}
+
+/// A 2D raster buffer.
+#[derive(Debug, Clone)]
+pub struct Buffer<T: GdalType> {
+    /// Shape as (cols, rows) — matches georust/gdal convention.
+    pub shape: (usize, usize),
+    pub data: Vec<T>,

Review Comment:
   Is there any future in which we need either (1) a shape larger than 2 
dimensions or (2) non-owned data?



-- 
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]

Reply via email to