Fix some obvious problems

Author: kontinuation

c/sedona-gdal/src/gdal.rs

@@ -23,6 +23,7 @@
 //! at every call site.
 
 use crate::config;
+use crate::cpl::CslStringList;
 use crate::dataset::Dataset;
 use crate::driver::{Driver, DriverManager};
 use crate::errors::Result;
@@ -177,7 +178,7 @@ impl Gdal {
         &self,
         path: &str,
         recurse_depth: i32,
-        options: Option<&crate::cpl::CslStringList>,
+        options: Option<&CslStringList>,
     ) -> Result<crate::vsi::VsiDir> {
         crate::vsi::open_dir(self.api, path, recurse_depth, options)
     }

c/sedona-gdal/src/vsi.rs

@@ -24,6 +24,7 @@
 use std::ffi::CString;
 use std::ops::Deref;
 
+use crate::cpl::CslStringList;
 use crate::errors::{GdalError, Result};
 use crate::gdal_api::{call_gdal_api, GdalApi};
 
@@ -147,6 +148,36 @@ pub fn unlink_mem_file(api: &'static GdalApi, file_name: &str) -> Result<()> {
     Ok(())
 }
 
+/// Return the directory separator for the specified path.
+///
+/// Default is forward slash. The only exception currently is the Windows
+/// file system which returns backslash, unless the specified path is of the
+/// form "{drive_letter}:/{rest_of_the_path}".
+///
+/// This function replicates the `VSIGetDirectorySeparator` function of GDAL 3.9+.
+/// We do not call the GDAL function directly since we want to be compatible with older
+/// GDAL versions.
+pub fn directory_separator_for_path(path: &str) -> &'static str {
+    if path.starts_with("http://") || path.starts_with("https://") {
+        "/"
+    } else {
+        #[cfg(windows)]
+        {
+            // Return forward slash for paths of the form
+            // "{drive_letter}:/{rest_of_the_path}", and backslash otherwise.
+            if path.len() >= 3 && path.as_bytes()[1] == b':' && path.as_bytes()[2] == b'/' {
+                "/"
+            } else {
+                "\\"
+            }
+        }
+        #[cfg(not(windows))]
+        {
+            "/"
+        }
+    }
+}
+
 #[cfg(test)]
 pub(crate) fn with_memfile<T>(
     api: &'static GdalApi,
@@ -208,6 +239,80 @@ pub fn get_vsi_mem_file_bytes_owned(api: &'static GdalApi, file_name: &str) -> R
     Ok(buffer.as_ref().to_vec())
 }
 
+pub struct VsiDirEntry {
+    pub name: String,
+    pub mode: Option<i32>,
+    pub size: Option<crate::gdal_dyn_bindgen::vsi_l_offset>,
+    pub mtime: Option<crate::gdal_dyn_bindgen::GIntBig>,
+}
+
+pub struct VsiDir {
+    api: &'static crate::gdal_api::GdalApi,
+    handle: *mut crate::gdal_dyn_bindgen::VSIDIR,
+}
+
+impl VsiDir {
+    pub fn next_entry(&mut self) -> Option<VsiDirEntry> {
+        let entry = unsafe { (self.api.inner.VSIGetNextDirEntry?)(self.handle) };
+        if entry.is_null() {
+            return None;
+        }
+        let entry = unsafe { &*entry };
+
+        let name = if entry.pszName.is_null() {
+            String::new()
+        } else {
+            unsafe { std::ffi::CStr::from_ptr(entry.pszName) }
+                .to_string_lossy()
+                .into_owned()
+        };
+
+        Some(VsiDirEntry {
+            name,
+            mode: (entry.bModeKnown != 0).then_some(entry.nMode),
+            size: (entry.bSizeKnown != 0).then_some(entry.nSize),
+            mtime: (entry.bMTimeKnown != 0).then_some(entry.nMTime),
+        })
+    }
+}
+
+impl Iterator for VsiDir {
+    type Item = VsiDirEntry;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.next_entry()
+    }
+}
+
+impl Drop for VsiDir {
+    fn drop(&mut self) {
+        if !self.handle.is_null() {
+            if let Some(close) = self.api.inner.VSICloseDir {
+                unsafe { close(self.handle) };
+            }
+            self.handle = std::ptr::null_mut();
+        }
+    }
+}
+
+pub fn open_dir(
+    api: &'static crate::gdal_api::GdalApi,
+    path: &str,
+    recurse_depth: i32,
+    options: Option<&CslStringList>,
+) -> crate::errors::Result<VsiDir> {
+    let c_path = std::ffi::CString::new(path)?;
+    let options_ptr: *const *const std::os::raw::c_char = options
+        .map(|opts| opts.as_ptr() as *const *const std::os::raw::c_char)
+        .unwrap_or(std::ptr::null());
+    let handle =
+        unsafe { call_gdal_api!(api, VSIOpenDir, c_path.as_ptr(), recurse_depth, options_ptr) };
+    if handle.is_null() {
+        return Err(api.last_null_pointer_err("VSIOpenDir"));
+    }
+    Ok(VsiDir { api, handle })
+}
+
 #[cfg(all(test, feature = "gdal-sys"))]
 mod tests {
     use super::*;
@@ -300,78 +405,24 @@ mod tests {
         })
         .unwrap();
     }
