From 0e940ae6901cdd8a812bf049d2f9627fce482d84 Mon Sep 17 00:00:00 2001 From: jameswillis Date: Tue, 9 Jun 2026 13:47:49 -0700 Subject: [PATCH 1/8] feat(raster): wire ViewEntries into band construction and reads Integrate the view-spec layer into the raster type. The reader's band() composes a band's view into byte strides + offset (with overflow and buffer-bounds checks) so a non-identity view decodes instead of being rejected; nd_buffer() exposes the strided region and as_contiguous() borrows it zero-copy when packed. The builder gains start_band_with_view and with_view to construct sliced / broadcast / permuted / stacked views. Stacked on the ViewEntries module PR. --- rust/sedona-raster/src/array.rs | 734 +++++++++++++++++++----- rust/sedona-raster/src/builder.rs | 922 +++++++++++++++++++++++++++++- rust/sedona-raster/src/traits.rs | 152 ++++- 3 files changed, 1633 insertions(+), 175 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index 7da4b3a6b4..e9f0727d02 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -22,14 +22,15 @@ use arrow_array::{ use arrow_schema::ArrowError; use crate::traits::{BandRef, Bands, NdBuffer, RasterRef}; -use crate::view_entries::ViewEntry; -use sedona_schema::raster::{band_indices, raster_indices, BandDataType}; +use crate::view_entries::{ViewEntries, ViewEntry}; +use sedona_schema::raster::{band_indices, band_view_indices, raster_indices, BandDataType}; /// Arrow-backed implementation of BandRef for a single band within a raster. /// -/// Today this handles only the canonical identity view: `view_entries` is -/// synthesised from `source_shape`, `visible_shape == source_shape`, -/// and `byte_strides` are plain C-order strides with `byte_offset = 0`. +/// View-derived layout (`visible_shape`, `byte_strides`, `byte_offset`, +/// `is_identity_view`) is computed once at construction and reused by every +/// accessor. Source-shape and dim-name slices are borrowed directly from +/// the underlying Arrow buffers. struct BandRefImpl<'a> { dim_names_list: &'a ListArray, dim_names_values: &'a StringArray, @@ -43,14 +44,20 @@ struct BandRefImpl<'a> { band_row: usize, /// Resolved at construction so accessors don't re-decode the discriminant. data_type: BandDataType, - /// Per-visible-axis view, length = ndim. Always identity today. - view_entries: Vec, - /// Visible shape, length = ndim. Equals `source_shape` today. + /// Per-visible-axis view, length = ndim + view_entries: ViewEntries, + /// Visible shape (== `[v.steps for v in view_entries]`), length = ndim. + /// `i64` to match `BandRef::shape()`'s return type and the surrounding + /// view-machinery arithmetic (strides, offsets). `validate_view` + /// guarantees entries are non-negative. visible_shape: Vec, - /// Byte strides per visible axis. C-order over `source_shape` today. + /// Byte strides per visible axis. May be 0 (broadcast) or negative. byte_strides: Vec, /// Byte offset into `data` of the visible region's `[0,...,0]` element. - byte_offset: u64, + /// Typed `i64` to match the surrounding stride arithmetic + /// (`byte_strides` are `i64` to allow negative steps). Always non-negative + /// by construction — `RasterRefImpl::band` asserts `>= 0` before storing. + byte_offset: i64, } impl<'a> BandRef for BandRefImpl<'a> { @@ -77,7 +84,7 @@ impl<'a> BandRef for BandRefImpl<'a> { } fn view(&self) -> &[ViewEntry] { - &self.view_entries + self.view_entries.as_slice() } fn data_type(&self) -> BandDataType { @@ -136,6 +143,80 @@ impl<'a> BandRef for BandRefImpl<'a> { } } +/// Verify that every byte the view can address lies within `buffer_len` +/// and that every stride × index product (and their accumulations) fits +/// in i64. +/// +/// **Load-bearing**: this is the *only* bound check between the view's +/// byte-stride description and the data buffer. Stride-aware consumers walk +/// the buffer with plain-arithmetic indexing and rely on this precheck +/// having proven every addressed byte is in range. Two corruption modes it +/// catches: +/// +/// 1. A writer that lies about `source_shape` (Arrow column shorter +/// than the view promises). +/// 2. A composed view whose stride × index product or accumulated +/// offset overflows i64 even though `validate_view` accepted the +/// per-entry bounds. +/// +/// Empty visible regions (any axis with `steps == 0`) address no bytes +/// and skip the check. +fn check_view_buffer_bounds( + buffer_len: usize, + visible_shape: &[i64], + byte_strides: &[i64], + byte_offset: i64, + dtype_size: usize, +) -> Result<(), ArrowError> { + if visible_shape.contains(&0) { + return Ok(()); + } + let mut min_offset = byte_offset; + let mut max_offset = byte_offset; + for (k, &stride) in byte_strides.iter().enumerate() { + // `validate_view` guarantees `steps >= 0`, so `visible_shape[k] >= 0` + // and `visible_shape[k] - 1` is in-range for any non-empty axis. + let last_idx = visible_shape[k] - 1; + let contribution = last_idx.checked_mul(stride).ok_or_else(|| { + ArrowError::InvalidArgumentError(format!( + "max addressable offset on axis {k} overflows i64" + )) + })?; + if contribution > 0 { + max_offset = max_offset.checked_add(contribution).ok_or_else(|| { + ArrowError::InvalidArgumentError( + "max addressable offset accumulation overflows i64".to_string(), + ) + })?; + } else if contribution < 0 { + min_offset = min_offset.checked_add(contribution).ok_or_else(|| { + ArrowError::InvalidArgumentError( + "min addressable offset accumulation overflows i64".to_string(), + ) + })?; + } + } + let last_byte = max_offset + .checked_add(dtype_size as i64 - 1) + .ok_or_else(|| { + ArrowError::InvalidArgumentError("max addressable byte overflows i64".to_string()) + })?; + if min_offset < 0 { + return Err(ArrowError::InvalidArgumentError(format!( + "view addresses out-of-bounds negative byte offset {min_offset}" + ))); + } + let buffer_len_i64 = i64::try_from(buffer_len).map_err(|_| { + ArrowError::InvalidArgumentError(format!("buffer length {buffer_len} exceeds i64::MAX")) + })?; + if last_byte >= buffer_len_i64 { + return Err(ArrowError::InvalidArgumentError(format!( + "view addresses byte {last_byte} but buffer is only {buffer_len} bytes" + ))); + } + Ok(()) +} + /// Arrow-backed implementation of RasterRef for a single raster row. /// /// Holds flat references to the underlying Arrow arrays so the impl does @@ -160,6 +241,10 @@ pub struct RasterRefImpl<'a> { band_datatype_array: &'a UInt32Array, band_nodata_array: &'a BinaryArray, band_view_list: &'a ListArray, + band_view_source_axis: &'a Int64Array, + band_view_start: &'a Int64Array, + band_view_step: &'a Int64Array, + band_view_steps: &'a Int64Array, band_outdb_uri_array: &'a StringArray, band_outdb_format_array: &'a StringViewArray, band_data_array: &'a BinaryViewArray, @@ -175,6 +260,124 @@ impl<'a> RasterRefImpl<'a> { Some(self.crs_array.value(self.raster_index)) } } + + /// Read the band's source_shape and convert u64 → i64 with overflow check. + /// + /// Rejects 0-D bands (empty source_shape) at the read boundary: the schema + /// doesn't forbid them outright but every consumer assumes ndim >= 1. Every + /// downstream consumer in the view machinery wants i64 (matches ViewEntry's + /// signed fields and the stride arithmetic); converting once here keeps the + /// rest of band() free of mixed-signedness gymnastics. + fn read_band_source_shape(&self, band_row: usize) -> Result, ArrowError> { + let ss_start = self.band_source_shape_list.value_offsets()[band_row] as usize; + let ss_end = self.band_source_shape_list.value_offsets()[band_row + 1] as usize; + let source_shape: &[i64] = &self.band_source_shape_values.values()[ss_start..ss_end]; + + if source_shape.is_empty() { + return Err(ArrowError::ExternalError(Box::new( + sedona_common::sedona_internal_datafusion_err!( + "band {band_row} has empty source_shape; ndim must be >= 1" + ), + ))); + } + + Ok(source_shape.to_vec()) + } + + /// Resolve the band's data-type discriminant or fail. An unknown + /// discriminant is schema-corruption, not user data. + fn read_band_data_type_or_err(&self, band_row: usize) -> Result { + let data_type_value = self.band_datatype_array.value(band_row); + BandDataType::try_from_u32(data_type_value).ok_or_else(|| { + ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( + "band {band_row} has unknown data_type discriminant {data_type_value}" + ))) + }) + } + + /// Read the band's view-entry list. Identity is encoded exclusively as a + /// NULL row — an empty (non-null, zero-length) list is malformed and + /// rejected later by [`ViewEntries::validate`]. The schema (see + /// `RasterSchema::view_type`) documents this contract. + fn read_band_view_entries( + &self, + band_row: usize, + source_shape: &[i64], + ) -> Result { + if self.band_view_list.is_null(band_row) { + return Ok(ViewEntries::identity_for_shape(source_shape)); + } + let v_start = self.band_view_list.value_offsets()[band_row] as usize; + let v_end = self.band_view_list.value_offsets()[band_row + 1] as usize; + Ok(ViewEntries::new( + (v_start..v_end) + .map(|i| ViewEntry { + source_axis: self.band_view_source_axis.value(i), + start: self.band_view_start.value(i), + step: self.band_view_step.value(i), + steps: self.band_view_steps.value(i), + }) + .collect(), + )) + } +} + +/// Compose a validated view against a source shape into C-order byte strides +/// and a byte offset. +/// +/// C-order source strides are dtype-scaled cumulative products of `source_shape`, +/// then each visible axis's stride/offset is composed as `view.step * +/// src_stride` / `view.start * src_stride`. All arithmetic is checked: even +/// after `ViewEntries::validate`, the cumulative byte product can overflow +/// `i64` for cosmically large shapes, and a corrupt source_shape whose product +/// wraps would otherwise silently pass downstream bound checks. The returned +/// `byte_offset` is non-negative by construction (start >= 0, src_stride > 0); +/// the defensive sign check guards future refactors that might break that +/// invariant before we cross the i64 → u64 boundary in `nd_buffer()`. +fn compose_byte_strides( + band_row: usize, + source_shape: &[i64], + view_entries: &ViewEntries, + dtype_byte_size: usize, +) -> Result<(Vec, i64), ArrowError> { + let overflow_err = |msg: &str| { + ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( + "band {band_row}: {msg}" + ))) + }; + + let dtype_size = dtype_byte_size as i64; + + let mut source_strides_bytes = vec![0i64; source_shape.len()]; + source_strides_bytes[source_shape.len() - 1] = dtype_size; + for k in (0..source_shape.len() - 1).rev() { + source_strides_bytes[k] = source_strides_bytes[k + 1] + .checked_mul(source_shape[k + 1]) + .ok_or_else(|| overflow_err("source-stride product overflows i64"))?; + } + + let mut byte_strides = vec![0i64; view_entries.len()]; + let mut byte_offset: i64 = 0; + for (k, v) in view_entries.iter().enumerate() { + let src_stride = source_strides_bytes[v.source_axis as usize]; + byte_strides[k] = v + .step + .checked_mul(src_stride) + .ok_or_else(|| overflow_err("view step × source-stride overflows i64"))?; + let start_off = v + .start + .checked_mul(src_stride) + .ok_or_else(|| overflow_err("view start × source-stride overflows i64"))?; + byte_offset = byte_offset + .checked_add(start_off) + .ok_or_else(|| overflow_err("view offset accumulation overflows i64"))?; + } + + if byte_offset < 0 { + return Err(overflow_err("composed byte_offset is negative")); + } + + Ok((byte_strides, byte_offset)) } impl<'a> RasterRef for RasterRefImpl<'a> { @@ -196,67 +399,44 @@ impl<'a> RasterRef for RasterRefImpl<'a> { let start = self.bands_list.value_offsets()[self.raster_index] as usize; let band_row = start + index; - // Read source shape slice. - let ss_start = self.band_source_shape_list.value_offsets()[band_row] as usize; - let ss_end = self.band_source_shape_list.value_offsets()[band_row + 1] as usize; - let source_shape: &[i64] = &self.band_source_shape_values.values()[ss_start..ss_end]; - - // Reject 0-D bands at the read boundary. Schema doesn't forbid them - // outright but every consumer assumes ndim >= 1. - if source_shape.is_empty() { - return Err(ArrowError::ExternalError(Box::new( - sedona_common::sedona_internal_datafusion_err!( - "band {band_row} has empty source_shape; ndim must be >= 1" - ), - ))); - } - - // Resolve data type up front; an unknown discriminant is a - // schema-corruption bug, not user data, so failing the band loudly - // here is appropriate. - let data_type_value = self.band_datatype_array.value(band_row); - let data_type = BandDataType::try_from_u32(data_type_value).ok_or_else(|| { + let source_shape = self.read_band_source_shape(band_row)?; + let data_type = self.read_band_data_type_or_err(band_row)?; + let view_entries = self.read_band_view_entries(band_row, &source_shape)?; + view_entries.validate(&source_shape).map_err(|e| { ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( - "band {band_row} has unknown data_type discriminant {data_type_value}" + "band {band_row} has malformed view: {e}" ))) })?; - // Only the canonical identity view (null view row) is written today. - // A non-null view row would require the view → byte-stride composition - // path, which is not yet implemented. Surface it loudly here rather - // than silently rejecting the band, so callers see the standardised - // SedonaDB-internal-error framing. - // - // This rejection is also the guardrail keeping `RS_EnsureLoaded` - // correct: it drops `view()` on rebuild, so it would corrupt a - // viewed band. When this comes off (view composition), the loader - // request/response must round-trip the view — tracked in - // . - if !self.band_view_list.is_null(band_row) { - return Err(ArrowError::ExternalError(Box::new( - sedona_common::sedona_internal_datafusion_err!( - "non-null view row at band {band_row}: view composition is not yet implemented" - ), - ))); - } - let view_entries: Vec = source_shape - .iter() - .enumerate() - .map(|(i, &s)| ViewEntry { - source_axis: i as i64, - start: 0, - step: 1, - steps: s, - }) - .collect(); - - let visible_shape: Vec = source_shape.to_vec(); - - let dtype_size = data_type.byte_size() as i64; - let mut byte_strides = vec![0i64; source_shape.len()]; - byte_strides[source_shape.len() - 1] = dtype_size; - for k in (0..source_shape.len() - 1).rev() { - byte_strides[k] = byte_strides[k + 1] * source_shape[k + 1]; + let visible_shape = view_entries.visible_shape(); + let (byte_strides, byte_offset) = compose_byte_strides( + band_row, + &source_shape, + &view_entries, + data_type.byte_size(), + )?; + + // For InDb bands, verify the underlying data column is long enough + // to cover every byte the view can address. The view-machinery + // validation above doesn't know the actual `data` BinaryView + // length — a writer that lies about source_shape vs the bytes + // written would otherwise slip through and panic later when a + // consumer walks the strided buffer. OutDb bands skip this check: + // their data column is empty by design. + let data_bytes = self.band_data_array.value(band_row); + if !data_bytes.is_empty() { + check_view_buffer_bounds( + data_bytes.len(), + &visible_shape, + &byte_strides, + byte_offset, + data_type.byte_size(), + ) + .map_err(|e| { + ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( + "band {band_row}: view-buffer bounds check failed: {e}" + ))) + })?; } Ok(Box::new(BandRefImpl { @@ -273,7 +453,7 @@ impl<'a> RasterRef for RasterRefImpl<'a> { view_entries, visible_shape, byte_strides, - byte_offset: 0, + byte_offset, })) } @@ -395,6 +575,10 @@ pub struct RasterStructArray<'a> { band_datatype_array: &'a UInt32Array, band_nodata_array: &'a BinaryArray, band_view_list: &'a ListArray, + band_view_source_axis: &'a Int64Array, + band_view_start: &'a Int64Array, + band_view_step: &'a Int64Array, + band_view_steps: &'a Int64Array, band_outdb_uri_array: &'a StringArray, band_outdb_format_array: &'a StringViewArray, band_data_array: &'a BinaryViewArray, @@ -494,6 +678,31 @@ impl<'a> RasterStructArray<'a> { .as_any() .downcast_ref::() .unwrap(); + let band_view_struct = band_view_list + .values() + .as_any() + .downcast_ref::() + .unwrap(); + let band_view_source_axis = band_view_struct + .column(band_view_indices::SOURCE_AXIS) + .as_any() + .downcast_ref::() + .unwrap(); + let band_view_start = band_view_struct + .column(band_view_indices::START) + .as_any() + .downcast_ref::() + .unwrap(); + let band_view_step = band_view_struct + .column(band_view_indices::STEP) + .as_any() + .downcast_ref::() + .unwrap(); + let band_view_steps = band_view_struct + .column(band_view_indices::STEPS) + .as_any() + .downcast_ref::() + .unwrap(); let band_outdb_uri_array = bands_struct .column(band_indices::OUTDB_URI) .as_any() @@ -528,6 +737,10 @@ impl<'a> RasterStructArray<'a> { band_datatype_array, band_nodata_array, band_view_list, + band_view_source_axis, + band_view_start, + band_view_step, + band_view_steps, band_outdb_uri_array, band_outdb_format_array, band_data_array, @@ -571,6 +784,10 @@ impl<'a> RasterStructArray<'a> { band_datatype_array: self.band_datatype_array, band_nodata_array: self.band_nodata_array, band_view_list: self.band_view_list, + band_view_source_axis: self.band_view_source_axis, + band_view_start: self.band_view_start, + band_view_step: self.band_view_step, + band_view_steps: self.band_view_steps, band_outdb_uri_array: self.band_outdb_uri_array, band_outdb_format_array: self.band_outdb_format_array, band_data_array: self.band_data_array, @@ -604,11 +821,12 @@ impl<'a> RasterStructArray<'a> { #[cfg(test)] mod tests { use super::*; - use crate::builder::RasterBuilder; + use crate::builder::{RasterBuilder, StartBandWithViewArgs}; use crate::traits::{BandMetadata, RasterMetadata}; - use arrow_array::{ArrayRef, ListArray, StructArray, UInt32Array}; - use arrow_buffer::{OffsetBuffer, ScalarBuffer}; - use arrow_schema::{DataType, Fields}; + use crate::view_entries::ViewEntry; + use arrow_array::{types::Int64Type, ArrayRef, ListArray, StructArray, UInt32Array}; + use arrow_buffer::{NullBuffer, OffsetBuffer, ScalarBuffer}; + use arrow_schema::{DataType, Field, Fields}; use sedona_schema::raster::{ band_indices, raster_indices, BandDataType, RasterSchema, StorageType, }; @@ -776,20 +994,36 @@ mod tests { assert!(rasters.is_null(1)); } - /// Build a single-raster, single-band raster StructArray with the - /// canonical identity view. Used as the baseline input to the surgery - /// helpers below; callers replace one band-level column to simulate - /// schema corruption on non-view fields. - fn build_identity_raster() -> StructArray { + /// Build a single-raster, single-band raster StructArray with an explicit + /// view. Used as the input to the surgery helpers below; callers replace + /// one band-level column to simulate schema corruption. + fn build_explicit_view_raster() -> StructArray { let mut builder = RasterBuilder::new(1); let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; builder .start_raster_nd(&transform, &["x"], &[3], None) .unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }]; builder - .start_band_nd(None, &["x"], &[3], BandDataType::UInt8, None, None, None) + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[8], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) .unwrap(); - builder.band_data_writer().append_value(vec![0u8, 1, 2]); + builder + .band_data_writer() + .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); builder.finish_band().unwrap(); builder.finish_raster().unwrap(); builder.finish().unwrap() @@ -842,11 +1076,73 @@ mod tests { ) } - // bad data_type discriminant + /// Rebuild the band view list with hand-rolled entries. `entries[i]` + /// supplies all four `(source_axis, start, step, steps)` Int64 values + /// for band-row `i`. `nulls` controls per-row validity bits — `None` + /// means every row is non-null. + fn make_band_view_list( + entries: Vec>, + nulls: Option>, + ) -> ArrayRef { + let mut offsets: Vec = vec![0]; + let mut sa: Vec = vec![]; + let mut start: Vec = vec![]; + let mut step: Vec = vec![]; + let mut steps: Vec = vec![]; + for row in &entries { + for &(a, s, k, n) in row { + sa.push(a); + start.push(s); + step.push(k); + steps.push(n); + } + offsets.push(sa.len() as i32); + } + let view_struct_fields = Fields::from(vec![ + Field::new("source_axis", DataType::Int64, false), + Field::new("start", DataType::Int64, false), + Field::new("step", DataType::Int64, false), + Field::new("steps", DataType::Int64, false), + ]); + let view_struct = StructArray::new( + view_struct_fields, + vec![ + Arc::new(arrow_array::PrimitiveArray::::from(sa)) as ArrayRef, + Arc::new(arrow_array::PrimitiveArray::::from(start)) as ArrayRef, + Arc::new(arrow_array::PrimitiveArray::::from(step)) as ArrayRef, + Arc::new(arrow_array::PrimitiveArray::::from(steps)) as ArrayRef, + ], + None, + ); + let DataType::List(view_field) = RasterSchema::view_type() else { + unreachable!() + }; + let null_buf = nulls.map(NullBuffer::from); + Arc::new(ListArray::new( + view_field, + OffsetBuffer::new(ScalarBuffer::from(offsets)), + Arc::new(view_struct), + null_buf, + )) + } + + // ---- Critical #1: malformed view entries ---- + + #[test] + fn band_returns_none_when_view_length_mismatches_source_shape() { + // source_shape has 1 dim but view encodes 2 entries. + let array = build_explicit_view_raster(); + let bad_view = make_band_view_list(vec![vec![(0, 0, 1, 3), (0, 0, 1, 3)]], None); + let mutated = replace_band_column(&array, band_indices::VIEW, bad_view); + let rasters = RasterStructArray::new(&mutated); + assert!(rasters.get(0).unwrap().band(0).is_err()); + } + + // ---- Critical #2: bad data_type discriminant ---- #[test] fn band_and_band_data_type_surface_corruption_for_unknown_discriminant() { - let array = build_identity_raster(); + let array = build_explicit_view_raster(); let bad_dtype: ArrayRef = Arc::new(UInt32Array::from(vec![0xFFu32])); let mutated = replace_band_column(&array, band_indices::DATA_TYPE, bad_dtype); let rasters = RasterStructArray::new(&mutated); @@ -868,7 +1164,7 @@ mod tests { #[test] fn band_surfaces_internal_error_when_source_shape_is_empty() { - let array = build_identity_raster(); + let array = build_explicit_view_raster(); // Replace source_shape with a single empty list row. let DataType::List(ss_field) = RasterSchema::source_shape_type() else { unreachable!() @@ -890,7 +1186,100 @@ mod tests { assert!(err.to_string().contains("empty source_shape")); } - // direct fast-path tests + #[test] + fn band_surfaces_internal_error_when_data_column_shorter_than_view() { + // build_explicit_view_raster writes 8 UInt8 source bytes with view + // (start=1, step=2, steps=3) which addresses bytes 1, 3, 5. + // Inflate source_shape to [16] and the view to cover steps=10 along + // the (now nominally-larger) source axis: the byte range jumps past + // the actual 8-byte data column and the precheck must fire. + let array = build_explicit_view_raster(); + // source_shape := [16] + let new_source_shape = make_band_source_shape_list(vec![vec![16i64]]); + let mutated_ss = replace_band_column(&array, band_indices::SOURCE_SHAPE, new_source_shape); + // view := (source_axis=0, start=0, step=1, steps=10) — addresses + // bytes 0..10 but the underlying data column only has 8 bytes. + let new_view = make_band_view_list(vec![vec![(0, 0, 1, 10)]], None); + let mutated = replace_band_column(&mutated_ss, band_indices::VIEW, new_view); + let rasters = RasterStructArray::new(&mutated); + let err = rasters.get(0).unwrap().band(0).err().unwrap(); + assert!(err.to_string().contains("SedonaDB internal error")); + assert!(err.to_string().contains("view-buffer bounds check failed")); + } + + #[test] + fn band_rejects_empty_non_null_view_row() { + // The identity view is encoded exclusively as a NULL row; a + // non-null zero-length list is malformed and must error rather + // than silently fall back to identity. (Pre-rev behaviour + // accepted it — see `RasterSchema::view_type` for the contract.) + let array = build_explicit_view_raster(); + let empty_non_null_view = make_band_view_list(vec![vec![]], Some(vec![true])); + let mutated = replace_band_column(&array, band_indices::VIEW, empty_non_null_view); + let rasters = RasterStructArray::new(&mutated); + let err = rasters.get(0).unwrap().band(0).err().unwrap(); + assert!(err.to_string().contains("view length"), "got: {err}"); + } + + // ---- Stride composition overflow guards ---- + + /// Build a band source_shape list with hand-rolled i64 entries so tests + /// can inject values that the builder's writer-side checks would refuse. + fn make_band_source_shape_list(rows: Vec>) -> ArrayRef { + let mut offsets: Vec = vec![0]; + let mut values: Vec = vec![]; + for row in &rows { + values.extend_from_slice(row); + offsets.push(values.len() as i32); + } + let DataType::List(field) = RasterSchema::source_shape_type() else { + unreachable!() + }; + Arc::new(ListArray::new( + field, + OffsetBuffer::new(ScalarBuffer::from(offsets)), + Arc::new(Int64Array::from(values)), + None, + )) + } + + #[test] + fn band_returns_none_when_source_strides_product_overflows() { + // dtype_size × Π source_shape[j>k] must not silently wrap. With a + // 3-D source_shape of `[1, 1<<32, 1<<32]` the product (1<<32) × + // (1<<32) = 1<<64 overflows i64 in the source-stride build. + let array = build_explicit_view_raster(); + let new_source_shape = + make_band_source_shape_list(vec![vec![1i64, 1i64 << 32, 1i64 << 32]]); + let mutated_ss = replace_band_column(&array, band_indices::SOURCE_SHAPE, new_source_shape); + // Pad the view to 3 entries; steps=0 on the giant axes keeps + // validate_view's start/last checks out of the casts-from-u64 path. + let new_view = + make_band_view_list(vec![vec![(0, 0, 1, 1), (1, 0, 1, 0), (2, 0, 1, 0)]], None); + let mutated = replace_band_column(&mutated_ss, band_indices::VIEW, new_view); + let rasters = RasterStructArray::new(&mutated); + assert!(rasters.get(0).unwrap().band(0).is_err()); + } + + #[test] + fn band_returns_none_when_view_step_times_source_stride_overflows() { + // `validate_view` bounds (steps-1)*step + start on the SOURCE axis + // but doesn't bound v.step × cumulative_byte_stride. A view with a + // small visible region but a step large enough to wrap the byte + // stride must be rejected at construction. + // + // Source `[3, 1<<60]`, dtype_size=1 (UInt8) → src_stride[0] = 1<<60. + // View on axis 0 with step=8 makes byte_strides[0] = 8 × (1<<60) = + // 1<<63 which overflows i64. The view itself only walks 1 step on + // that axis so validate_view's (steps-1)*step bound holds. + let array = build_explicit_view_raster(); + let new_source_shape = make_band_source_shape_list(vec![vec![3i64, 1i64 << 60]]); + let mutated_ss = replace_band_column(&array, band_indices::SOURCE_SHAPE, new_source_shape); + let new_view = make_band_view_list(vec![vec![(0, 0, 8, 1), (1, 0, 1, 1)]], None); + let mutated = replace_band_column(&mutated_ss, band_indices::VIEW, new_view); + let rasters = RasterStructArray::new(&mutated); + assert!(rasters.get(0).unwrap().band(0).is_err()); + } #[test] fn raster_ref_fast_paths_return_expected_values() { @@ -991,18 +1380,17 @@ mod tests { assert!(bm0.outdb_band_id().is_none()); } - // multi-band, multi-raster identity + // ---- Important #9: multi-band, multi-raster mixed identity/explicit ---- #[test] - fn multi_raster_identity_views() { - // Two rasters with multiple identity bands each. Exercises the - // `bands_list.value_offsets()` routing for every per-band lookup — - // a naive reader that forgets to add the per-raster offset would - // hand back data from the wrong band. + fn multi_raster_mixed_identity_and_explicit_views() { + // Two rasters. Raster 0 has 3 bands (identity, explicit slice, + // identity). Raster 1 has 2 bands (explicit broadcast, identity). + // bands_list.value_offsets() must correctly route each band. let mut builder = RasterBuilder::new(2); let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - // Raster 0: three identity bands. + // Raster 0 builder .start_raster_nd(&transform, &["x"], &[3], None) .unwrap(); @@ -1012,9 +1400,25 @@ mod tests { builder.band_data_writer().append_value(vec![10u8, 20, 30]); builder.finish_band().unwrap(); builder - .start_band_nd(None, &["x"], &[3], BandDataType::UInt8, None, None, None) + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[8], + view: &[ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }], + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) .unwrap(); - builder.band_data_writer().append_value(vec![40u8, 50, 60]); + builder + .band_data_writer() + .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); builder.finish_band().unwrap(); builder .start_band_nd(None, &["x"], &[3], BandDataType::UInt8, None, None, None) @@ -1025,16 +1429,28 @@ mod tests { builder.finish_band().unwrap(); builder.finish_raster().unwrap(); - // Raster 1: two identity bands of a different shape. + // Raster 1 builder .start_raster_nd(&transform, &["x"], &[4], None) .unwrap(); builder - .start_band_nd(None, &["x"], &[4], BandDataType::UInt8, None, None, None) + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[1], + view: &[ViewEntry { + source_axis: 0, + start: 0, + step: 0, + steps: 4, + }], + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) .unwrap(); - builder - .band_data_writer() - .append_value(vec![42u8, 43, 44, 45]); + builder.band_data_writer().append_value(vec![42u8]); builder.finish_band().unwrap(); builder .start_band_nd(None, &["x"], &[4], BandDataType::UInt8, None, None, None) @@ -1046,61 +1462,39 @@ mod tests { let array = builder.finish().unwrap(); let rasters = RasterStructArray::new(&array); + // Raster 0 bands: identity (3), slice (3), identity (3). The identity + // bands are contiguous and borrow zero-copy; the step=2 slice is + // strided so `as_contiguous` rejects it. let r0 = rasters.get(0).unwrap(); assert_eq!(r0.num_bands(), 3); assert_eq!(r0.band(0).unwrap().shape(), &[3]); - assert_eq!( - r0.band(0) - .unwrap() - .nd_buffer() - .unwrap() - .as_contiguous() - .unwrap(), - &[10u8, 20, 30] - ); + let b0 = r0.band(0).unwrap(); + let nd0 = b0.nd_buffer().unwrap(); + assert_eq!(nd0.as_contiguous().unwrap(), &[10u8, 20, 30]); assert_eq!(r0.band(1).unwrap().shape(), &[3]); - assert_eq!( - r0.band(1) - .unwrap() - .nd_buffer() - .unwrap() - .as_contiguous() - .unwrap(), - &[40u8, 50, 60] - ); + let b1 = r0.band(1).unwrap(); + let nd1 = b1.nd_buffer().unwrap(); + assert!(!nd1.is_contiguous()); + assert!(nd1.as_contiguous().is_err()); assert_eq!(r0.band(2).unwrap().shape(), &[3]); - assert_eq!( - r0.band(2) - .unwrap() - .nd_buffer() - .unwrap() - .as_contiguous() - .unwrap(), - &[100u8, 101, 102] - ); + let b2 = r0.band(2).unwrap(); + let nd2 = b2.nd_buffer().unwrap(); + assert_eq!(nd2.as_contiguous().unwrap(), &[100u8, 101, 102]); + // Raster 1 bands: broadcast (4 copies of 42), identity (4). The + // broadcast band has a zero stride so it is non-contiguous and + // rejected; the identity band borrows zero-copy. let r1 = rasters.get(1).unwrap(); assert_eq!(r1.num_bands(), 2); assert_eq!(r1.band(0).unwrap().shape(), &[4]); - assert_eq!( - r1.band(0) - .unwrap() - .nd_buffer() - .unwrap() - .as_contiguous() - .unwrap(), - &[42u8, 43, 44, 45] - ); + let r1b0 = r1.band(0).unwrap(); + let r1nd0 = r1b0.nd_buffer().unwrap(); + assert!(!r1nd0.is_contiguous()); + assert!(r1nd0.as_contiguous().is_err()); assert_eq!(r1.band(1).unwrap().shape(), &[4]); - assert_eq!( - r1.band(1) - .unwrap() - .nd_buffer() - .unwrap() - .as_contiguous() - .unwrap(), - &[1u8, 2, 3, 4] - ); + let r1b1 = r1.band(1).unwrap(); + let r1nd1 = r1b1.nd_buffer().unwrap(); + assert_eq!(r1nd1.as_contiguous().unwrap(), &[1u8, 2, 3, 4]); // Fast paths must honour the same offsets. assert_eq!(r0.band_data_type(1), Some(BandDataType::UInt8)); @@ -1158,6 +1552,60 @@ mod tests { assert!(r1.band_nodata(0).is_none()); } + // ---- Fast-path / band(i) divergence on a corrupt view ---- + + #[test] + fn fast_paths_return_columnar_values_when_band_is_corrupt() { + // band(i) goes through validate_view and returns None for a + // malformed view; the columnar fast paths read their fields + // directly without consulting the view at all. Pin down that + // contract so a future reader doesn't accidentally couple them + // (or "fix" the divergence in either direction without us + // noticing). Also catches a regression where a fast path would + // panic instead of returning the underlying value. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x"], &[3], None) + .unwrap(); + builder + .start_band_with_view(StartBandWithViewArgs { + name: Some("a"), + dim_names: &["x"], + source_shape: &[8], + view: &[ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }], + data_type: BandDataType::UInt32, + nodata: Some(&[0u8, 0, 0, 0]), + outdb_uri: Some("s3://bucket/a.tif"), + outdb_format: Some("GTiff"), + }) + .unwrap(); + builder.band_data_writer().append_value(vec![0u8; 32]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + + let bad_view = make_band_view_list(vec![vec![(0, 0, 1, -1)]], None); + let mutated = replace_band_column(&array, band_indices::VIEW, bad_view); + let rasters = RasterStructArray::new(&mutated); + let r = rasters.get(0).unwrap(); + + // band(i) rejects on validate_view. + assert!(r.band(0).is_err()); + + // Fast paths still surface the underlying columnar values — + // they don't validate the view, by design. Locking that in. + assert_eq!(r.band_data_type(0), Some(BandDataType::UInt32)); + assert_eq!(r.band_outdb_uri(0), Some("s3://bucket/a.tif")); + assert_eq!(r.band_outdb_format(0), Some("GTiff")); + assert_eq!(r.band_nodata(0), Some(&[0u8, 0, 0, 0][..])); + } + #[test] fn zero_element_indb_band_classifies_as_indb() { // A band with a 0-size dim (here `time = 0`) legitimately holds 0 bytes. diff --git a/rust/sedona-raster/src/builder.rs b/rust/sedona-raster/src/builder.rs index 8747917086..f521336a73 100644 --- a/rust/sedona-raster/src/builder.rs +++ b/rust/sedona-raster/src/builder.rs @@ -29,7 +29,8 @@ use std::sync::Arc; use sedona_schema::raster::{BandDataType, RasterSchema}; -use crate::traits::{BandMetadata, MetadataRef}; +use crate::traits::{BandMetadata, BandRef, MetadataRef}; +use crate::view_entries::{ViewEntries, ViewEntry}; /// Maximum byte length of an inline `BinaryViewArray` view. Views this short /// store their bytes in the 16-byte view itself; longer views reference a data @@ -84,6 +85,36 @@ const MAX_INLINE_VIEW_LEN: u32 = 12; /// // Get the final StructArray /// let raster_array = builder.finish().unwrap(); /// ``` +/// Arguments to [`RasterBuilder::start_band_with_view`]. Bundled into a +/// struct to keep the call site readable — eight slots is enough that +/// positional args invite mis-ordering bugs. +pub(crate) struct StartBandWithViewArgs<'a> { + pub name: Option<&'a str>, + pub dim_names: &'a [&'a str], + pub source_shape: &'a [i64], + pub view: &'a [ViewEntry], + pub data_type: BandDataType, + pub nodata: Option<&'a [u8]>, + pub outdb_uri: Option<&'a str>, + pub outdb_format: Option<&'a str>, +} + +/// Arguments to [`RasterBuilder::with_view`]. Mirrors +/// [`StartBandWithViewArgs`] minus the two fields `with_view` derives from +/// `input` (`source_shape` from `input.raw_source_shape()`, `data_type` from +/// `input.data_type()`) — accepting those from the caller would let them +/// contradict `input`. `view` here is a *delta* composed against +/// `input.view()`, not the absolute view stored on the band. +pub struct WithViewArgs<'a> { + pub name: Option<&'a str>, + pub dim_names: &'a [&'a str], + pub input: &'a dyn BandRef, + pub view: &'a [ViewEntry], + pub nodata: Option<&'a [u8]>, + pub outdb_uri: Option<&'a str>, + pub outdb_format: Option<&'a str>, +} + pub struct RasterBuilder { // Top-level raster fields crs: StringViewBuilder, @@ -387,6 +418,187 @@ impl RasterBuilder { Ok(()) } + /// Internal: write a band with an explicit view over a raw source + /// shape. Public callers should use [`Self::with_view`] instead, + /// which derives `source_shape`, validates view composition, and + /// inherits the input band's source bytes — `with_view` calls this + /// helper after composing. + /// + /// Each `ViewEntry` describes one *visible* axis in `dim_names` order: + /// `(source_axis, start, step, steps)`. Validates that: + /// - `dim_names`, `source_shape`, and `view` have equal length. + /// - Across `view`, `source_axis` values form a permutation of + /// `0..ndim` (no axis duplicated, none missing). + /// - For each entry with `steps > 0`: `start` and (when `step != 0`) + /// `start + (steps - 1) * step` are in `[0, source_shape[source_axis])`. + /// - `steps >= 0`. + pub(crate) fn start_band_with_view( + &mut self, + args: StartBandWithViewArgs<'_>, + ) -> Result<(), ArrowError> { + let StartBandWithViewArgs { + name, + dim_names, + source_shape, + view, + data_type, + nodata, + outdb_uri, + outdb_format, + } = args; + let ndim = dim_names.len(); + if ndim == 0 { + return Err(ArrowError::InvalidArgumentError( + "start_band_with_view: 0-dimensional bands are not supported".into(), + )); + } + if source_shape.len() != ndim || view.len() != ndim { + return Err(ArrowError::InvalidArgumentError(format!( + "start_band_with_view: dim_names ({}), source_shape ({}), and view ({}) \ + must all have the same length", + ndim, + source_shape.len(), + view.len() + ))); + } + + let view_entries = ViewEntries::new(view.to_vec()); + view_entries.validate(source_shape)?; + + // Write fields. + match name { + Some(n) => self.band_name.append_value(n), + None => self.band_name.append_null(), + } + + for dn in dim_names { + self.band_dim_names_values.append_value(dn); + } + let next = *self.band_dim_names_offsets.last().unwrap() + ndim as i32; + self.band_dim_names_offsets.push(next); + + for &s in source_shape { + self.band_shape_values.append_value(s); + } + let next = *self.band_shape_offsets.last().unwrap() + ndim as i32; + self.band_shape_offsets.push(next); + + self.band_datatype.append_value(data_type as u32); + + match nodata { + Some(b) => self.band_nodata.append_value(b), + None => self.band_nodata.append_null(), + } + + for v in view { + self.band_view_source_axis_values + .append_value(v.source_axis); + self.band_view_start_values.append_value(v.start); + self.band_view_step_values.append_value(v.step); + self.band_view_steps_values.append_value(v.steps); + } + let next = *self.band_view_offsets.last().unwrap() + ndim as i32; + self.band_view_offsets.push(next); + self.band_view_validity.push(true); + + match outdb_uri { + Some(uri) => self.band_outdb_uri.append_value(uri), + None => self.band_outdb_uri.append_null(), + } + match outdb_format { + Some(format) => self.band_outdb_format.append_value(format), + None => self.band_outdb_format.append_null(), + } + + self.current_band_count += 1; + self.band_data_count_at_start = self.band_data.len(); + + // finish_raster compares visible shape against spatial_shape. + self.current_raster_bands.push(( + dim_names.iter().map(|s| s.to_string()).collect(), + view_entries.visible_shape(), + )); + + Ok(()) + } + + /// Build a band that is a new view into an existing band. + /// + /// The output band stores a view that is the composition of `input`'s + /// existing view with the supplied `view`. The supplied `view`'s + /// `source_axis` entries refer to `input`'s *visible* axes, not its + /// source axes — composition with `input.view()` translates them. + /// + /// `dim_names` names the output's *visible* axes (length == view.len()). + /// + /// Storage: + /// - **InDb input** → output is InDb. The input's source bytes are + /// copied verbatim into the output's `data` column (today's + /// simple-share strategy; buffer-sharing via Arrow `BinaryView` is a + /// future optimisation). + /// - **OutDb input** → output is OutDb. The data column stays empty; + /// the input's `outdb_uri` and `outdb_format` are inherited (unless + /// overridden via the explicit `outdb_uri` / `outdb_format` args). + /// The composed view lives alongside the same external pointer — + /// loading is deferred to whoever reads the visible bytes. + /// + /// Identity-input shortcut: when `input` carries identity view, the + /// composed view equals `view` verbatim. + pub fn with_view(&mut self, args: WithViewArgs) -> Result<(), ArrowError> { + let WithViewArgs { + name, + dim_names, + input, + view, + nodata, + outdb_uri, + outdb_format, + } = args; + let source_shape: Vec = input.raw_source_shape().to_vec(); + let composed = + ViewEntries::new(input.view().to_vec()).compose(&ViewEntries::new(view.to_vec()))?; + + // Inherit storage metadata from the input unless the caller has + // explicitly overridden it. For OutDb inputs this propagates the + // external pointer to the output; for InDb inputs the input's + // outdb_uri/outdb_format are typically None anyway. + let final_outdb_uri = outdb_uri.or_else(|| input.outdb_uri()); + let final_outdb_format = outdb_format.or_else(|| input.outdb_format()); + + // Reuse the internal start_band_with_view helper to perform + // validation + write the schema fields. + self.start_band_with_view(StartBandWithViewArgs { + name, + dim_names, + source_shape: &source_shape, + view: composed.as_slice(), + data_type: input.data_type(), + nodata, + outdb_uri: final_outdb_uri, + outdb_format: final_outdb_format, + })?; + + if input.is_indb() { + // InDb: nd_buffer().buffer is the source bytes — borrow them + // directly into `append_value` so the only copy is the one + // BinaryViewBuilder makes into its block. This is still + // a full source-bytes copy per `with_view` call, which + // undermines the "lazy slice" framing for large rasters. + // + // The principled fix is Arrow `BinaryView` buffer-sharing: + // the output's data row references the input's existing + // backing `Buffer` instead of copying. Tracked separately + // in the Raster Clean Up project. + let buf = input.nd_buffer()?; + self.band_data_writer().append_value(buf.buffer); + } else { + // OutDb: data column stays empty; the source bytes live at the + // inherited outdb_uri and are fetched lazily on read. + self.band_data_writer().append_value([]); + } + Ok(()) + } + /// Convenience: start a 2D band with `dim_names=["y","x"]` and `shape=[height, width]`. /// /// Must be called after `start_raster_2d` / `start_raster_2d` which sets @@ -1161,9 +1373,13 @@ mod tests { // Test creating raster with OutDb reference metadata let mut builder = RasterBuilder::new(10); + // 10x10 raster of UInt8 = 100 visible bytes, matching the data buffer + // written below. `RasterRef::band()` now verifies the data column is + // long enough to cover the visible region, so the dimensions and the + // byte count must agree. let metadata = RasterMetadata { - width: 1024, - height: 1024, + width: 10, + height: 10, upperleft_x: 0.0, upperleft_y: 0.0, scale_x: 1.0, @@ -1220,6 +1436,8 @@ mod tests { assert!(indb_metadata.outdb_url().is_none()); assert!(indb_metadata.outdb_band_id().is_none()); assert!(indb_band.is_indb()); + let indb_nd = indb_band.nd_buffer().unwrap(); + assert_eq!(indb_nd.as_contiguous().unwrap().len(), 100); // Test OutDbRef band let outdb_band = bands.band(2).unwrap(); @@ -1234,7 +1452,10 @@ mod tests { "s3://mybucket/satellite_image.tif" ); assert_eq!(outdb_metadata.outdb_band_id().unwrap(), 2); + // OutDbRef bands carry no in-band bytes; byte access via nd_buffer() + // is not supported (backend-specific resolvers are tracked separately). assert!(!outdb_band.is_indb()); + assert!(outdb_band.nd_buffer().is_err()); } #[test] @@ -1831,6 +2052,44 @@ mod tests { ); } + #[test] + fn test_start_band_with_view_identity_matches_start_band() { + // Identity view through start_band_with_view should produce the same + // visible shape and byte strides as the convenience start_band path. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[5, 4], None) + .unwrap(); + + let view = crate::view_entries![0:4, 0:5]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["y", "x"], + source_shape: &[4, 5], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder.band_data_writer().append_value(vec![0u8; 20]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + assert_eq!(band.shape(), &[4, 5]); + assert_eq!(band.raw_source_shape(), &[4, 5]); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.strides, &[5, 1]); + assert_eq!(buf.offset, 0); + } + #[test] fn test_start_band_rejects_zero_dim() { // 0-D bands carry no spatial extent and no caller has a use for @@ -1848,6 +2107,63 @@ mod tests { ); } + #[test] + fn test_start_band_with_view_rejects_zero_dim() { + // start_band_with_view must apply the same 0-D guard as start_band + // — accepting empty dim_names would otherwise bypass it via the + // explicit-view path. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let err = builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &[], + source_shape: &[], + view: &[], + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap_err(); + assert!( + err.to_string().contains("0-dimensional"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_view_validation_rejects_step_overrun() { + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + // start=1, step=2, steps=4 → addresses element 1+(4-1)*2 = 7 which is + // out of range for a source size of 7. + let view = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 4, + }]; + let err = builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[7], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap_err(); + assert!( + err.to_string().contains("out of range"), + "unexpected error: {err}" + ); + } + #[test] fn test_as_contiguous_identity_via_start_band_borrows() { // Canonical identity: the row's view list is null, and the read path @@ -1890,6 +2206,149 @@ mod tests { assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); } + #[test] + fn test_as_contiguous_explicit_identity_view_borrows() { + // Identity expressed *explicitly* through start_band_with_view must be + // indistinguishable to consumers from the null-row identity above — + // same visible shape, same byte strides, same zero-copy borrow. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + let view = crate::view_entries![0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["y", "x"], + source_shape: &[2, 3], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let pixels: Vec = (0..6).collect(); + builder.band_data_writer().append_value(pixels.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + assert_eq!(band.shape(), &[2, 3]); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.strides, &[3, 1]); + assert_eq!(buf.offset, 0); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); + } + + #[test] + fn test_zero_step_broadcast_2d_is_strided_and_rejected() { + // 2D broadcast: source shape [1, 3], view broadcasts axis 0 four + // times so the visible region is 4×3. Each visible row must equal the + // source's only row. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ + ViewEntry { + source_axis: 0, + start: 0, + step: 0, + steps: 4, + }, + ViewEntry { + source_axis: 1, + start: 0, + step: 1, + steps: 3, + }, + ]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["row", "col"], + source_shape: &[1, 3], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder.band_data_writer().append_value(vec![10u8, 20, 30]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[4, 3]); + // Broadcast row stride is 0; column stride is 1 byte per UInt8. + assert_eq!(buf.strides, &[0, 1]); + assert_eq!(buf.offset, 0); + + // A zero stride is not C-order packed, so the buffer is non-contiguous + // and as_contiguous rejects it (repacking lives behind + // RS_EnsureContiguous, https://github.com/apache/sedona-db/issues/899). + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } + + #[test] + fn test_negative_step_strided_reverse_is_rejected() { + // 1D source [0..8] with start=6, step=-2, steps=3 picks every other + // element walking backwards: {6, 4, 2}. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 6, + step: -2, + steps: 3, + }]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[8], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder + .band_data_writer() + .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[3]); + assert_eq!(buf.strides, &[-2]); + assert_eq!(buf.offset, 6); + + // A negative stride is not C-order packed → non-contiguous, rejected. + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } + #[test] fn test_view_field_is_null_for_identity_band() { // Schema invariant: identity views are stored as null list rows so @@ -1970,6 +2429,187 @@ mod tests { ); } + #[test] + fn test_outer_axis_slice_float32_is_contiguous() { + // Multi-byte dtype outer-axis slice: a 2D view over Float32 that + // takes the leading rows from offset 0 is contiguous-but-not-identity, + // so as_contiguous borrows the source prefix zero-copy. Catches a + // regression where contiguity assumed dtype_size == 1. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + // Slice the outer axis: take rows 0 and 1 of a 3-row source. With + // start=0, step=1, steps=2 over an axis of size 3, the view is not + // identity, but its byte strides are still C-order packed from + // offset 0, so the buffer is contiguous and borrows zero-copy. + let view = crate::view_entries![0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["y", "x"], + source_shape: &[3, 3], // 3x3 source + view: view.as_slice(), + data_type: BandDataType::Float32, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let source: Vec = (0..9).map(|i| i as f32).collect(); + let source_bytes: Vec = source.iter().flat_map(|f| f.to_le_bytes()).collect(); + builder + .band_data_writer() + .append_value(source_bytes.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + // Visible shape is [2, 3]; the first 6 source floats (rows 0,1) are + // exactly the visible pixels — i.e. the first 24 source bytes. + let buf = band.nd_buffer().unwrap(); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), &source_bytes[0..24]); + } + + #[test] + fn test_outer_axis_slice_3d_is_contiguous() { + // 3D source [T=3, Y=2, X=3] of UInt8. View slices T to T=1..3 + // (start=1, step=1, steps=2), keeps Y and X identity. The visible + // region is a contiguous source sub-range (offset 6, C-order packed + // strides), so as_contiguous borrows it zero-copy. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + let view = crate::view_entries![1:3, 0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["t", "y", "x"], + source_shape: &[3, 2, 3], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let source: Vec = (0..18).collect(); + builder.band_data_writer().append_value(source.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + // Visible region = source[6..18] (T=1 and T=2 planes). + assert_eq!(band.shape(), &[2, 2, 3]); + let buf = band.nd_buffer().unwrap(); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), &source[6..18]); + } + + #[test] + fn test_nd_buffer_permutation_and_slice_combined() { + // 2D source [Y=4, X=3]. View permutes (visible order [X, Y]) and + // slices Y from 1, step 2, steps 2. Expected: + // visible_shape = [3, 2] + // byte_strides = [step_X * stride_X_src, step_Y * stride_Y_src] + // = [1 * 1, 2 * 3] = [1, 6] + // byte_offset = start_X * stride_X_src + start_Y * stride_Y_src + // = 0 * 1 + 1 * 3 = 3 + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ + ViewEntry { + source_axis: 1, + start: 0, + step: 1, + steps: 3, + }, // X + ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 2, + }, // Y + ]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x", "y"], + source_shape: &[4, 3], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder + .band_data_writer() + .append_value((0u8..12).collect::>()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[3, 2]); + assert_eq!(buf.strides, &[1, 6]); + assert_eq!(buf.offset, 3); + + // The permuted+strided layout (strides [1, 6]) is not C-order packed, + // so the buffer is non-contiguous and as_contiguous rejects it. + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } + + #[test] + fn test_nd_buffer_multidim_with_zero_axis() { + // visible_shape contains a zero axis somewhere in the middle. The + // buffer spans zero bytes, so it is trivially contiguous and + // as_contiguous borrows an empty slice; nd_buffer returns the + // zero-element shape. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = crate::view_entries![0:3, 0:0, 0:5]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["a", "b", "c"], + source_shape: &[3, 4, 5], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder.band_data_writer().append_value(vec![0u8; 60]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + assert_eq!(band.shape(), &[3, 0, 5]); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[3, 0, 5]); + assert!(buf.is_contiguous()); + assert!(buf.as_contiguous().unwrap().is_empty()); + } + #[test] fn test_view_null_round_trips_through_arrow_ipc() { // Schema invariant: a band built via start_band_nd serialises with a @@ -1978,8 +2618,9 @@ mod tests { // instead, downstream readers (DuckDB, PyArrow, sedona-py) will // disagree about whether the view is identity. - let mut builder = RasterBuilder::new(1); + let mut builder = RasterBuilder::new(2); let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + // Raster 0: identity-view band → null view row. builder .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) .unwrap(); @@ -1997,6 +2638,33 @@ mod tests { builder.band_data_writer().append_value(vec![0u8; 6]); builder.finish_band().unwrap(); builder.finish_raster().unwrap(); + // Raster 1: explicit non-identity view → non-null view row. + builder + .start_raster_nd(&transform, &["x"], &[3], None) + .unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[8], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder + .band_data_writer() + .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); let array = builder.finish().unwrap(); let schema = Arc::new(Schema::new(vec![Arc::new(arrow_schema::Field::new( @@ -2023,6 +2691,8 @@ mod tests { .downcast_ref::() .unwrap(); + // Reach into the restored bands list and confirm the view list + // preserves null/non-null per row. let bands_list = restored_struct .column(sedona_schema::raster::raster_indices::BANDS) .as_any() @@ -2038,15 +2708,257 @@ mod tests { .as_any() .downcast_ref::() .unwrap(); - assert_eq!(view_list.len(), 1); + assert_eq!(view_list.len(), 2); assert!( view_list.is_null(0), "identity-view band must remain a null view row after IPC round-trip" ); + assert!( + !view_list.is_null(1), + "explicit-view band must remain non-null after IPC round-trip" + ); + // Sanity: read paths still produce the expected visible shapes. let rasters = RasterStructArray::new(restored_struct); let r0 = rasters.get(0).unwrap(); assert_eq!(r0.band(0).unwrap().shape(), &[2, 3]); + let r1 = rasters.get(1).unwrap(); + assert_eq!(r1.band(0).unwrap().shape(), &[3]); + } + + // ---- with_view: public "create a new view into an existing band" ---- + + /// Build a 1-D UInt8 raster with `source_shape=[8]` and bytes + /// `[0, 1, ..., 7]`. Identity-view; used as input to with_view tests. + fn build_1d_identity_raster() -> StructArray { + let mut b = RasterBuilder::new(1); + b.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[8], None) + .unwrap(); + b.start_band_nd(None, &["x"], &[8], BandDataType::UInt8, None, None, None) + .unwrap(); + b.band_data_writer() + .append_value((0u8..8).collect::>()); + b.finish_band().unwrap(); + b.finish_raster().unwrap(); + b.finish().unwrap() + } + + #[test] + fn with_view_over_identity_input_produces_expected_visible_bytes() { + // Input is identity over [0..8]. with_view layers a slice + // (start=1, step=2, steps=3) producing visible bytes [1, 3, 5]. + let input_array = build_1d_identity_raster(); + let input_rasters = RasterStructArray::new(&input_array); + let input_raster = input_rasters.get(0).unwrap(); + let input_band = input_raster.band(0).unwrap(); + + let mut b = RasterBuilder::new(1); + b.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[3], None) + .unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }]; + b.with_view(WithViewArgs { + name: None, + dim_names: &["x"], + input: input_band.as_ref(), + view: &view, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + b.finish_band().unwrap(); + b.finish_raster().unwrap(); + let out_array = b.finish().unwrap(); + + let out_rasters = RasterStructArray::new(&out_array); + let out_raster = out_rasters.get(0).unwrap(); + let out_band = out_raster.band(0).unwrap(); + assert_eq!(out_band.shape(), &[3]); + // The composed view is a strided slice (start=1, step=2) over the + // original source: byte stride 2, offset 1. Non-contiguous, so + // as_contiguous rejects it; the visible bytes [1, 3, 5] would be + // produced by RS_EnsureContiguous + // (https://github.com/apache/sedona-db/issues/899). + let buf = out_band.nd_buffer().unwrap(); + assert_eq!(buf.strides, &[2]); + assert_eq!(buf.offset, 1); + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + // The output's source_shape is inherited from the input. + assert_eq!(out_band.raw_source_shape(), &[8]); + } + + #[test] + fn with_view_chained_composes_into_single_view() { + // Round 1: with_view layers (start=1, step=2, steps=4) → visible + // bytes [1, 3, 5, 7] over source [0..8]. + // Round 2: with_view on that, layering (start=1, step=1, steps=2) → + // visible bytes [3, 5] (input visible indices 1 and 2). + // + // After Round 2 the output's view, when composed against the + // ORIGINAL source [0..8], must give bytes [3, 5] from indices + // 3 and 5. compose_view collapses the chain into one source-space + // view; the test verifies the bytes round-trip end-to-end. + let input_array = build_1d_identity_raster(); + let input_rasters = RasterStructArray::new(&input_array); + let input_raster = input_rasters.get(0).unwrap(); + let input_band = input_raster.band(0).unwrap(); + + // Round 1. + let mut b1 = RasterBuilder::new(1); + b1.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[4], None) + .unwrap(); + let v1 = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 4, + }]; + b1.with_view(WithViewArgs { + name: None, + dim_names: &["x"], + input: input_band.as_ref(), + view: &v1, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + b1.finish_band().unwrap(); + b1.finish_raster().unwrap(); + let mid_array = b1.finish().unwrap(); + + // Sanity: Round 1 alone produces [1, 3, 5, 7]. + let mid_rasters = RasterStructArray::new(&mid_array); + let mid_raster = mid_rasters.get(0).unwrap(); + let mid_band = mid_raster.band(0).unwrap(); + assert_eq!(mid_band.shape(), &[4]); + // Round 1 view: start=1, step=2 over source [0..8] → strides [2], + // offset 1 (visible bytes would be [1, 3, 5, 7]). Strided, so the + // buffer is non-contiguous. + let mid_buf = mid_band.nd_buffer().unwrap(); + assert_eq!(mid_buf.strides, &[2]); + assert_eq!(mid_buf.offset, 1); + assert!(!mid_buf.is_contiguous()); + + // Round 2: with_view applied on the view-bearing mid_band. + let mut b2 = RasterBuilder::new(1); + b2.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[2], None) + .unwrap(); + let v2 = crate::view_entries![1:3]; + b2.with_view(WithViewArgs { + name: None, + dim_names: &["x"], + input: mid_band.as_ref(), + view: v2.as_slice(), + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + b2.finish_band().unwrap(); + b2.finish_raster().unwrap(); + let final_array = b2.finish().unwrap(); + + let final_rasters = RasterStructArray::new(&final_array); + let final_raster = final_rasters.get(0).unwrap(); + let final_band = final_raster.band(0).unwrap(); + assert_eq!(final_band.shape(), &[2]); + // compose_view collapses both rounds into a single source-space view: + // visible bytes [3, 5] map to source indices 3 and 5 → strides [2], + // offset 3. Strided, so the composed buffer is non-contiguous. + let final_buf = final_band.nd_buffer().unwrap(); + assert_eq!(final_buf.strides, &[2]); + assert_eq!(final_buf.offset, 3); + assert!(!final_buf.is_contiguous()); + // The composed view still references the original 8-byte source. + assert_eq!(final_band.raw_source_shape(), &[8]); + } + + #[test] + fn with_view_on_outdb_input_produces_outdb_output_with_composed_view() { + // Viewing an OutDb band doesn't need the source bytes — the output + // band is itself OutDb, pointing at the same external resource via + // an inherited outdb_uri, with the composed view describing the + // slice. Loading is deferred to whoever reads the visible bytes. + let mut b = RasterBuilder::new(1); + b.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[8], None) + .unwrap(); + b.start_band_nd( + None, + &["x"], + &[8], + BandDataType::UInt8, + None, + Some("s3://bucket/file.tif#band=1"), + Some("geotiff"), + ) + .unwrap(); + b.band_data_writer().append_value([0u8; 0]); // empty → OutDb + b.finish_band().unwrap(); + b.finish_raster().unwrap(); + let input_array = b.finish().unwrap(); + + let input_rasters = RasterStructArray::new(&input_array); + let input_raster = input_rasters.get(0).unwrap(); + let input_band = input_raster.band(0).unwrap(); + assert!(!input_band.is_indb(), "fixture must be OutDb"); + + // Slice the OutDb band's visible axis: start=1, step=2, steps=3. + let mut b2 = RasterBuilder::new(1); + b2.start_raster_nd(&[0.0, 1.0, 0.0, 0.0, 0.0, -1.0], &["x"], &[3], None) + .unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }]; + b2.with_view(WithViewArgs { + name: None, + dim_names: &["x"], + input: input_band.as_ref(), + view: &view, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + b2.finish_band().unwrap(); + b2.finish_raster().unwrap(); + let out_array = b2.finish().unwrap(); + + let out_rasters = RasterStructArray::new(&out_array); + let out_raster = out_rasters.get(0).unwrap(); + let out_band = out_raster.band(0).unwrap(); + + // Output remains OutDb (data column stayed empty). + assert!( + !out_band.is_indb(), + "output of OutDb-input with_view must be OutDb" + ); + // Storage metadata inherited from the input. + assert_eq!(out_band.outdb_uri(), Some("s3://bucket/file.tif#band=1")); + assert_eq!(out_band.outdb_format(), Some("geotiff")); + // Composed view: input had identity view, so composed == supplied + // view entry verbatim. + assert_eq!( + out_band.view(), + &[ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 3, + }] + ); + assert_eq!(out_band.raw_source_shape(), &[8]); + // Visible shape derived from composed view. + assert_eq!(out_band.shape(), &[3]); } /// Navigate an output raster `StructArray` to its bands' `data` diff --git a/rust/sedona-raster/src/traits.rs b/rust/sedona-raster/src/traits.rs index 6bfa69aa6a..099464c07a 100644 --- a/rust/sedona-raster/src/traits.rs +++ b/rust/sedona-raster/src/traits.rs @@ -45,8 +45,14 @@ pub fn is_spatial_dim_pair(y_like: &str, x_like: &str) -> bool { /// `source_shape` (the natural extent of `buffer`) with its `view` /// (the per-axis `(source_axis, start, step, steps)` slice spec). Stride /// can be zero (broadcast) or negative (reverse iteration), and may not be -/// C-order. Consumers that need a flat row-major buffer should use -/// `BandRef::contiguous_data()` instead. +/// C-order. Consumers that need a flat row-major buffer should check +/// `NdBuffer::is_contiguous()` and borrow via `NdBuffer::as_contiguous()`, +/// which errors on strided layouts rather than allocating. +/// +/// `shape` and `offset` are `i64` (not `u64`) to match the surrounding +/// stride arithmetic (`strides: Vec` to allow negative steps); +/// shape elements are always non-negative — `validate_view` enforces +/// this — and `offset` is non-negative by construction. /// /// Only `buffer` is tied to the producer's lifetime `'a` (it can be tens of /// MBs of pixel data and must not be copied). `shape` and `strides` are @@ -58,7 +64,7 @@ pub struct NdBuffer<'a> { pub buffer: &'a [u8], pub shape: Vec, pub strides: Vec, - pub offset: u64, + pub offset: i64, pub data_type: BandDataType, } @@ -499,11 +505,15 @@ pub trait RasterRef { /// Trait for accessing a single band/variable within an N-D raster. /// /// This is the consumer interface. Implementations handle storage details -/// Two data access paths: -/// - `contiguous_data()` — flat row-major bytes for consumers that don't need -/// stride awareness (most RS_* functions, GDAL boundary, serialization). -/// - `nd_buffer()` — raw buffer + shape + strides + offset for stride-aware -/// consumers (numpy zero-copy views, Arrow FFI) that want to avoid copies. +/// and view composition transparently. +/// +/// `nd_buffer()` is the sole zero-copy byte accessor: it returns the raw +/// buffer plus shape + strides + offset describing the visible region. +/// Stride-aware consumers (numpy zero-copy views, Arrow FFI) read it +/// directly; consumers that need flat row-major bytes borrow via +/// `NdBuffer::as_contiguous()`, which errors on strided layouts rather than +/// allocating. The trait never materializes a strided view behind the +/// caller's back — repacking is an explicit plan-node concern. pub trait BandRef { // -- Dimension metadata -- @@ -581,7 +591,7 @@ pub trait BandRef { /// OutDb format — how to interpret the bytes at `outdb_uri` /// (e.g. `"geotiff"`, `"zarr"`). None means in-memory — the band's - /// `contiguous_data()` / `nd_buffer()` is authoritative. + /// `nd_buffer()` is authoritative. fn outdb_format(&self) -> Option<&str> { None } @@ -645,6 +655,18 @@ pub trait BandRef { /// `offset` are computed by composing the view with the source's /// natural C-order byte strides. Strides may be zero (broadcast) or /// negative (reverse iteration). + /// + /// The returned `shape`, `strides`, and `offset` are guaranteed + /// in-bounds for `buffer`: `RasterRef::band()` rejects malformed views, + /// overflowing stride composition, and source-shape/data-column length + /// mismatches at construction. Stride-aware consumers can walk the + /// returned layout without further bound checks against `buffer.len()`. + /// + /// This is the **sole** byte-access method on the trait — it is + /// zero-copy and never allocates. Contiguous-byte consumers call + /// [`NdBuffer::as_contiguous`] on the result (borrow-or-error); + /// materialization of a strided view is an explicit `RS_EnsureContiguous` + /// step, never a transparent allocation behind this interface. fn nd_buffer(&self) -> Result, ArrowError>; /// Nodata value interpreted as f64. @@ -984,40 +1006,116 @@ mod tests { assert!(!b.is_spatial_2d()); } - /// Build a bufferless `NdBuffer` for contiguity checks — `is_contiguous` - /// inspects only shape/strides/data_type, never the bytes. - fn ndbuf(shape: &[i64], strides: &[i64], offset: u64) -> NdBuffer<'static> { + // ---- NdBuffer::is_contiguous / as_contiguous ---- + // + // The contiguity predicate is a pure function of (shape, strides, + // data_type), so it is exercised here directly on NdBuffer literals + // rather than through the full RasterBuilder → reader path. The + // builder/reader tests in builder.rs and array.rs cover that the view + // composition produces these strides/offsets in the first place. + + fn ndbuf<'a>( + buffer: &'a [u8], + shape: &[i64], + strides: &[i64], + offset: i64, + data_type: BandDataType, + ) -> NdBuffer<'a> { NdBuffer { - buffer: &[], + buffer, shape: shape.to_vec(), strides: strides.to_vec(), offset, - data_type: BandDataType::UInt8, + data_type, } } #[test] - fn is_contiguous_packed_identity() { - // C-order packed strides for shape [2, 3], byte_size 1. - assert!(ndbuf(&[2, 3], &[3, 1], 0).is_contiguous()); + fn is_contiguous_identity_2d_uint8_borrows_full_buffer() { + let bytes: Vec = (0..6).collect(); + let b = ndbuf(&bytes, &[2, 3], &[3, 1], 0, BandDataType::UInt8); + assert!(b.is_contiguous()); + assert_eq!(b.as_contiguous().unwrap(), &bytes[..]); + } + + #[test] + fn is_contiguous_identity_multibyte_float32() { + // shape [2, 3] Float32 → C-order byte strides [12, 4]. + let bytes = vec![7u8; 24]; + let b = ndbuf(&bytes, &[2, 3], &[12, 4], 0, BandDataType::Float32); + assert!(b.is_contiguous()); + assert_eq!(b.as_contiguous().unwrap().len(), 24); + } + + #[test] + fn is_contiguous_is_offset_agnostic_for_outer_axis_slice() { + // Take rows 1..3 of a [3, 3] UInt8 source: offset 3, packed strides. + // Contiguous-but-not-identity → borrows the sub-range zero-copy. + let bytes: Vec = (0..9).collect(); + let b = ndbuf(&bytes, &[2, 3], &[3, 1], 3, BandDataType::UInt8); + assert!(b.is_contiguous()); + assert_eq!(b.as_contiguous().unwrap(), &bytes[3..9]); + } + + #[test] + fn is_contiguous_false_for_broadcast_zero_stride() { + let bytes = vec![0u8; 3]; + let b = ndbuf(&bytes, &[4, 3], &[0, 1], 0, BandDataType::UInt8); + assert!(!b.is_contiguous()); + assert!(b.as_contiguous().is_err()); + } + + #[test] + fn is_contiguous_false_for_negative_stride() { + let bytes: Vec = (0..8).collect(); + let b = ndbuf(&bytes, &[3], &[-2], 6, BandDataType::UInt8); + assert!(!b.is_contiguous()); + assert!(b.as_contiguous().is_err()); + } + + #[test] + fn is_contiguous_false_for_permuted_inner_stride() { + // shape [3, 2], strides [1, 6] — inner stride 6 != dtype_size. + let bytes = vec![0u8; 12]; + let b = ndbuf(&bytes, &[3, 2], &[1, 6], 0, BandDataType::UInt8); + assert!(!b.is_contiguous()); + assert!(b.as_contiguous().is_err()); + } + + #[test] + fn is_contiguous_false_for_inner_strided_multibyte() { + // UInt16 is 2 bytes; a step-2 view gives byte stride 4 != 2. + let bytes = vec![0u8; 12]; + let b = ndbuf(&bytes, &[3], &[4], 0, BandDataType::UInt16); + assert!(!b.is_contiguous()); + assert!(b.as_contiguous().is_err()); } #[test] - fn is_contiguous_packed_with_offset() { - // Offset is irrelevant to contiguity — a packed sub-window still - // counts (this is the relaxation the GDAL gate relies on). - assert!(ndbuf(&[2, 3], &[3, 1], 12).is_contiguous()); + fn is_contiguous_true_for_zero_extent_axis_borrows_empty() { + // A zero-extent axis addresses no bytes — trivially contiguous, + // regardless of the surrounding strides. + let bytes = vec![0u8; 8]; + let b = ndbuf(&bytes, &[3, 0, 5], &[0, 5, 1], 0, BandDataType::UInt8); + assert!(b.is_contiguous()); + assert!(b.as_contiguous().unwrap().is_empty()); } #[test] - fn is_contiguous_strided_is_false() { - // Inner stride 2 != byte_size 1 — gaps between elements. - assert!(!ndbuf(&[2, 3], &[6, 2], 0).is_contiguous()); + fn is_contiguous_false_for_shape_strides_length_mismatch() { + let bytes = vec![0u8; 6]; + let b = ndbuf(&bytes, &[2, 3], &[3], 0, BandDataType::UInt8); + assert!(!b.is_contiguous()); + assert!(b.as_contiguous().is_err()); } #[test] - fn is_contiguous_broadcast_is_false() { - // Zero stride (broadcast) is not packed. - assert!(!ndbuf(&[2, 3], &[0, 1], 0).is_contiguous()); + fn as_contiguous_errors_when_region_exceeds_buffer() { + // Layout is C-order packed, but offset pushes the region past the + // buffer end — the defensive bounds check must reject it. + let bytes = vec![0u8; 4]; + let b = ndbuf(&bytes, &[4], &[1], 2, BandDataType::UInt8); + assert!(b.is_contiguous()); + assert!(b.as_contiguous().is_err()); } } From 1e8eac34c6df87f113f56a54e1562768e75e9df2 Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 15 Jun 2026 13:47:46 -0700 Subject: [PATCH 2/8] refactor(rust/sedona-raster): split band() accessor into named helpers Per review, band() mixed several levels of abstraction. Extract resolve_band_row (index -> bounds-checked row), compose_band_layout (read + validate view, compose byte strides/offset), and check_band_buffer_bounds (InDb data-buffer bounds), bundling the composed result in a BandLayout struct. band() now reads as resolve -> decode dtype -> compose -> construct. No behavior change. --- rust/sedona-raster/src/array.rs | 151 +++++++++++++++++++++----------- 1 file changed, 100 insertions(+), 51 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index e9f0727d02..cf7b083b4b 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -320,6 +320,100 @@ impl<'a> RasterRefImpl<'a> { .collect(), )) } + + /// Resolve a 0-based band `index` to its absolute row in the flattened + /// bands arrays, bounds-checked against this raster's band count. + fn resolve_band_row(&self, index: usize) -> Result { + let nbands = self.num_bands(); + if index >= nbands { + return Err(ArrowError::InvalidArgumentError(format!( + "Band index {index} is out of range: this raster has {nbands} bands" + ))); + } + let start = self.bands_list.value_offsets()[self.raster_index] as usize; + Ok(start + index) + } + + /// Read the band's view and source shape, validate the view against the + /// source shape, and compose the byte-stride layout (visible shape, byte + /// strides, byte offset), checking it against the backing data buffer. + fn compose_band_layout( + &self, + band_row: usize, + data_type: BandDataType, + ) -> Result { + let source_shape = self.read_band_source_shape(band_row)?; + let view_entries = self.read_band_view_entries(band_row, &source_shape)?; + view_entries.validate(&source_shape).map_err(|e| { + ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( + "band {band_row} has malformed view: {e}" + ))) + })?; + + let visible_shape = view_entries.visible_shape(); + let (byte_strides, byte_offset) = compose_byte_strides( + band_row, + &source_shape, + &view_entries, + data_type.byte_size(), + )?; + + self.check_band_buffer_bounds( + band_row, + &visible_shape, + &byte_strides, + byte_offset, + data_type, + )?; + + Ok(BandLayout { + view_entries, + visible_shape, + byte_strides, + byte_offset, + }) + } + + /// For InDb bands, verify the `data` BinaryView is long enough to cover + /// every byte the composed view can address. [`ViewEntries::validate`] + /// doesn't know the actual buffer length, so a writer that lies about + /// `source_shape` vs the bytes written would otherwise slip through and + /// panic later when a consumer walks the strided buffer. OutDb bands skip + /// this: their data column is empty by design. + fn check_band_buffer_bounds( + &self, + band_row: usize, + visible_shape: &[i64], + byte_strides: &[i64], + byte_offset: i64, + data_type: BandDataType, + ) -> Result<(), ArrowError> { + let data_bytes = self.band_data_array.value(band_row); + if data_bytes.is_empty() { + return Ok(()); + } + check_view_buffer_bounds( + data_bytes.len(), + visible_shape, + byte_strides, + byte_offset, + data_type.byte_size(), + ) + .map_err(|e| { + ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( + "band {band_row}: view-buffer bounds check failed: {e}" + ))) + }) + } +} + +/// The composed, validated byte-stride layout for one band's view — everything +/// [`RasterRefImpl::band`] derives before constructing a [`BandRefImpl`]. +struct BandLayout { + view_entries: ViewEntries, + visible_shape: Vec, + byte_strides: Vec, + byte_offset: i64, } /// Compose a validated view against a source shape into C-order byte strides @@ -390,54 +484,9 @@ impl<'a> RasterRef for RasterRefImpl<'a> { } fn band(&self, index: usize) -> Result, ArrowError> { - let nbands = self.num_bands(); - if index >= nbands { - return Err(ArrowError::InvalidArgumentError(format!( - "Band index {index} is out of range: this raster has {nbands} bands" - ))); - } - let start = self.bands_list.value_offsets()[self.raster_index] as usize; - let band_row = start + index; - - let source_shape = self.read_band_source_shape(band_row)?; + let band_row = self.resolve_band_row(index)?; let data_type = self.read_band_data_type_or_err(band_row)?; - let view_entries = self.read_band_view_entries(band_row, &source_shape)?; - view_entries.validate(&source_shape).map_err(|e| { - ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( - "band {band_row} has malformed view: {e}" - ))) - })?; - - let visible_shape = view_entries.visible_shape(); - let (byte_strides, byte_offset) = compose_byte_strides( - band_row, - &source_shape, - &view_entries, - data_type.byte_size(), - )?; - - // For InDb bands, verify the underlying data column is long enough - // to cover every byte the view can address. The view-machinery - // validation above doesn't know the actual `data` BinaryView - // length — a writer that lies about source_shape vs the bytes - // written would otherwise slip through and panic later when a - // consumer walks the strided buffer. OutDb bands skip this check: - // their data column is empty by design. - let data_bytes = self.band_data_array.value(band_row); - if !data_bytes.is_empty() { - check_view_buffer_bounds( - data_bytes.len(), - &visible_shape, - &byte_strides, - byte_offset, - data_type.byte_size(), - ) - .map_err(|e| { - ArrowError::ExternalError(Box::new(sedona_common::sedona_internal_datafusion_err!( - "band {band_row}: view-buffer bounds check failed: {e}" - ))) - })?; - } + let layout = self.compose_band_layout(band_row, data_type)?; Ok(Box::new(BandRefImpl { dim_names_list: self.band_dim_names_list, @@ -450,10 +499,10 @@ impl<'a> RasterRef for RasterRefImpl<'a> { data_array: self.band_data_array, band_row, data_type, - view_entries, - visible_shape, - byte_strides, - byte_offset, + view_entries: layout.view_entries, + visible_shape: layout.visible_shape, + byte_strides: layout.byte_strides, + byte_offset: layout.byte_offset, })) } From 541da0a7b667aae7be08e17a2846f8875a0b170f Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 15 Jun 2026 14:01:14 -0700 Subject: [PATCH 3/8] test(rust/sedona-raster): relocate reader/NdBuffer tests to array.rs; add indb_band_meta helper Per review, builder.rs (Arrow serialization) carried tests that actually exercise reader/NdBuffer byte-layout semantics. Move the 9 nd_buffer/ as_contiguous/strides/contiguity tests (broadcast + negative-step rejection, outer-axis-slice contiguity, permutation) to array.rs alongside the other reader tests; keep builder-API and view-serialization tests (start_band_with_view, view-field-null, IPC round-trip) in builder.rs. Add a small indb_band_meta(datatype) test helper to cut repeated InDb BandMetadata literals (3 call sites). No behavior change. --- rust/sedona-raster/src/array.rs | 430 +++++++++++++++++++++++++++ rust/sedona-raster/src/builder.rs | 464 +----------------------------- 2 files changed, 443 insertions(+), 451 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index cf7b083b4b..67dd1647b0 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -1689,4 +1689,434 @@ mod tests { ); assert_eq!(band.metadata().storage_type().unwrap(), StorageType::InDb); } + + #[test] + fn test_as_contiguous_borrows_identity_view() { + let mut builder = RasterBuilder::new(1); + builder + .start_raster_2d(4, 4, 0.0, 0.0, 1.0, -1.0, 0.0, 0.0, None) + .unwrap(); + builder.start_band_2d(BandDataType::UInt8, None).unwrap(); + builder.band_data_writer().append_value([1u8; 16]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + let ndb = band.nd_buffer().unwrap(); + // Identity-view bands are always contiguous, so as_contiguous borrows + // the underlying bytes zero-copy rather than erroring. + assert!(ndb.is_contiguous()); + let data = ndb.as_contiguous().unwrap(); + assert_eq!(data.len(), 16); + } + + #[test] + fn test_nd_buffer_strides_various_types() { + // Each raster exercises a different shape; strict spatial-grid + // validation forbids mixing bands of disagreeing spatial sizes within + // one raster. + let mut builder = RasterBuilder::new(3); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + + // Raster 0 — UInt8: element size = 1, shape [3, 4] → strides [4, 1] + builder + .start_raster_nd(&transform, &["x", "y"], &[4, 3], None) + .unwrap(); + builder + .start_band_nd( + None, + &["y", "x"], + &[3, 4], + BandDataType::UInt8, + None, + None, + None, + ) + .unwrap(); + builder.band_data_writer().append_value(vec![0u8; 12]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + // Raster 1 — Float64: element size = 8, shape [2, 3, 5] → strides [120, 40, 8] + builder + .start_raster_nd(&transform, &["x", "y"], &[5, 3], None) + .unwrap(); + builder + .start_band_nd( + None, + &["z", "y", "x"], + &[2, 3, 5], + BandDataType::Float64, + None, + None, + None, + ) + .unwrap(); + builder + .band_data_writer() + .append_value(vec![0u8; 2 * 3 * 5 * 8]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + // Raster 2 — UInt16: element size = 2, shape [10] → strides [2]. + // Only has an "x" dim, so declare spatial_dims=["x"]. + builder + .start_raster_nd(&transform, &["x"], &[10], None) + .unwrap(); + builder + .start_band_nd(None, &["x"], &[10], BandDataType::UInt16, None, None, None) + .unwrap(); + builder.band_data_writer().append_value(vec![0u8; 20]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + + let r0 = rasters.get(0).unwrap(); + let b0 = r0.band(0).unwrap(); + assert_eq!(b0.nd_buffer().unwrap().strides, &[4, 1]); // UInt8 [3, 4] + + let r1 = rasters.get(1).unwrap(); + let b1 = r1.band(0).unwrap(); + assert_eq!(b1.nd_buffer().unwrap().strides, &[120, 40, 8]); // Float64 [2, 3, 5] + + let r2 = rasters.get(2).unwrap(); + let b2 = r2.band(0).unwrap(); + assert_eq!(b2.nd_buffer().unwrap().strides, &[2]); // UInt16 [10] + } + + #[test] + fn test_as_contiguous_identity_via_start_band_borrows() { + // Canonical identity: the row's view list is null, and the read path + // synthesises the identity view. Should still hand the underlying + // bytes back without copying. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + builder + .start_band_nd( + None, + &["y", "x"], + &[2, 3], + BandDataType::UInt8, + None, + None, + None, + ) + .unwrap(); + let pixels: Vec = (0..6).collect(); + builder.band_data_writer().append_value(pixels.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + // Visible shape comes from the synthesised identity view. + assert_eq!(band.shape(), &[2, 3]); + assert_eq!(band.raw_source_shape(), &[2, 3]); + + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.strides, &[3, 1]); + assert_eq!(buf.offset, 0); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); + } + + #[test] + fn test_as_contiguous_explicit_identity_view_borrows() { + // Identity expressed *explicitly* through start_band_with_view must be + // indistinguishable to consumers from the null-row identity above — + // same visible shape, same byte strides, same zero-copy borrow. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + let view = crate::view_entries![0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["y", "x"], + source_shape: &[2, 3], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let pixels: Vec = (0..6).collect(); + builder.band_data_writer().append_value(pixels.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + assert_eq!(band.shape(), &[2, 3]); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.strides, &[3, 1]); + assert_eq!(buf.offset, 0); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); + } + + #[test] + fn test_zero_step_broadcast_2d_is_strided_and_rejected() { + // 2D broadcast: source shape [1, 3], view broadcasts axis 0 four + // times so the visible region is 4×3. Each visible row must equal the + // source's only row. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ + ViewEntry { + source_axis: 0, + start: 0, + step: 0, + steps: 4, + }, + ViewEntry { + source_axis: 1, + start: 0, + step: 1, + steps: 3, + }, + ]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["row", "col"], + source_shape: &[1, 3], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder.band_data_writer().append_value(vec![10u8, 20, 30]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[4, 3]); + // Broadcast row stride is 0; column stride is 1 byte per UInt8. + assert_eq!(buf.strides, &[0, 1]); + assert_eq!(buf.offset, 0); + + // A zero stride is not C-order packed, so the buffer is non-contiguous + // and as_contiguous rejects it (repacking lives behind + // RS_EnsureContiguous, https://github.com/apache/sedona-db/issues/899). + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } + + #[test] + fn test_negative_step_strided_reverse_is_rejected() { + // 1D source [0..8] with start=6, step=-2, steps=3 picks every other + // element walking backwards: {6, 4, 2}. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ViewEntry { + source_axis: 0, + start: 6, + step: -2, + steps: 3, + }]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x"], + source_shape: &[8], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder + .band_data_writer() + .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[3]); + assert_eq!(buf.strides, &[-2]); + assert_eq!(buf.offset, 6); + + // A negative stride is not C-order packed → non-contiguous, rejected. + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } + + #[test] + fn test_outer_axis_slice_float32_is_contiguous() { + // Multi-byte dtype outer-axis slice: a 2D view over Float32 that + // takes the leading rows from offset 0 is contiguous-but-not-identity, + // so as_contiguous borrows the source prefix zero-copy. Catches a + // regression where contiguity assumed dtype_size == 1. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + // Slice the outer axis: take rows 0 and 1 of a 3-row source. With + // start=0, step=1, steps=2 over an axis of size 3, the view is not + // identity, but its byte strides are still C-order packed from + // offset 0, so the buffer is contiguous and borrows zero-copy. + let view = crate::view_entries![0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["y", "x"], + source_shape: &[3, 3], // 3x3 source + view: view.as_slice(), + data_type: BandDataType::Float32, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let source: Vec = (0..9).map(|i| i as f32).collect(); + let source_bytes: Vec = source.iter().flat_map(|f| f.to_le_bytes()).collect(); + builder + .band_data_writer() + .append_value(source_bytes.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + // Visible shape is [2, 3]; the first 6 source floats (rows 0,1) are + // exactly the visible pixels — i.e. the first 24 source bytes. + let buf = band.nd_buffer().unwrap(); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), &source_bytes[0..24]); + } + + #[test] + fn test_outer_axis_slice_3d_is_contiguous() { + // 3D source [T=3, Y=2, X=3] of UInt8. View slices T to T=1..3 + // (start=1, step=1, steps=2), keeps Y and X identity. The visible + // region is a contiguous source sub-range (offset 6, C-order packed + // strides), so as_contiguous borrows it zero-copy. + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder + .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) + .unwrap(); + let view = crate::view_entries![1:3, 0:2, 0:3]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["t", "y", "x"], + source_shape: &[3, 2, 3], + view: view.as_slice(), + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + let source: Vec = (0..18).collect(); + builder.band_data_writer().append_value(source.clone()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + + // Visible region = source[6..18] (T=1 and T=2 planes). + assert_eq!(band.shape(), &[2, 2, 3]); + let buf = band.nd_buffer().unwrap(); + assert!(buf.is_contiguous()); + assert_eq!(buf.as_contiguous().unwrap(), &source[6..18]); + } + + #[test] + fn test_nd_buffer_permutation_and_slice_combined() { + // 2D source [Y=4, X=3]. View permutes (visible order [X, Y]) and + // slices Y from 1, step 2, steps 2. Expected: + // visible_shape = [3, 2] + // byte_strides = [step_X * stride_X_src, step_Y * stride_Y_src] + // = [1 * 1, 2 * 3] = [1, 6] + // byte_offset = start_X * stride_X_src + start_Y * stride_Y_src + // = 0 * 1 + 1 * 3 = 3 + let mut builder = RasterBuilder::new(1); + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + builder.start_raster_nd(&transform, &[], &[], None).unwrap(); + let view = [ + ViewEntry { + source_axis: 1, + start: 0, + step: 1, + steps: 3, + }, // X + ViewEntry { + source_axis: 0, + start: 1, + step: 2, + steps: 2, + }, // Y + ]; + builder + .start_band_with_view(StartBandWithViewArgs { + name: None, + dim_names: &["x", "y"], + source_shape: &[4, 3], + view: &view, + data_type: BandDataType::UInt8, + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + builder + .band_data_writer() + .append_value((0u8..12).collect::>()); + builder.finish_band().unwrap(); + builder.finish_raster().unwrap(); + let array = builder.finish().unwrap(); + let rasters = RasterStructArray::new(&array); + let r = rasters.get(0).unwrap(); + let band = r.band(0).unwrap(); + let buf = band.nd_buffer().unwrap(); + assert_eq!(buf.shape, &[3, 2]); + assert_eq!(buf.strides, &[1, 6]); + assert_eq!(buf.offset, 3); + + // The permuted+strided layout (strides [1, 6]) is not C-order packed, + // so the buffer is non-contiguous and as_contiguous rejects it. + assert!(!buf.is_contiguous()); + assert!(buf.as_contiguous().is_err()); + } } diff --git a/rust/sedona-raster/src/builder.rs b/rust/sedona-raster/src/builder.rs index f521336a73..6abca4351b 100644 --- a/rust/sedona-raster/src/builder.rs +++ b/rust/sedona-raster/src/builder.rs @@ -989,6 +989,16 @@ mod tests { use sedona_schema::raster::StorageType; use std::io::Cursor; + fn indb_band_meta(datatype: BandDataType) -> BandMetadata { + BandMetadata { + nodata_value: None, + storage_type: StorageType::InDb, + datatype, + outdb_url: None, + outdb_band_id: None, + } + } + #[test] fn test_iterator_basic_functionality() { // Create a simple raster for testing using the correct API @@ -1186,13 +1196,7 @@ mod tests { .unwrap(); // Add new band data while preserving original metadata - let new_band_metadata = BandMetadata { - nodata_value: None, - storage_type: StorageType::InDb, - datatype: BandDataType::UInt16, - outdb_url: None, - outdb_band_id: None, - }; + let new_band_metadata = indb_band_meta(BandDataType::UInt16); target_builder.start_band(new_band_metadata).unwrap(); let new_data = vec![100u16; 1008]; // Different data, same dimensions @@ -1317,13 +1321,7 @@ mod tests { ]; for (expected_data_type, test_data) in test_cases { - let band_metadata = BandMetadata { - nodata_value: None, - storage_type: StorageType::InDb, - datatype: expected_data_type, - outdb_url: None, - outdb_band_id: None, - }; + let band_metadata = indb_band_meta(expected_data_type); builder.start_band(band_metadata).unwrap(); builder.band_data_writer().append_value(&test_data); @@ -1476,13 +1474,7 @@ mod tests { builder.start_raster(&metadata, None).unwrap(); - let band_metadata = BandMetadata { - nodata_value: None, - storage_type: StorageType::InDb, - datatype: BandDataType::UInt8, - outdb_url: None, - outdb_band_id: None, - }; + let band_metadata = indb_band_meta(BandDataType::UInt8); builder.start_band(band_metadata).unwrap(); builder.band_data_writer().append_value([1u8; 100]); @@ -1811,106 +1803,6 @@ mod tests { assert_eq!(band.dim_size("wavelength"), None); } - #[test] - fn test_as_contiguous_borrows_identity_view() { - let mut builder = RasterBuilder::new(1); - builder - .start_raster_2d(4, 4, 0.0, 0.0, 1.0, -1.0, 0.0, 0.0, None) - .unwrap(); - builder.start_band_2d(BandDataType::UInt8, None).unwrap(); - builder.band_data_writer().append_value([1u8; 16]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - let ndb = band.nd_buffer().unwrap(); - // Identity-view bands are always contiguous, so as_contiguous borrows - // the underlying bytes zero-copy rather than erroring. - assert!(ndb.is_contiguous()); - let data = ndb.as_contiguous().unwrap(); - assert_eq!(data.len(), 16); - } - - #[test] - fn test_nd_buffer_strides_various_types() { - // Each raster exercises a different shape; strict spatial-grid - // validation forbids mixing bands of disagreeing spatial sizes within - // one raster. - let mut builder = RasterBuilder::new(3); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - - // Raster 0 — UInt8: element size = 1, shape [3, 4] → strides [4, 1] - builder - .start_raster_nd(&transform, &["x", "y"], &[4, 3], None) - .unwrap(); - builder - .start_band_nd( - None, - &["y", "x"], - &[3, 4], - BandDataType::UInt8, - None, - None, - None, - ) - .unwrap(); - builder.band_data_writer().append_value(vec![0u8; 12]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - // Raster 1 — Float64: element size = 8, shape [2, 3, 5] → strides [120, 40, 8] - builder - .start_raster_nd(&transform, &["x", "y"], &[5, 3], None) - .unwrap(); - builder - .start_band_nd( - None, - &["z", "y", "x"], - &[2, 3, 5], - BandDataType::Float64, - None, - None, - None, - ) - .unwrap(); - builder - .band_data_writer() - .append_value(vec![0u8; 2 * 3 * 5 * 8]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - // Raster 2 — UInt16: element size = 2, shape [10] → strides [2]. - // Only has an "x" dim, so declare spatial_dims=["x"]. - builder - .start_raster_nd(&transform, &["x"], &[10], None) - .unwrap(); - builder - .start_band_nd(None, &["x"], &[10], BandDataType::UInt16, None, None, None) - .unwrap(); - builder.band_data_writer().append_value(vec![0u8; 20]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - - let r0 = rasters.get(0).unwrap(); - let b0 = r0.band(0).unwrap(); - assert_eq!(b0.nd_buffer().unwrap().strides, &[4, 1]); // UInt8 [3, 4] - - let r1 = rasters.get(1).unwrap(); - let b1 = r1.band(0).unwrap(); - assert_eq!(b1.nd_buffer().unwrap().strides, &[120, 40, 8]); // Float64 [2, 3, 5] - - let r2 = rasters.get(2).unwrap(); - let b2 = r2.band(0).unwrap(); - assert_eq!(b2.nd_buffer().unwrap().strides, &[2]); // UInt16 [10] - } - #[test] fn test_width_height_no_bands() { // Zero-band raster — used as a "target grid" specification (GDAL warp @@ -2164,191 +2056,6 @@ mod tests { ); } - #[test] - fn test_as_contiguous_identity_via_start_band_borrows() { - // Canonical identity: the row's view list is null, and the read path - // synthesises the identity view. Should still hand the underlying - // bytes back without copying. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder - .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) - .unwrap(); - builder - .start_band_nd( - None, - &["y", "x"], - &[2, 3], - BandDataType::UInt8, - None, - None, - None, - ) - .unwrap(); - let pixels: Vec = (0..6).collect(); - builder.band_data_writer().append_value(pixels.clone()); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - // Visible shape comes from the synthesised identity view. - assert_eq!(band.shape(), &[2, 3]); - assert_eq!(band.raw_source_shape(), &[2, 3]); - - let buf = band.nd_buffer().unwrap(); - assert_eq!(buf.strides, &[3, 1]); - assert_eq!(buf.offset, 0); - assert!(buf.is_contiguous()); - assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); - } - - #[test] - fn test_as_contiguous_explicit_identity_view_borrows() { - // Identity expressed *explicitly* through start_band_with_view must be - // indistinguishable to consumers from the null-row identity above — - // same visible shape, same byte strides, same zero-copy borrow. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder - .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) - .unwrap(); - let view = crate::view_entries![0:2, 0:3]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["y", "x"], - source_shape: &[2, 3], - view: view.as_slice(), - data_type: BandDataType::UInt8, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - let pixels: Vec = (0..6).collect(); - builder.band_data_writer().append_value(pixels.clone()); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - assert_eq!(band.shape(), &[2, 3]); - let buf = band.nd_buffer().unwrap(); - assert_eq!(buf.strides, &[3, 1]); - assert_eq!(buf.offset, 0); - assert!(buf.is_contiguous()); - assert_eq!(buf.as_contiguous().unwrap(), pixels.as_slice()); - } - - #[test] - fn test_zero_step_broadcast_2d_is_strided_and_rejected() { - // 2D broadcast: source shape [1, 3], view broadcasts axis 0 four - // times so the visible region is 4×3. Each visible row must equal the - // source's only row. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder.start_raster_nd(&transform, &[], &[], None).unwrap(); - let view = [ - ViewEntry { - source_axis: 0, - start: 0, - step: 0, - steps: 4, - }, - ViewEntry { - source_axis: 1, - start: 0, - step: 1, - steps: 3, - }, - ]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["row", "col"], - source_shape: &[1, 3], - view: &view, - data_type: BandDataType::UInt8, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - builder.band_data_writer().append_value(vec![10u8, 20, 30]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - let buf = band.nd_buffer().unwrap(); - assert_eq!(buf.shape, &[4, 3]); - // Broadcast row stride is 0; column stride is 1 byte per UInt8. - assert_eq!(buf.strides, &[0, 1]); - assert_eq!(buf.offset, 0); - - // A zero stride is not C-order packed, so the buffer is non-contiguous - // and as_contiguous rejects it (repacking lives behind - // RS_EnsureContiguous, https://github.com/apache/sedona-db/issues/899). - assert!(!buf.is_contiguous()); - assert!(buf.as_contiguous().is_err()); - } - - #[test] - fn test_negative_step_strided_reverse_is_rejected() { - // 1D source [0..8] with start=6, step=-2, steps=3 picks every other - // element walking backwards: {6, 4, 2}. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder.start_raster_nd(&transform, &[], &[], None).unwrap(); - let view = [ViewEntry { - source_axis: 0, - start: 6, - step: -2, - steps: 3, - }]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["x"], - source_shape: &[8], - view: &view, - data_type: BandDataType::UInt8, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - builder - .band_data_writer() - .append_value(vec![0u8, 1, 2, 3, 4, 5, 6, 7]); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - let buf = band.nd_buffer().unwrap(); - assert_eq!(buf.shape, &[3]); - assert_eq!(buf.strides, &[-2]); - assert_eq!(buf.offset, 6); - - // A negative stride is not C-order packed → non-contiguous, rejected. - assert!(!buf.is_contiguous()); - assert!(buf.as_contiguous().is_err()); - } - #[test] fn test_view_field_is_null_for_identity_band() { // Schema invariant: identity views are stored as null list rows so @@ -2429,151 +2136,6 @@ mod tests { ); } - #[test] - fn test_outer_axis_slice_float32_is_contiguous() { - // Multi-byte dtype outer-axis slice: a 2D view over Float32 that - // takes the leading rows from offset 0 is contiguous-but-not-identity, - // so as_contiguous borrows the source prefix zero-copy. Catches a - // regression where contiguity assumed dtype_size == 1. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder - .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) - .unwrap(); - // Slice the outer axis: take rows 0 and 1 of a 3-row source. With - // start=0, step=1, steps=2 over an axis of size 3, the view is not - // identity, but its byte strides are still C-order packed from - // offset 0, so the buffer is contiguous and borrows zero-copy. - let view = crate::view_entries![0:2, 0:3]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["y", "x"], - source_shape: &[3, 3], // 3x3 source - view: view.as_slice(), - data_type: BandDataType::Float32, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - let source: Vec = (0..9).map(|i| i as f32).collect(); - let source_bytes: Vec = source.iter().flat_map(|f| f.to_le_bytes()).collect(); - builder - .band_data_writer() - .append_value(source_bytes.clone()); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - // Visible shape is [2, 3]; the first 6 source floats (rows 0,1) are - // exactly the visible pixels — i.e. the first 24 source bytes. - let buf = band.nd_buffer().unwrap(); - assert!(buf.is_contiguous()); - assert_eq!(buf.as_contiguous().unwrap(), &source_bytes[0..24]); - } - - #[test] - fn test_outer_axis_slice_3d_is_contiguous() { - // 3D source [T=3, Y=2, X=3] of UInt8. View slices T to T=1..3 - // (start=1, step=1, steps=2), keeps Y and X identity. The visible - // region is a contiguous source sub-range (offset 6, C-order packed - // strides), so as_contiguous borrows it zero-copy. - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder - .start_raster_nd(&transform, &["x", "y"], &[3, 2], None) - .unwrap(); - let view = crate::view_entries![1:3, 0:2, 0:3]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["t", "y", "x"], - source_shape: &[3, 2, 3], - view: view.as_slice(), - data_type: BandDataType::UInt8, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - let source: Vec = (0..18).collect(); - builder.band_data_writer().append_value(source.clone()); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - - // Visible region = source[6..18] (T=1 and T=2 planes). - assert_eq!(band.shape(), &[2, 2, 3]); - let buf = band.nd_buffer().unwrap(); - assert!(buf.is_contiguous()); - assert_eq!(buf.as_contiguous().unwrap(), &source[6..18]); - } - - #[test] - fn test_nd_buffer_permutation_and_slice_combined() { - // 2D source [Y=4, X=3]. View permutes (visible order [X, Y]) and - // slices Y from 1, step 2, steps 2. Expected: - // visible_shape = [3, 2] - // byte_strides = [step_X * stride_X_src, step_Y * stride_Y_src] - // = [1 * 1, 2 * 3] = [1, 6] - // byte_offset = start_X * stride_X_src + start_Y * stride_Y_src - // = 0 * 1 + 1 * 3 = 3 - let mut builder = RasterBuilder::new(1); - let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; - builder.start_raster_nd(&transform, &[], &[], None).unwrap(); - let view = [ - ViewEntry { - source_axis: 1, - start: 0, - step: 1, - steps: 3, - }, // X - ViewEntry { - source_axis: 0, - start: 1, - step: 2, - steps: 2, - }, // Y - ]; - builder - .start_band_with_view(StartBandWithViewArgs { - name: None, - dim_names: &["x", "y"], - source_shape: &[4, 3], - view: &view, - data_type: BandDataType::UInt8, - nodata: None, - outdb_uri: None, - outdb_format: None, - }) - .unwrap(); - builder - .band_data_writer() - .append_value((0u8..12).collect::>()); - builder.finish_band().unwrap(); - builder.finish_raster().unwrap(); - let array = builder.finish().unwrap(); - let rasters = RasterStructArray::new(&array); - let r = rasters.get(0).unwrap(); - let band = r.band(0).unwrap(); - let buf = band.nd_buffer().unwrap(); - assert_eq!(buf.shape, &[3, 2]); - assert_eq!(buf.strides, &[1, 6]); - assert_eq!(buf.offset, 3); - - // The permuted+strided layout (strides [1, 6]) is not C-order packed, - // so the buffer is non-contiguous and as_contiguous rejects it. - assert!(!buf.is_contiguous()); - assert!(buf.as_contiguous().is_err()); - } - #[test] fn test_nd_buffer_multidim_with_zero_axis() { // visible_shape contains a zero axis somewhere in the middle. The From 31fda8bb11a2cbb37209ef72631d297429f0a06c Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 15 Jun 2026 15:07:03 -0700 Subject: [PATCH 4/8] feat(rust/sedona-raster): BandRef::copy_into derive-with-overrides; with_view shares buffer zero-copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a band-driven derive API: BandRef::copy_into(builder, BandOverrides{..}) writes a derived band, inheriting unspecified fields from the source and carrying its bytes over. The data step is a separate append_data_into trait method — default copies via append_value; BandRefImpl overrides it to share the source row's backing Buffer zero-copy (append_band_data_from), keeping the Arrow buffer plumbing encapsulated (no raw Buffer accessor on the trait). with_view now composes the view and delegates to copy_into, so InDb derivation no longer copies the source bytes per call — it shares the backing buffer (verified by a ptr-equality test). Implements the RasterBuilder copy-ergonomics groundwork (DB-81); the copy_raster_from sibling and the RS_EnsureLoaded view-preserving passthrough follow. --- rust/sedona-raster/src/array.rs | 13 ++++ rust/sedona-raster/src/builder.rs | 111 +++++++++++++++++++----------- rust/sedona-raster/src/traits.rs | 72 +++++++++++++++++++ 3 files changed, 154 insertions(+), 42 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index 67dd1647b0..29ec26e526 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -21,6 +21,7 @@ use arrow_array::{ }; use arrow_schema::ArrowError; +use crate::builder::RasterBuilder; use crate::traits::{BandRef, Bands, NdBuffer, RasterRef}; use crate::view_entries::{ViewEntries, ViewEntry}; use sedona_schema::raster::{band_indices, band_view_indices, raster_indices, BandDataType}; @@ -141,6 +142,18 @@ impl<'a> BandRef for BandRefImpl<'a> { data_type: self.data_type, }) } + + /// Zero-copy override: share the source row's backing `Buffer` into the + /// builder (refcount bump) instead of copying the visible bytes. OutDb + /// bands have an empty data column by design. + fn append_data_into(&self, builder: &mut RasterBuilder) -> Result<(), ArrowError> { + if self.is_indb() { + builder.append_band_data_from(self.data_array, self.band_row) + } else { + builder.band_data_writer().append_value([]); + Ok(()) + } + } } /// Verify that every byte the view can address lies within `buffer_len` diff --git a/rust/sedona-raster/src/builder.rs b/rust/sedona-raster/src/builder.rs index 6abca4351b..6d3f05de66 100644 --- a/rust/sedona-raster/src/builder.rs +++ b/rust/sedona-raster/src/builder.rs @@ -29,7 +29,7 @@ use std::sync::Arc; use sedona_schema::raster::{BandDataType, RasterSchema}; -use crate::traits::{BandMetadata, BandRef, MetadataRef}; +use crate::traits::{BandMetadata, BandOverrides, BandRef, MetadataRef}; use crate::view_entries::{ViewEntries, ViewEntry}; /// Maximum byte length of an inline `BinaryViewArray` view. Views this short @@ -554,49 +554,24 @@ impl RasterBuilder { outdb_uri, outdb_format, } = args; - let source_shape: Vec = input.raw_source_shape().to_vec(); + // Compose the input band's existing view with the requested one, then + // delegate to `copy_into`: it writes the schema (inheriting source_shape + // and any unspecified fields from `input`) and carries the bytes over — + // zero-copy for an Arrow-backed input via `append_band_data_from`, + // copying only for a generic `BandRef`. let composed = ViewEntries::new(input.view().to_vec()).compose(&ViewEntries::new(view.to_vec()))?; - - // Inherit storage metadata from the input unless the caller has - // explicitly overridden it. For OutDb inputs this propagates the - // external pointer to the output; for InDb inputs the input's - // outdb_uri/outdb_format are typically None anyway. - let final_outdb_uri = outdb_uri.or_else(|| input.outdb_uri()); - let final_outdb_format = outdb_format.or_else(|| input.outdb_format()); - - // Reuse the internal start_band_with_view helper to perform - // validation + write the schema fields. - self.start_band_with_view(StartBandWithViewArgs { - name, - dim_names, - source_shape: &source_shape, - view: composed.as_slice(), - data_type: input.data_type(), - nodata, - outdb_uri: final_outdb_uri, - outdb_format: final_outdb_format, - })?; - - if input.is_indb() { - // InDb: nd_buffer().buffer is the source bytes — borrow them - // directly into `append_value` so the only copy is the one - // BinaryViewBuilder makes into its block. This is still - // a full source-bytes copy per `with_view` call, which - // undermines the "lazy slice" framing for large rasters. - // - // The principled fix is Arrow `BinaryView` buffer-sharing: - // the output's data row references the input's existing - // backing `Buffer` instead of copying. Tracked separately - // in the Raster Clean Up project. - let buf = input.nd_buffer()?; - self.band_data_writer().append_value(buf.buffer); - } else { - // OutDb: data column stays empty; the source bytes live at the - // inherited outdb_uri and are fetched lazily on read. - self.band_data_writer().append_value([]); - } - Ok(()) + input.copy_into( + self, + BandOverrides { + name, + dim_names: Some(dim_names), + view: Some(composed.as_slice()), + nodata, + outdb_uri, + outdb_format, + }, + ) } /// Convenience: start a 2D band with `dim_names=["y","x"]` and `shape=[height, width]`. @@ -2355,6 +2330,58 @@ mod tests { assert_eq!(out_band.raw_source_shape(), &[8]); } + #[test] + fn with_view_indb_shares_source_buffer_zero_copy() { + // 16 bytes (> inline threshold) so the band is block-backed and its + // backing Buffer can be shared rather than copied. + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + let mut ib = RasterBuilder::new(1); + ib.start_raster_nd(&transform, &["x"], &[16], None).unwrap(); + ib.start_band_nd(None, &["x"], &[16], BandDataType::UInt8, None, None, None) + .unwrap(); + ib.band_data_writer() + .append_value((0u8..16).collect::>()); + ib.finish_band().unwrap(); + ib.finish_raster().unwrap(); + let input_array = ib.finish().unwrap(); + let input_rasters = RasterStructArray::new(&input_array); + let input_raster = input_rasters.get(0).unwrap(); + let input_band = input_raster.band(0).unwrap(); + let input_ptr = input_band.nd_buffer().unwrap().buffer.as_ptr(); + + // Identity with_view: the output band must reference the same backing + // bytes as the input (refcount bump), not a fresh copy. + let mut ob = RasterBuilder::new(1); + ob.start_raster_nd(&transform, &["x"], &[16], None).unwrap(); + let id = crate::view_entries![0:16]; + ob.with_view(WithViewArgs { + name: None, + dim_names: &["x"], + input: input_band.as_ref(), + view: id.as_slice(), + nodata: None, + outdb_uri: None, + outdb_format: None, + }) + .unwrap(); + ob.finish_band().unwrap(); + ob.finish_raster().unwrap(); + let out_array = ob.finish().unwrap(); + let out_rasters = RasterStructArray::new(&out_array); + let out_raster = out_rasters.get(0).unwrap(); + let out_band = out_raster.band(0).unwrap(); + + assert_eq!( + input_ptr, + out_band.nd_buffer().unwrap().buffer.as_ptr(), + "with_view must share the source buffer zero-copy, not copy it" + ); + assert_eq!( + out_band.nd_buffer().unwrap().as_contiguous().unwrap(), + (0u8..16).collect::>().as_slice() + ); + } + #[test] fn with_view_chained_composes_into_single_view() { // Round 1: with_view layers (start=1, step=2, steps=4) → visible diff --git a/rust/sedona-raster/src/traits.rs b/rust/sedona-raster/src/traits.rs index 099464c07a..cb900b0049 100644 --- a/rust/sedona-raster/src/traits.rs +++ b/rust/sedona-raster/src/traits.rs @@ -18,6 +18,7 @@ use arrow_schema::ArrowError; use sedona_schema::raster::BandDataType; +use crate::builder::{RasterBuilder, StartBandWithViewArgs}; use crate::view_entries::ViewEntry; /// Recognized spatial dimension-name pairs, in band C-order: the slower- @@ -502,6 +503,25 @@ pub trait RasterRef { } } +/// Field overrides for [`BandRef::copy_into`]. Each field defaults to `None`, +/// meaning "inherit from the source band". `name` has no source on a `BandRef` +/// (band names live at the raster level), so it defaults to unnamed. +#[derive(Default)] +pub struct BandOverrides<'a> { + /// Name for the derived band (the source has none to inherit). + pub name: Option<&'a str>, + /// Override the dimension names; `None` inherits the source's. + pub dim_names: Option<&'a [&'a str]>, + /// Override the view (e.g. a composed slice); `None` inherits the source's. + pub view: Option<&'a [ViewEntry]>, + /// Override the nodata value; `None` inherits the source's. + pub nodata: Option<&'a [u8]>, + /// Override the OutDb URI; `None` inherits the source's. + pub outdb_uri: Option<&'a str>, + /// Override the OutDb format; `None` inherits the source's. + pub outdb_format: Option<&'a str>, +} + /// Trait for accessing a single band/variable within an N-D raster. /// /// This is the consumer interface. Implementations handle storage details @@ -689,6 +709,58 @@ pub trait BandRef { }; nodata_bytes_to_f64_lossless(bytes, &self.data_type()).map(Some) } + + /// Write a derived band into `builder`, inheriting every field not set in + /// `overrides` from `self`, and carrying over the source bytes. + /// + /// This is the canonical "derive a band from an existing one" path — it + /// replaces hand-rebuilding via `start_band_with_view` + a manual data + /// append (which silently drops the view or copies the bytes). The data + /// transfer is zero-copy when the implementation supports it; see + /// [`Self::append_data_into`]. + fn copy_into( + &self, + builder: &mut RasterBuilder, + overrides: BandOverrides<'_>, + ) -> Result<(), ArrowError> { + let inherited_dims = self.dim_names(); + let dim_names: Vec<&str> = match overrides.dim_names { + Some(d) => d.to_vec(), + None => inherited_dims, + }; + let source_shape = self.raw_source_shape().to_vec(); + let inherited_view = self.view().to_vec(); + let view: &[ViewEntry] = overrides.view.unwrap_or(inherited_view.as_slice()); + builder.start_band_with_view(StartBandWithViewArgs { + name: overrides.name, + dim_names: &dim_names, + source_shape: &source_shape, + view, + data_type: self.data_type(), + nodata: overrides.nodata.or_else(|| self.nodata()), + outdb_uri: overrides.outdb_uri.or_else(|| self.outdb_uri()), + outdb_format: overrides.outdb_format.or_else(|| self.outdb_format()), + })?; + self.append_data_into(builder) + } + + /// Append `self`'s band data as the current band's single `data` value. + /// + /// The default copies the visible source bytes via `append_value`. Arrow- + /// backed implementations override this to share the source row's backing + /// `Buffer` zero-copy (a refcount bump via + /// [`RasterBuilder::append_band_data_from`]), keeping the buffer plumbing + /// encapsulated rather than exposing a raw `Buffer` accessor on the trait. + /// Call after the band's schema has been written (e.g. by [`Self::copy_into`]). + fn append_data_into(&self, builder: &mut RasterBuilder) -> Result<(), ArrowError> { + if self.is_indb() { + let ndb = self.nd_buffer()?; + builder.band_data_writer().append_value(ndb.buffer); + } else { + builder.band_data_writer().append_value([]); + } + Ok(()) + } } /// Convert raw nodata bytes to f64 given a [`BandDataType`]. From 1aadd694ec6bce0bfb518c04d0dd5f2cab6295a2 Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 15 Jun 2026 19:52:09 -0700 Subject: [PATCH 5/8] feat(rust/sedona-raster): BandRef::copy_into derive-with-overrides (zero-copy) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a band-driven derive API: BandRef::copy_into(builder, BandOverrides{..}) writes a derived band, inheriting unspecified fields (dim names, shape, data type, nodata, OutDb pointers) from the source and carrying its bytes over. The data step is a separate append_data_into trait method — default copies via append_value; BandRefImpl overrides it to share the source row's backing Buffer zero-copy (append_band_data_from), keeping the Arrow buffer plumbing encapsulated (no raw Buffer accessor on the trait). Identity-view only: the derived band uses start_band_nd. View-carrying overrides land with the view machinery. First step of DB-81; consumers (RS_EnsureLoaded rebuild, etc.) migrate next. --- rust/sedona-raster/src/array.rs | 76 +++++++++++++++++++++++++++++++- rust/sedona-raster/src/traits.rs | 71 +++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 1 deletion(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index 7da4b3a6b4..9b7b4d30af 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -21,6 +21,7 @@ use arrow_array::{ }; use arrow_schema::ArrowError; +use crate::builder::RasterBuilder; use crate::traits::{BandRef, Bands, NdBuffer, RasterRef}; use crate::view_entries::ViewEntry; use sedona_schema::raster::{band_indices, raster_indices, BandDataType}; @@ -134,6 +135,18 @@ impl<'a> BandRef for BandRefImpl<'a> { data_type: self.data_type, }) } + + /// Zero-copy override: share the source row's backing `Buffer` into the + /// builder (refcount bump) instead of copying the visible bytes. OutDb + /// bands have an empty data column by design. + fn append_data_into(&self, builder: &mut RasterBuilder) -> Result<(), ArrowError> { + if self.is_indb() { + builder.append_band_data_from(self.data_array, self.band_row) + } else { + builder.band_data_writer().append_value([]); + Ok(()) + } + } } /// Arrow-backed implementation of RasterRef for a single raster row. @@ -605,7 +618,7 @@ impl<'a> RasterStructArray<'a> { mod tests { use super::*; use crate::builder::RasterBuilder; - use crate::traits::{BandMetadata, RasterMetadata}; + use crate::traits::{BandMetadata, BandOverrides, RasterMetadata}; use arrow_array::{ArrayRef, ListArray, StructArray, UInt32Array}; use arrow_buffer::{OffsetBuffer, ScalarBuffer}; use arrow_schema::{DataType, Fields}; @@ -615,6 +628,67 @@ mod tests { use sedona_testing::rasters::generate_test_rasters; use std::sync::Arc; + #[test] + fn copy_into_shares_buffer_zero_copy_and_overrides() { + // 16-byte InDb band (> inline threshold, so block-backed and shareable). + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + let mut ib = RasterBuilder::new(1); + ib.start_raster_nd(&transform, &["x"], &[16], None).unwrap(); + ib.start_band_nd( + Some("orig"), + &["x"], + &[16], + BandDataType::UInt8, + None, + None, + None, + ) + .unwrap(); + ib.band_data_writer() + .append_value((0u8..16).collect::>()); + ib.finish_band().unwrap(); + ib.finish_raster().unwrap(); + let input_array = ib.finish().unwrap(); + let input_rasters = RasterStructArray::new(&input_array); + let input_raster = input_rasters.get(0).unwrap(); + let input_band = input_raster.band(0).unwrap(); + let input_ptr = input_band.nd_buffer().unwrap().buffer.as_ptr(); + + // copy_into with a name override; everything else inherited. + let mut ob = RasterBuilder::new(1); + ob.start_raster_nd(&transform, &["x"], &[16], None).unwrap(); + input_band + .copy_into( + &mut ob, + BandOverrides { + name: Some("derived"), + ..Default::default() + }, + ) + .unwrap(); + ob.finish_band().unwrap(); + ob.finish_raster().unwrap(); + let out_array = ob.finish().unwrap(); + let out_rasters = RasterStructArray::new(&out_array); + let out_raster = out_rasters.get(0).unwrap(); + let out_band = out_raster.band(0).unwrap(); + + // Zero-copy: the derived band references the same backing bytes. + assert_eq!( + input_ptr, + out_band.nd_buffer().unwrap().buffer.as_ptr(), + "copy_into must share the source buffer, not copy it" + ); + assert_eq!( + out_band.nd_buffer().unwrap().as_contiguous().unwrap(), + (0u8..16).collect::>().as_slice() + ); + // Name overridden; dim names + data type inherited from the source. + assert_eq!(out_raster.band_name(0), Some("derived")); + assert_eq!(out_band.dim_names(), vec!["x"]); + assert_eq!(out_band.data_type(), BandDataType::UInt8); + } + #[test] fn test_array_basic_functionality() { // Create a simple raster for testing using the correct API diff --git a/rust/sedona-raster/src/traits.rs b/rust/sedona-raster/src/traits.rs index 6bfa69aa6a..d8faa8a74a 100644 --- a/rust/sedona-raster/src/traits.rs +++ b/rust/sedona-raster/src/traits.rs @@ -18,6 +18,7 @@ use arrow_schema::ArrowError; use sedona_schema::raster::BandDataType; +use crate::builder::RasterBuilder; use crate::view_entries::ViewEntry; /// Recognized spatial dimension-name pairs, in band C-order: the slower- @@ -496,6 +497,23 @@ pub trait RasterRef { } } +/// Field overrides for [`BandRef::copy_into`]. Each field defaults to `None`, +/// meaning "inherit from the source band". `name` has no source on a `BandRef` +/// (band names live at the raster level), so it defaults to unnamed. +#[derive(Default)] +pub struct BandOverrides<'a> { + /// Name for the derived band (the source has none to inherit). + pub name: Option<&'a str>, + /// Override the dimension names; `None` inherits the source's. + pub dim_names: Option<&'a [&'a str]>, + /// Override the nodata value; `None` inherits the source's. + pub nodata: Option<&'a [u8]>, + /// Override the OutDb URI; `None` inherits the source's. + pub outdb_uri: Option<&'a str>, + /// Override the OutDb format; `None` inherits the source's. + pub outdb_format: Option<&'a str>, +} + /// Trait for accessing a single band/variable within an N-D raster. /// /// This is the consumer interface. Implementations handle storage details @@ -667,6 +685,59 @@ pub trait BandRef { }; nodata_bytes_to_f64_lossless(bytes, &self.data_type()).map(Some) } + + /// Write a derived band into `builder`, inheriting every field not set in + /// `overrides` from `self`, and carrying the source bytes over. + /// + /// This is the canonical "derive a band from an existing one" path — it + /// replaces hand-rebuilding via `start_band_nd` + a manual data append. The + /// data transfer is zero-copy when the implementation supports it (see + /// [`Self::append_data_into`]). + /// + /// The derived band has an identity view: it carries the source's + /// dimension names, shape, data type, nodata, and OutDb pointers, but not a + /// non-identity `view`. (View-carrying overrides land with the view + /// machinery.) + fn copy_into( + &self, + builder: &mut RasterBuilder, + overrides: BandOverrides<'_>, + ) -> Result<(), ArrowError> { + let inherited_dims = self.dim_names(); + let dim_names: Vec<&str> = match overrides.dim_names { + Some(d) => d.to_vec(), + None => inherited_dims, + }; + let shape = self.raw_source_shape().to_vec(); + builder.start_band_nd( + overrides.name, + &dim_names, + &shape, + self.data_type(), + overrides.nodata.or_else(|| self.nodata()), + overrides.outdb_uri.or_else(|| self.outdb_uri()), + overrides.outdb_format.or_else(|| self.outdb_format()), + )?; + self.append_data_into(builder) + } + + /// Append `self`'s band data as the current band's single `data` value. + /// + /// The default copies the visible source bytes via `append_value`. Arrow- + /// backed implementations override this to share the source row's backing + /// `Buffer` zero-copy (a refcount bump via + /// [`RasterBuilder::append_band_data_from`]), keeping the buffer plumbing + /// encapsulated rather than exposing a raw `Buffer` accessor. Call after the + /// band's schema has been written (e.g. by [`Self::copy_into`]). + fn append_data_into(&self, builder: &mut RasterBuilder) -> Result<(), ArrowError> { + if self.is_indb() { + let ndb = self.nd_buffer()?; + builder.band_data_writer().append_value(ndb.buffer); + } else { + builder.band_data_writer().append_value([]); + } + Ok(()) + } } /// Convert raw nodata bytes to f64 given a [`BandDataType`]. From cc11952b2eec8af8d05bd6ac676ad461c8bae7cc Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 22 Jun 2026 15:45:35 -0700 Subject: [PATCH 6/8] fix(rust/sedona-raster): reject non-identity source views in copy_into copy_into emits an identity-view band and copies the source's visible bytes assuming offset 0 / canonical strides. Guard that precondition at the single dispatch point (covering both the default and the Arrow append_band_data_from data paths): a non-identity source view now errors loudly instead of silently copying mislocated bytes. Carrying views is a follow-up (the view machinery). --- rust/sedona-raster/src/traits.rs | 38 +++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/rust/sedona-raster/src/traits.rs b/rust/sedona-raster/src/traits.rs index d8faa8a74a..f4f08fe1ab 100644 --- a/rust/sedona-raster/src/traits.rs +++ b/rust/sedona-raster/src/traits.rs @@ -19,7 +19,7 @@ use arrow_schema::ArrowError; use sedona_schema::raster::BandDataType; use crate::builder::RasterBuilder; -use crate::view_entries::ViewEntry; +use crate::view_entries::{ViewEntries, ViewEntry}; /// Recognized spatial dimension-name pairs, in band C-order: the slower- /// varying Y-like (row) axis first, the faster-varying X-like (column) axis @@ -694,15 +694,30 @@ pub trait BandRef { /// data transfer is zero-copy when the implementation supports it (see /// [`Self::append_data_into`]). /// - /// The derived band has an identity view: it carries the source's - /// dimension names, shape, data type, nodata, and OutDb pointers, but not a - /// non-identity `view`. (View-carrying overrides land with the view - /// machinery.) + /// The derived band has an identity view, so the **source must also have + /// one**: a non-identity source view (slice, broadcast, permutation, or a + /// non-zero byte offset) is rejected with an error rather than silently + /// copying mislocated bytes, because the builder can't yet persist a + /// non-identity view to carry it over. Carrying views is the follow-up + /// (the view machinery); materialize with `RS_EnsureContiguous` until then. fn copy_into( &self, builder: &mut RasterBuilder, overrides: BandOverrides<'_>, ) -> Result<(), ArrowError> { + // `start_band_nd` writes the identity view, and the whole-buffer data + // copy below assumes the source's visible bytes start at offset 0 with + // canonical strides. A non-identity source view breaks both, so refuse + // it loudly here — the single dispatch point for both the default and + // the Arrow-backed (`append_band_data_from`) data paths. + if !ViewEntries::new(self.view().to_vec()).is_identity(self.raw_source_shape()) { + return Err(ArrowError::InvalidArgumentError( + "copy_into: source band has a non-identity view (slice, broadcast, \ + permutation, or offset); carrying views is not yet supported — \ + materialize the band (e.g. via RS_EnsureContiguous) first" + .into(), + )); + } let inherited_dims = self.dim_names(); let dim_names: Vec<&str> = match overrides.dim_names { Some(d) => d.to_vec(), @@ -1006,6 +1021,19 @@ mod tests { assert!(b.is_spatial_2d()); } + #[test] + fn copy_into_rejects_non_identity_view() { + // A sliced view (step 2 on the outer axis) is non-identity; copy_into + // can't carry it yet, so it must error rather than copy mislocated bytes. + let src = band(&["y", "x"], &[4, 5], &[ve(0, 0, 2, 2), ve(1, 0, 1, 5)]); + let mut ob = RasterBuilder::new(1); + let err = src + .copy_into(&mut ob, BandOverrides::default()) + .unwrap_err() + .to_string(); + assert!(err.contains("non-identity view"), "unexpected error: {err}"); + } + #[test] fn is_spatial_2d_latlon_is_true() { let b = band(&["lat", "lon"], &[4, 5], &[ve(0, 0, 1, 4), ve(1, 0, 1, 5)]); From 958ca069e23b48167500c51e6ad13f316916ae9c Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 22 Jun 2026 16:04:30 -0700 Subject: [PATCH 7/8] refactor(rust/sedona-raster): copy_into carries a composed band view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the in-copy_into non-identity guard with a future-proof seam: - Add RasterBuilder::start_band_nd_with_view, which persists an explicit view (identity -> the canonical null sentinel; non-identity -> errors, gated until view persistence lands, #897). - copy_into now composes BandOverrides.view onto the source's own view and forwards the result, so callers express overrides in the source's visible coordinates and never manage composition themselves. - Add BandOverrides.view. When non-identity view persistence lands, only the builder + reader + loader change — copy_into, append_data_into, and BandOverrides are frozen. --- rust/sedona-raster/src/array.rs | 55 +++++++++++++++++++++ rust/sedona-raster/src/builder.rs | 48 +++++++++++++++++++ rust/sedona-raster/src/traits.rs | 79 +++++++++++++++++++++---------- 3 files changed, 158 insertions(+), 24 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index 9b7b4d30af..c463532771 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -689,6 +689,61 @@ mod tests { assert_eq!(out_band.data_type(), BandDataType::UInt8); } + #[test] + fn copy_into_with_identity_override_view_succeeds() { + // An explicit identity override composes back to the identity, so it is + // accepted and behaves exactly like the inherited (None) case — this + // exercises the new `BandOverrides::view` path end to end. + let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0]; + let mut ib = RasterBuilder::new(1); + ib.start_raster_nd(&transform, &["x"], &[4], None).unwrap(); + ib.start_band_nd( + Some("orig"), + &["x"], + &[4], + BandDataType::UInt8, + None, + None, + None, + ) + .unwrap(); + ib.band_data_writer().append_value(vec![1u8, 2, 3, 4]); + ib.finish_band().unwrap(); + ib.finish_raster().unwrap(); + let in_array = ib.finish().unwrap(); + let in_rasters = RasterStructArray::new(&in_array); + let in_raster = in_rasters.get(0).unwrap(); + let in_band = in_raster.band(0).unwrap(); + + let identity = [ViewEntry { + source_axis: 0, + start: 0, + step: 1, + steps: 4, + }]; + let mut ob = RasterBuilder::new(1); + ob.start_raster_nd(&transform, &["x"], &[4], None).unwrap(); + in_band + .copy_into( + &mut ob, + BandOverrides { + view: Some(&identity), + ..Default::default() + }, + ) + .unwrap(); + ob.finish_band().unwrap(); + ob.finish_raster().unwrap(); + let out_array = ob.finish().unwrap(); + let out_rasters = RasterStructArray::new(&out_array); + let out_raster = out_rasters.get(0).unwrap(); + let out_band = out_raster.band(0).unwrap(); + assert_eq!( + out_band.nd_buffer().unwrap().as_contiguous().unwrap(), + &[1u8, 2, 3, 4] + ); + } + #[test] fn test_array_basic_functionality() { // Create a simple raster for testing using the correct API diff --git a/rust/sedona-raster/src/builder.rs b/rust/sedona-raster/src/builder.rs index 8747917086..04ecb1740c 100644 --- a/rust/sedona-raster/src/builder.rs +++ b/rust/sedona-raster/src/builder.rs @@ -30,6 +30,7 @@ use std::sync::Arc; use sedona_schema::raster::{BandDataType, RasterSchema}; use crate::traits::{BandMetadata, MetadataRef}; +use crate::view_entries::{ViewEntries, ViewEntry}; /// Maximum byte length of an inline `BinaryViewArray` view. Views this short /// store their bytes in the 16-byte view itself; longer views reference a data @@ -387,6 +388,53 @@ impl RasterBuilder { Ok(()) } + /// Like [`Self::start_band_nd`], but persists an explicit band `view` + /// (a window of offsets/steps over the source `shape`) instead of the + /// implicit identity. + /// + /// **Today only the identity view is accepted**: a non-identity view + /// returns an error, because persisting one isn't wired through the band + /// reader or the `RS_EnsureLoaded` round-trip yet (tracked in + /// ). An identity `view` is + /// stored as the canonical null sentinel, exactly as `start_band_nd` does, + /// so this is a drop-in for callers that want to forward a (currently + /// always identity) view. When view persistence lands, only this method + /// changes — callers routing through it (e.g. `BandRef::copy_into`) are + /// unaffected. + #[allow(clippy::too_many_arguments)] + pub fn start_band_nd_with_view( + &mut self, + name: Option<&str>, + dim_names: &[&str], + shape: &[i64], + data_type: BandDataType, + nodata: Option<&[u8]>, + outdb_uri: Option<&str>, + outdb_format: Option<&str>, + view: &[ViewEntry], + ) -> Result<(), ArrowError> { + // Reject up front — before any column appends — so a rejected view can + // never leave the builder in a half-written state. + if !ViewEntries::new(view.to_vec()).is_identity(shape) { + return Err(ArrowError::InvalidArgumentError( + "start_band_nd_with_view: persisting a non-identity band view is \ + not yet supported (see \ + https://github.com/apache/sedona-db/issues/897); materialize the \ + band (e.g. via RS_EnsureContiguous) first" + .into(), + )); + } + self.start_band_nd( + name, + dim_names, + shape, + data_type, + nodata, + outdb_uri, + outdb_format, + ) + } + /// Convenience: start a 2D band with `dim_names=["y","x"]` and `shape=[height, width]`. /// /// Must be called after `start_raster_2d` / `start_raster_2d` which sets diff --git a/rust/sedona-raster/src/traits.rs b/rust/sedona-raster/src/traits.rs index f4f08fe1ab..4d234fd16e 100644 --- a/rust/sedona-raster/src/traits.rs +++ b/rust/sedona-raster/src/traits.rs @@ -512,6 +512,13 @@ pub struct BandOverrides<'a> { pub outdb_uri: Option<&'a str>, /// Override the OutDb format; `None` inherits the source's. pub outdb_format: Option<&'a str>, + /// View to apply to the derived band, expressed in the **source's visible + /// coordinates**. [`BandRef::copy_into`] composes it onto the source's own + /// view for you — you don't manage that composition and don't need to know + /// whether the source already carries a view. `None` inherits the source's + /// view unchanged. (A non-identity result isn't persistable yet; see + /// [`RasterBuilder::start_band_nd_with_view`].) + pub view: Option<&'a [ViewEntry]>, } /// Trait for accessing a single band/variable within an N-D raster. @@ -694,37 +701,39 @@ pub trait BandRef { /// data transfer is zero-copy when the implementation supports it (see /// [`Self::append_data_into`]). /// - /// The derived band has an identity view, so the **source must also have - /// one**: a non-identity source view (slice, broadcast, permutation, or a - /// non-zero byte offset) is rejected with an error rather than silently - /// copying mislocated bytes, because the builder can't yet persist a - /// non-identity view to carry it over. Carrying views is the follow-up - /// (the view machinery); materialize with `RS_EnsureContiguous` until then. + /// The derived band's view is the source's own view with any + /// `overrides.view` **composed on top for you**: express an override in the + /// source's *visible* coordinates and `copy_into` composes it against the + /// source's view — callers never manage that composition and don't need to + /// know whether the source already carries one. `overrides.view = None` + /// inherits the source's view unchanged. + /// + /// A non-identity effective view can't be persisted yet, so it errors (the + /// gate lives in [`RasterBuilder::start_band_nd_with_view`]); in practice + /// today the source is identity-viewed and any override must compose back + /// to the identity. When view persistence lands + /// () this method is + /// unchanged — it already carries the view. fn copy_into( &self, builder: &mut RasterBuilder, overrides: BandOverrides<'_>, ) -> Result<(), ArrowError> { - // `start_band_nd` writes the identity view, and the whole-buffer data - // copy below assumes the source's visible bytes start at offset 0 with - // canonical strides. A non-identity source view breaks both, so refuse - // it loudly here — the single dispatch point for both the default and - // the Arrow-backed (`append_band_data_from`) data paths. - if !ViewEntries::new(self.view().to_vec()).is_identity(self.raw_source_shape()) { - return Err(ArrowError::InvalidArgumentError( - "copy_into: source band has a non-identity view (slice, broadcast, \ - permutation, or offset); carrying views is not yet supported — \ - materialize the band (e.g. via RS_EnsureContiguous) first" - .into(), - )); - } let inherited_dims = self.dim_names(); let dim_names: Vec<&str> = match overrides.dim_names { Some(d) => d.to_vec(), None => inherited_dims, }; let shape = self.raw_source_shape().to_vec(); - builder.start_band_nd( + // Compose the caller's override (if any) onto the source's own view, so + // the override is interpreted in the source's visible space and the + // caller doesn't have to. `None` keeps the source view unchanged. + let source_view = ViewEntries::new(self.view().to_vec()); + let effective_view = match overrides.view { + Some(v) => source_view.compose(&ViewEntries::new(v.to_vec()))?, + None => source_view, + }; + builder.start_band_nd_with_view( overrides.name, &dim_names, &shape, @@ -732,6 +741,7 @@ pub trait BandRef { overrides.nodata.or_else(|| self.nodata()), overrides.outdb_uri.or_else(|| self.outdb_uri()), overrides.outdb_format.or_else(|| self.outdb_format()), + effective_view.as_slice(), )?; self.append_data_into(builder) } @@ -1022,16 +1032,37 @@ mod tests { } #[test] - fn copy_into_rejects_non_identity_view() { - // A sliced view (step 2 on the outer axis) is non-identity; copy_into - // can't carry it yet, so it must error rather than copy mislocated bytes. + fn copy_into_rejects_non_identity_source_view() { + // A sliced source view (step 2 on the outer axis) composes to a + // non-identity effective view; copy_into can't persist it yet, so it + // must error rather than copy mislocated bytes. let src = band(&["y", "x"], &[4, 5], &[ve(0, 0, 2, 2), ve(1, 0, 1, 5)]); let mut ob = RasterBuilder::new(1); let err = src .copy_into(&mut ob, BandOverrides::default()) .unwrap_err() .to_string(); - assert!(err.contains("non-identity view"), "unexpected error: {err}"); + assert!(err.contains("non-identity"), "unexpected error: {err}"); + } + + #[test] + fn copy_into_rejects_non_identity_override_view() { + // Identity source, but the caller's override slices it (step 2); the + // composed effective view is non-identity and can't be persisted yet. + let src = band(&["y", "x"], &[4, 5], &[ve(0, 0, 1, 4), ve(1, 0, 1, 5)]); + let override_view = [ve(0, 0, 2, 2), ve(1, 0, 1, 5)]; + let mut ob = RasterBuilder::new(1); + let err = src + .copy_into( + &mut ob, + BandOverrides { + view: Some(&override_view), + ..Default::default() + }, + ) + .unwrap_err() + .to_string(); + assert!(err.contains("non-identity"), "unexpected error: {err}"); } #[test] From 85b4ff60988d9a027b38645b943c268d5f88647e Mon Sep 17 00:00:00 2001 From: jameswillis Date: Mon, 22 Jun 2026 16:37:01 -0700 Subject: [PATCH 8/8] fix(rust/sedona-raster): migrate copy_into tests to RasterStructArray::try_new main renamed RasterStructArray::new -> try_new; update the copy_into tests (merged in from this branch) to match. --- rust/sedona-raster/src/array.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/sedona-raster/src/array.rs b/rust/sedona-raster/src/array.rs index 0f1d71809c..33e3a46a70 100644 --- a/rust/sedona-raster/src/array.rs +++ b/rust/sedona-raster/src/array.rs @@ -589,7 +589,7 @@ mod tests { ib.finish_band().unwrap(); ib.finish_raster().unwrap(); let input_array = ib.finish().unwrap(); - let input_rasters = RasterStructArray::new(&input_array); + let input_rasters = RasterStructArray::try_new(&input_array).unwrap(); let input_raster = input_rasters.get(0).unwrap(); let input_band = input_raster.band(0).unwrap(); let input_ptr = input_band.nd_buffer().unwrap().buffer.as_ptr(); @@ -609,7 +609,7 @@ mod tests { ob.finish_band().unwrap(); ob.finish_raster().unwrap(); let out_array = ob.finish().unwrap(); - let out_rasters = RasterStructArray::new(&out_array); + let out_rasters = RasterStructArray::try_new(&out_array).unwrap(); let out_raster = out_rasters.get(0).unwrap(); let out_band = out_raster.band(0).unwrap(); @@ -651,7 +651,7 @@ mod tests { ib.finish_band().unwrap(); ib.finish_raster().unwrap(); let in_array = ib.finish().unwrap(); - let in_rasters = RasterStructArray::new(&in_array); + let in_rasters = RasterStructArray::try_new(&in_array).unwrap(); let in_raster = in_rasters.get(0).unwrap(); let in_band = in_raster.band(0).unwrap(); @@ -675,7 +675,7 @@ mod tests { ob.finish_band().unwrap(); ob.finish_raster().unwrap(); let out_array = ob.finish().unwrap(); - let out_rasters = RasterStructArray::new(&out_array); + let out_rasters = RasterStructArray::try_new(&out_array).unwrap(); let out_raster = out_rasters.get(0).unwrap(); let out_band = out_raster.band(0).unwrap(); assert_eq!(