feat(sedona-gdal): add convenience facade and mem builder by Kontinuation · Pull Request #697 · apache/sedona-db

Kontinuation · 2026-03-09T16:58:07Z

Summary

add the high-level Gdal facade and with_global_gdal convenience entry point
add the MEM dataset builder on top of the lower-level dataset and raster wrappers
keep the top-level API explicit by importing concrete raster/vector modules instead of relying on wrapper re-export aliases

Depends on #698.

Testing

cargo test -p sedona-gdal
cargo clippy -p sedona-gdal -- -D warnings

Copilot

Pull request overview

This PR introduces a higher-level ergonomics layer for sedona-gdal by adding a Gdal facade and a fluent MEM dataset builder, while keeping the lower-level API available for explicit use.

Changes:

Add Gdal high-level facade wrapping &'static GdalApi to reduce explicit api plumbing at call sites.
Add MemDatasetBuilder for constructing MEM datasets (including zero-copy band attachment plus optional geotransform/projection/nodata).
Export call_gdal_api! as a macro and add with_global_gdal convenience entry point.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
c/sedona-gdal/src/mem.rs	New MEM dataset builder and MEM dataset creation helper + tests.
c/sedona-gdal/src/lib.rs	Exposes the new `gdal` and `mem` modules publicly.
c/sedona-gdal/src/global.rs	Adds `with_global_gdal` convenience wrapper around the global API handle.
c/sedona-gdal/src/gdal.rs	New `Gdal` facade providing ergonomic wrappers for common operations.
c/sedona-gdal/src/gdal_api.rs	Exports `call_gdal_api!` macro (now public/root-exported).

Comments suppressed due to low confidence (1)

c/sedona-gdal/src/gdal_api.rs:48

call_gdal_api is now marked #[macro_export], which makes it part of the public API and exports it at the crate root. However, the macro expands to $api.inner.$func, and inner is pub(crate), so downstream crates cannot actually use the exported macro (privacy check will fail). Consider removing #[macro_export] and keeping it crate-internal (the existing pub(crate) use call_gdal_api; already enables use across this crate without widening the public surface).

#[macro_export]
macro_rules! call_gdal_api {
    ($api:expr, $func:ident $(, $arg:expr)*) => {
        if let Some(func) = $api.inner.$func {
            func($($arg),*)
        } else {
            panic!("{} function not available", stringify!($func))
        }
    };
}

pub(crate) use call_gdal_api;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

paleolimbot

A few optional things that may be worth considering. Thank you!

paleolimbot · 2026-04-01T21:03:10Z

+    /// Add a zero-copy band with custom offsets and optional nodata.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure `data_ptr` points to a valid buffer of sufficient size
+    /// for the given dimensions and offsets, is properly aligned for `data_type`, and
+    /// outlives the built [`Dataset`].
+    pub unsafe fn add_band_with_options(
+        mut self,
+        data_type: GdalDataType,
+        data_ptr: *mut u8,
+        pixel_offset: Option<i64>,
+        line_offset: Option<i64>,
+        nodata: Option<Nodata>,
+    ) -> Self {
+        self.bands.push(MemBand {
+            data_type,
+            data_ptr,
+            pixel_offset,
+            line_offset,
+            nodata,
+        });
+        self
+    }


This is so cool

paleolimbot · 2026-04-01T21:09:16Z