-}
-
-pub struct VsiDirEntry {
-    pub name: String,
-    pub mode: Option<i32>,
-    pub size: Option<crate::gdal_dyn_bindgen::vsi_l_offset>,
-    pub mtime: Option<crate::gdal_dyn_bindgen::GIntBig>,
-}
-
-pub struct VsiDir {
-    api: &'static crate::gdal_api::GdalApi,
-    handle: *mut crate::gdal_dyn_bindgen::VSIDIR,
-}
 
-impl VsiDir {
-    pub fn next_entry(&mut self) -> Option<VsiDirEntry> {
-        let entry = unsafe { (self.api.inner.VSIGetNextDirEntry?)(self.handle) };
-        if entry.is_null() {
-            return None;
+    #[test]
+    fn test_directory_separator_for_path() {
+        #[cfg(windows)]
+        {
+            assert_eq!(directory_separator_for_path("/vsis3/bucket/prefix"), r"\");
+            assert_eq!(directory_separator_for_path("https://host/data.tif"), "/");
+            assert_eq!(directory_separator_for_path(r"C:\data\dir"), r"\");
+            assert_eq!(directory_separator_for_path(r"C:/data/dir"), "/");
+            assert_eq!(directory_separator_for_path("/tmp/data"), r"\");
         }
-        let entry = unsafe { &*entry };
-
-        let name = if entry.pszName.is_null() {
-            String::new()
-        } else {
-            unsafe { std::ffi::CStr::from_ptr(entry.pszName) }
-                .to_string_lossy()
-                .into_owned()
-        };
-
-        Some(VsiDirEntry {
-            name,
-            mode: (entry.bModeKnown != 0).then_some(entry.nMode),
-            size: (entry.bSizeKnown != 0).then_some(entry.nSize),
-            mtime: (entry.bMTimeKnown != 0).then_some(entry.nMTime),
-        })
-    }
-}
-
-impl Iterator for VsiDir {
-    type Item = VsiDirEntry;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        self.next_entry()
-    }
-}
-
-impl Drop for VsiDir {
-    fn drop(&mut self) {
-        if !self.handle.is_null() {
-            if let Some(close) = self.api.inner.VSICloseDir {
-                unsafe { close(self.handle) };
-            }
-            self.handle = std::ptr::null_mut();
+        #[cfg(not(windows))]
+        {
+            assert_eq!(directory_separator_for_path("/vsis3/bucket/prefix"), "/");
+            assert_eq!(directory_separator_for_path("https://host/data.tif"), "/");
+            assert_eq!(directory_separator_for_path(r"C:\data\dir"), "/");
+            assert_eq!(directory_separator_for_path(r"C:/data/dir"), "/");
+            assert_eq!(directory_separator_for_path("/tmp/data"), "/");
         }
     }
 }
-
-pub fn open_dir(
-    api: &'static crate::gdal_api::GdalApi,
-    path: &str,
-    recurse_depth: i32,
-    options: Option<&crate::cpl::CslStringList>,
-) -> crate::errors::Result<VsiDir> {
-    let c_path = std::ffi::CString::new(path)?;
-    let options_ptr: *const *const std::os::raw::c_char = options
-        .map(|opts| opts.as_ptr() as *const *const std::os::raw::c_char)
-        .unwrap_or(std::ptr::null());
-    let handle =
-        unsafe { (api.inner.VSIOpenDir.unwrap())(c_path.as_ptr(), recurse_depth, options_ptr) };
-    if handle.is_null() {
-        return Err(api.last_null_pointer_err("VSIOpenDir"));
-    }
-    Ok(VsiDir { api, handle })
-}

rust/sedona-geoparquet/src/statistics_accumulator.rs

@@ -255,6 +255,7 @@ impl GeoStatsAccumulator for GeographyGeoStatsAccumulator {
 #[cfg(test)]
 mod test {
     use super::*;
+    #[cfg(feature = "s2geography")]
     use parquet::geospatial::bounding_box::BoundingBox;
     use sedona_schema::datatypes::{WKB_GEOGRAPHY, WKB_VIEW_GEOGRAPHY};
     use sedona_testing::create::create_scalar;

rust/sedona-raster-gdal/src/rs_geotiff_tiles.rs

@@ -24,7 +24,7 @@ use std::sync::Arc;
 use arrow_array::{builder::StringBuilder, builder::UInt32Builder, ArrayRef, RecordBatch};
 use arrow_schema::{DataType, Field, Schema, SchemaRef};
 use async_trait::async_trait;
-use datafusion::catalog::TableFunctionImpl;
+use datafusion::catalog::{Session, TableFunctionImpl, TableProvider};
 use datafusion::execution::context::TaskContext;
 use datafusion::physical_plan::execution_plan::{Boundedness, EmissionType};
 use datafusion::physical_plan::expressions::Column;
@@ -45,6 +45,7 @@ use datafusion_common_runtime::SpawnedTask;
 use futures::{StreamExt, TryStreamExt};
 use sedona_gdal::gdal_dyn_bindgen::{VSI_S_IFMT, VSI_S_IFREG};
 use sedona_gdal::spatial_ref::SpatialRef;
+use sedona_gdal::vsi::directory_separator_for_path;
 use sedona_raster::builder::RasterBuilder;
 use sedona_raster::traits::{BandMetadata, RasterMetadata};
 use sedona_schema::raster::StorageType;
@@ -62,7 +63,7 @@ pub fn rs_geotiff_tiles_udtf() -> Arc<dyn TableFunctionImpl> {
 pub struct RsGeoTiffTilesFunction {}
 
 impl TableFunctionImpl for RsGeoTiffTilesFunction {
-    fn call(&self, exprs: &[Expr]) -> Result<Arc<dyn datafusion::catalog::TableProvider>> {
+    fn call(&self, exprs: &[Expr]) -> Result<Arc<dyn TableProvider>> {
         if exprs.is_empty() || exprs.len() > 2 {
             return plan_err!(
                 "rs_geotiff_tiles() expected 1 or 2 arguments (path[, recursive]) but got {}",
@@ -124,7 +125,7 @@ impl GeoTiffTilesProvider {
 }
 
 #[async_trait]
-impl datafusion::catalog::TableProvider for GeoTiffTilesProvider {
+impl TableProvider for GeoTiffTilesProvider {
     fn as_any(&self) -> &dyn Any {
         self
     }
@@ -134,12 +135,12 @@ impl datafusion::catalog::TableProvider for GeoTiffTilesProvider {
     }
 
     fn table_type(&self) -> TableType {
-        TableType::View
+        TableType::Base
     }
 
     async fn scan(
         &self,
-        _state: &dyn datafusion::catalog::Session,
+        _state: &dyn Session,
         projection: Option<&Vec<usize>>,
         _filters: &[Expr],
         _limit: Option<usize>,
@@ -178,6 +179,7 @@ impl GeoTiffTilesExec {
     fn new(dir: String, recursive: bool, schema: SchemaRef) -> Self {
         let properties = PlanProperties::new(
             EquivalenceProperties::new(schema.clone()),
+            // TODO: allow split paths to load into multiple partitions to enable parallelism.
             Partitioning::UnknownPartitioning(1),
             EmissionType::Incremental,
             Boundedness::Bounded,
@@ -232,9 +234,13 @@ impl ExecutionPlan for GeoTiffTilesExec {
 
     fn execute(
         &self,
-        _partition: usize,
+        partition: usize,
         _context: Arc<TaskContext>,
     ) -> Result<datafusion::physical_plan::SendableRecordBatchStream> {
+        if partition != 0 {
+            return exec_err!("GeoTiffTilesExec only has one partition (partition 0) but got partition {partition}");
+        }
+
         let schema_worker = self.schema.clone();
         let schema_empty = self.schema.clone();
         let schema_adapter = self.schema.clone();
@@ -248,7 +254,7 @@ impl ExecutionPlan for GeoTiffTilesExec {
                 let schema = schema_worker.clone();
                 SpawnedTask::spawn_blocking(move || build_batch_for_file(path, schema))
             })
-            .buffered(4)
+            .buffered(2)
             .map(move |res| match res {
                 Ok(Ok(Some(batch))) => Ok(batch),
                 Ok(Ok(None)) => Ok(RecordBatch::new_empty(schema_empty.clone())),
@@ -462,16 +468,6 @@ fn list_geotiffs(path: &str, recursive: bool) -> Result<Vec<String>> {
     }
 }
 
-fn directory_separator_for_path(path: &str) -> &'static str {
-    if path.starts_with("/vsi") || path.contains("://") {
-        "/"
-    } else if path.contains('\\') {
-        "\\"
-    } else {
-        "/"
-    }
-}
-
 fn join_vsi_path(base: &str, separator: &str, child_name: &str) -> String {
     if base.ends_with(separator) {
         format!("{base}{child_name}")
@@ -506,11 +502,11 @@ fn div_ceil_u32(n: u32, d: u32) -> u32 {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use datafusion::catalog::TableProvider;
     use datafusion::prelude::SessionContext;
     use sedona_gdal::raster::types::Buffer;
     use std::path::PathBuf;
     use tempfile::tempdir;
+    use TableProvider;
 
     fn write_test_geotiff(base: &Path, name: &str) -> PathBuf {
         let path = base.join(name);
@@ -589,11 +585,6 @@ mod tests {
         assert!(is_geotiff_path_str("https://host/data.tif?token=abc#f"));
         assert!(!is_geotiff_path_str("/tmp/a.txt"));
         assert!(!is_geotiff_path_str("/tmp/a"));
-
-        assert_eq!(directory_separator_for_path("/vsis3/bucket/prefix"), "/");
-        assert_eq!(directory_separator_for_path("https://host/data.tif"), "/");
-        assert_eq!(directory_separator_for_path(r"C:\data\dir"), r"\");
-        assert_eq!(directory_separator_for_path("/tmp/data"), "/");
     }
 
     #[tokio::test]