+    /// Fetch GDAL version information for a standard request key.
+    pub fn version_info(&self, request: &str) -> String {


These are all a bit light on the documentation (maybe they could link to the underlying implementation's docs which I seem to remember have a bit more depth?). Also OK to defer since this is mostly intended to be for internal usage.

Added "See also" style pointers to more comprehensive docs.

paleolimbot · 2026-04-01T21:28:37Z

+/// 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 {


I do like the idea of a high-level object that dishes out other safe high-level objects, although putting it all here involves repeating some documentation (or omitting it) and I'm not sure it's needed given that the constructors it refers to are pub anyway. Reexporting the structs or functions that are part of the intended external API might be another way to expose the API without loosing the docs?

This Gdal facade is to avoid passing the api: GdalApi value every time we call a GDAL safe API. I have also found that exposing with_global_gdal instead of with_global_gdal_api makes the users of the gdal crate much easier to discover GDAL APIs, since all almost all GDAL APIs were grouped in the same place instead of scattering in different modules.

paleolimbot · 2026-04-01T21:30:06Z

+    /// Set the dataset projection definition string.
+    pub fn projection(mut self, wkt: impl Into<String>) -> Self {
+        self.projection = Some(wkt.into());


Is this actually WKT or can this by anything? (It may be less error prone to accept anything or an explicit spatialref object)

It does not have to be a WKT. I have renamed the parameter to projection to avoid confusion. It calls GDALSetProjection to set the projection, which handles X/Y axis order automatically. The unique semantics of GDALSetProjection makes it a bit different from GDALSetSpatialRef, and I believe that GDALSetProjection is a more suitable API for sedona-db's axis order convention.

paleolimbot · 2026-04-01T21:36:19Z

+    /// `data_ptr` must point to valid mutable band data that outlives this dataset.
    pub unsafe fn add_band_with_data(
        &self,
        data_type: RustGdalDataType,
-        data_ptr: *const u8,
+        data_ptr: *mut u8,


Is this actually required to be mutable? (This seems like a strange constraint, and also one that will result in it not being particularly useful to us)

*mut u8 is required because this crate exposes safe APIs like RasterBand::write, rasterize, and rasterize_affine that can write through the attached MEM band into the caller-provided backing buffer. If the builder accepted *const u8, safe Rust could later trigger writes into immutable memory.

Actually one of the most important use case of the MEM dataset is for holding the result of rasterize, or the intermediate mask raster as a result of rasterize when running zonal stats. The underlying buffer will be mutated in such use cases.

paleolimbot · 2026-04-01T21:42:41Z

+    pub unsafe fn add_band(self, data_type: GdalDataType, data_ptr: *mut u8) -> Self {
+        self.add_band_with_options(data_type, data_ptr, None, None, None)
+    }


Are you sure the data pointer has to be mutable? (This would preclude backing a GDAL mem dataset with Arrow data?)

paleolimbot

Thank you!

Kontinuation force-pushed the feat/sedona-gdal-safe-facade branch 6 times, most recently from e7e3be7 to 98cbbd2 Compare March 13, 2026 06:38

Kontinuation force-pushed the feat/sedona-gdal-safe-facade branch 2 times, most recently from 4008bc8 to 006695f Compare March 26, 2026 14:37

Kontinuation force-pushed the feat/sedona-gdal-safe-facade branch from 006695f to 43b3ba5 Compare April 1, 2026 05:10

Kontinuation requested a review from Copilot April 1, 2026 05:18

Copilot started reviewing on behalf of Kontinuation April 1, 2026 05:18 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Comment thread c/sedona-gdal/src/mem.rs Outdated

Comment thread c/sedona-gdal/src/gdal.rs

Comment thread c/sedona-gdal/src/mem.rs Outdated

Kontinuation marked this pull request as ready for review April 1, 2026 06:49

paleolimbot approved these changes Apr 2, 2026

View reviewed changes

Kontinuation and others added 13 commits April 11, 2026 20:59

feat(sedona-gdal): add dataset and vector/raster wrappers

6329c27

refactor(sedona-gdal): prefer imported core types

2d7bc66

refactor(sedona-gdal): remove wrapper re-export aliases

d17a45f

fix(sedona-gdal): use last null pointer helper

5c2b188

test(sedona-gdal): add layer iteration coverage

faab8ad

docs(sedona-gdal): tighten wrapper API comments

6f4f8ff

feat(sedona-gdal): add convenience facade and mem builder

22526e3

refactor(sedona-gdal): prefer imported mem helpers

31b38e7

refactor(sedona-gdal): remove facade re-export aliases

14acdcc

docs(sedona-gdal): tighten facade API comments

ef96fd3

refactor(sedona-gdal): use owned dataset constructors

4d5f762

Remove ignored example since the interfaces are subject to change

eaa720c

fix(sedona-gdal): require mutable MEM band buffers

2fbd969

Kontinuation force-pushed the feat/sedona-gdal-safe-facade branch from 4409107 to 2fbd969 Compare April 11, 2026 12:59

Fix review comments

8dd409b

paleolimbot approved these changes Apr 12, 2026

View reviewed changes

Kontinuation merged commit 58c8d65 into apache:main Apr 12, 2026
22 of 23 checks passed

		/// Fetch GDAL version information for a standard request key.
		pub fn version_info(&self, request: &str) -> String {

Conversation

Kontinuation commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kontinuation commented Mar 9, 2026 •

edited

Loading