paleolimbot commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3250666496
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -115,8 +115,11 @@ impl RasterSchema {
/// describing how the visible axis maps onto the band's source shape.
/// The field is nullable: a null view denotes the identity view
/// `[(i, 0, 1, source_shape[i]) for i in 0..ndim]` and is the canonical
- /// representation for any band whose data has not been sliced. See
- /// `RasterSchema` doc for full semantics.
+ /// representation for any band whose data has not been sliced. An
+ /// empty (non-null, zero-length) list is read equivalently to NULL
+ /// for forward compatibility with producers that emit one form versus
+ /// the other; writers should prefer NULL. See `RasterSchema` doc for
+ /// full semantics.
Review Comment:
Can we just say it has to be NULL since we just invented this?
##########
rust/sedona-raster/src/traits.rs:
##########
@@ -667,6 +673,188 @@ pub trait BandRef {
}
}
+/// Derive the visible (per-axis) shape from a view.
+///
+/// `view[k].steps` is the visible extent along view axis `k` after slicing /
+/// broadcasting. Callers should treat the returned shape as authoritative for
+/// the visible region; `source_shape` is only meaningful in conjunction with
+/// the per-entry `source_axis`.
+///
+/// `validate_view` guarantees `steps >= 0`, so the `as u64` cast is lossless
+/// when the input has already been validated. Callers that haven't validated
+/// yet should still call `validate_view` first.
+pub(crate) fn visible_shape_from_view(view: &[ViewEntry]) -> Vec<u64> {
+ view.iter().map(|v| v.steps as u64).collect()
+}
+
+/// True iff `view` is the canonical identity over a C-order source buffer:
+/// every visible axis `k` is a no-op (no permutation, no slice offset, no
+/// stride/reverse, full coverage of the corresponding source axis).
+///
+/// Identity views let the reader borrow the underlying `data` column
+/// directly through `BandRef::contiguous_data()` and `BandRef::data()`
+/// with no allocation or copy. Non-identity views must materialize a
+/// row-major copy on first byte access.
+///
+/// Callers should validate the view first; this function checks lengths
+/// defensively but trusts the inputs otherwise.
+pub(crate) fn is_identity_view(view: &[ViewEntry], source_shape: &[u64]) ->
bool {
+ if view.len() != source_shape.len() {
+ return false;
+ }
+ view.iter().enumerate().all(|(k, v)| {
+ v.source_axis == k as i64
+ && v.start == 0
+ && v.step == 1
+ && v.steps >= 0
+ && (v.steps as u64) == source_shape[k]
+ })
+}
+
+/// Compose `next_view` (a view spec over `input_view`'s visible axes)
+/// with `input_view` (a view spec over a band's `source_shape`), producing
+/// a single view over the same `source_shape` that represents the result
+/// of viewing the input through `next_view`.
+///
+/// `next_view[k].source_axis` indexes into `input_view`'s visible axes
+/// (`0..input_view.len()`), NOT into the underlying source axes. This
+/// lets callers (e.g. lazy slicing) describe the new view in terms of
+/// what they see, without knowing the input's internal layout.
+///
+/// Math: for each output axis `k`, walking through `input_view` resolves
+/// the source axis and translates start/step:
+/// - `out.source_axis = input_view[next.source_axis].source_axis`
+/// - `out.start = input.start + next.start * input.step`
+/// - `out.step = next.step * input.step`
+/// - `out.steps = next.steps`
+///
+/// Uses checked arithmetic at every step. Returns the composed view; the
+/// caller must `validate_view` it against the source_shape before use.
+pub(crate) fn compose_view(
+ input_view: &[ViewEntry],
+ next_view: &[ViewEntry],
+) -> Result<Vec<ViewEntry>, ArrowError> {
+ let mut out = Vec::with_capacity(next_view.len());
+ for (k, next) in next_view.iter().enumerate() {
+ if next.source_axis < 0 || (next.source_axis as usize) >=
input_view.len() {
Review Comment:
Should `source_axis` be a `usize`? I'm wary of using `as` here given that
this is sensitive territory (will panic on out of bounds) although it is
probably fine.
##########
rust/sedona-raster/src/traits.rs:
##########
@@ -667,6 +673,188 @@ pub trait BandRef {
}
}
+/// Derive the visible (per-axis) shape from a view.
+///
+/// `view[k].steps` is the visible extent along view axis `k` after slicing /
+/// broadcasting. Callers should treat the returned shape as authoritative for
+/// the visible region; `source_shape` is only meaningful in conjunction with
+/// the per-entry `source_axis`.
+///
+/// `validate_view` guarantees `steps >= 0`, so the `as u64` cast is lossless
+/// when the input has already been validated. Callers that haven't validated
+/// yet should still call `validate_view` first.
+pub(crate) fn visible_shape_from_view(view: &[ViewEntry]) -> Vec<u64> {
+ view.iter().map(|v| v.steps as u64).collect()
+}
Review Comment:
These should probably be all `i64s` unless there is a compelling reason to
use the unsigned version to minimize the number of times we have to `as`
between signed and unsigned types.
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -374,6 +377,184 @@ 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`.
+ #[allow(clippy::too_many_arguments)]
+ pub(crate) fn start_band_with_view(
+ &mut self,
+ name: Option<&str>,
+ dim_names: &[&str],
+ source_shape: &[u64],
+ view: &[ViewEntry],
+ data_type: BandDataType,
+ nodata: Option<&[u8]>,
+ outdb_uri: Option<&str>,
+ outdb_format: Option<&str>,
+ ) -> Result<(), ArrowError> {
+ 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()
+ )));
+ }
+
+ validate_view(view, 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(),
+ visible_shape_from_view(view),
+ ));
+
+ 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.
+ #[allow(clippy::too_many_arguments)]
+ pub fn with_view(
Review Comment:
Here as well (you can probably use a common Args struct)
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -1717,222 +1902,1410 @@ mod tests {
}
#[test]
- fn test_start_band_rejects_zero_dim() {
- // 0-D bands carry no spatial extent and no caller has a use for
- // them. start_band_nd must reject an empty dim_names slice eagerly so
- // the malformed band never reaches the buffer layer.
- 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_nd(None, &[], &[], BandDataType::UInt8, None, None,
None)
- .unwrap_err();
- assert!(
- err.to_string().contains("0-dimensional"),
- "unexpected error: {err}"
- );
- }
-
- #[test]
- fn test_contiguous_data_identity_via_start_band_is_borrowed() {
- // 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.
+ 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"], &[3, 2], None)
+ .start_raster_nd(&transform, &["x", "y"], &[5, 4], None)
.unwrap();
+
+ let view = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 4,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 5,
+ },
+ ];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
&["y", "x"],
- &[2, 3],
+ &[4, 5],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- let pixels: Vec<u8> = (0..6).collect();
- builder.band_data_writer().append_value(pixels.clone());
+ 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();
-
- // Visible shape comes from the synthesised identity view.
- assert_eq!(band.shape(), &[2, 3]);
- assert_eq!(band.raw_source_shape(), &[2, 3]);
-
+ assert_eq!(band.shape(), &[4, 5]);
+ assert_eq!(band.raw_source_shape(), &[4, 5]);
let buf = band.nd_buffer().unwrap();
- assert_eq!(buf.strides, &[3, 1]);
+ assert_eq!(buf.strides, &[5, 1]);
assert_eq!(buf.offset, 0);
-
- let bytes = band.contiguous_data().unwrap();
- assert!(matches!(bytes, Cow::Borrowed(_)));
- assert_eq!(&*bytes, pixels.as_slice());
}
#[test]
- fn test_view_field_is_null_for_identity_band() {
- // Schema invariant: identity views are stored as null list rows so
- // the canonical "no slice" case costs no Arrow space. Confirm by
- // poking the raw column.
- use arrow_array::Array;
-
+ fn test_view_slice_nd_buffer_and_contiguous_data() {
+ // 1D source of size 8 (UInt8), view (start=1, step=2, steps=3) selects
+ // elements at byte offsets 1, 3, 5. Source: 0..8.
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"], &[2, 2], None)
+ .start_raster_nd(&transform, &["x"], &[3], None)
.unwrap();
+
+ let view = [ViewEntry {
+ source_axis: 0,
+ start: 1,
+ step: 2,
+ steps: 3,
+ }];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
- &[2, 2],
+ &["x"],
+ &[8],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 4]);
+ 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 bands_list = array
- .column(sedona_schema::raster::raster_indices::BANDS)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- let bands_struct = bands_list
- .values()
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
- let view_list = bands_struct
- .column(sedona_schema::raster::band_indices::VIEW)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- assert_eq!(view_list.len(), 1);
- assert!(
- view_list.is_null(0),
- "identity-view band should serialise as a null view row"
- );
+ let rasters = RasterStructArray::new(&array);
+ let r = rasters.get(0).unwrap();
+ let band = r.band(0).unwrap();
+
+ assert_eq!(band.shape(), &[3]);
+ assert_eq!(band.raw_source_shape(), &[8]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.shape, &[3]);
+ assert_eq!(buf.strides, &[2]);
+ assert_eq!(buf.offset, 1);
+
+ // Materialised contiguous bytes should be [1, 3, 5].
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[1u8, 3, 5]);
+ assert!(matches!(bytes, std::borrow::Cow::Owned(_)));
}
#[test]
- fn test_band_spatial_dim_size_mismatch_errors() {
+ fn test_view_broadcast() {
+ // Broadcast: source size 1, step=0 → expose the same byte 4 times.
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"], &[4, 4], None)
+ .start_raster_nd(&transform, &["x"], &[4], None)
.unwrap();
- // Band has "x" and "y" but x-size disagrees with top-level shape.
+
+ let view = [ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 0,
+ steps: 4,
+ }];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
- &[4, 8],
+ &["x"],
+ &[1],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 32]);
+ builder.band_data_writer().append_value(vec![42u8]);
builder.finish_band().unwrap();
+ builder.finish_raster().unwrap();
- let err = builder.finish_raster().unwrap_err();
- let msg = err.to_string();
- assert!(
- msg.contains("has size 8") && msg.contains("expected 4"),
- "unexpected error: {msg}"
- );
+ 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]);
+ assert_eq!(buf.strides, &[0]);
+ assert_eq!(buf.offset, 0);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[42u8, 42, 42, 42]);
}
#[test]
- fn test_view_null_round_trips_through_arrow_ipc() {
- // Schema invariant: a band built via start_band_nd serialises with a
- // null view row, and the null must survive an Arrow IPC round-trip.
- // If a future change accidentally writes a non-null empty list
- // instead, downstream readers (DuckDB, PyArrow, sedona-py) will
- // disagree about whether the view is identity.
-
+ fn test_view_permutation_transpose() {
+ // 2×3 source (UInt8), values 0..6 in C-order:
+ // row 0: [0, 1, 2]
+ // row 1: [3, 4, 5]
+ // Transposed view exposes axes (cols, rows) → 3×2:
+ // row 0: [0, 3]
+ // row 1: [1, 4]
+ // row 2: [2, 5]
let mut builder = RasterBuilder::new(1);
let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0];
+ // The transposed visible shape on the spatial axes would conflict with
+ // a 2D spatial grid; declare a single non-spatial dim "i" so the
+ // strict spatial check is trivially satisfied.
+ builder.start_raster_nd(&transform, &[], &[], None).unwrap();
+
+ let view = [
+ // visible axis 0 reads source axis 1 (cols), full extent 3
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ // visible axis 1 reads source axis 0 (rows), full extent 2
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ];
builder
- .start_raster_nd(&transform, &["x", "y"], &[3, 2], None)
- .unwrap();
- builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
+ &["a", "b"],
&[2, 3],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 6]);
+ builder
+ .band_data_writer()
+ .append_value(vec![0u8, 1, 2, 3, 4, 5]);
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(
- "raster",
- array.data_type().clone(),
- true,
- )) as arrow_schema::FieldRef]));
- let batch = RecordBatch::try_new(schema.clone(),
vec![Arc::new(array.clone())]).unwrap();
+ let rasters = RasterStructArray::new(&array);
+ let r = rasters.get(0).unwrap();
+ let band = r.band(0).unwrap();
- let mut buf: Vec<u8> = Vec::new();
- {
- let mut writer = StreamWriter::try_new(&mut buf,
schema.as_ref()).unwrap();
- writer.write(&batch).unwrap();
- writer.finish().unwrap();
- }
+ assert_eq!(band.shape(), &[3, 2]);
+ assert_eq!(band.raw_source_shape(), &[2, 3]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.strides, &[1, 3]); // visible axis 0 → source col
stride; visible axis 1 → source row stride
- let cursor = Cursor::new(buf);
- let reader = StreamReader::try_new(cursor, None).unwrap();
- let batches: Vec<_> = reader.collect::<Result<Vec<_>, _>>().unwrap();
- assert_eq!(batches.len(), 1);
- let restored_struct = batches[0]
- .column(0)
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[0u8, 3, 1, 4, 2, 5]);
+ }
- let bands_list = restored_struct
- .column(sedona_schema::raster::raster_indices::BANDS)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- let bands_struct = bands_list
- .values()
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
- let view_list = bands_struct
- .column(sedona_schema::raster::band_indices::VIEW)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- assert_eq!(view_list.len(), 1);
- assert!(
- view_list.is_null(0),
- "identity-view band must remain a null view row after IPC
round-trip"
- );
+ #[test]
+ fn test_view_empty_axis() {
+ // steps=0 → empty visible axis. contiguous_data must succeed and
+ // return an empty buffer.
+ 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: 1,
+ steps: 0,
+ }];
+ builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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();
+ assert_eq!(band.shape(), &[0]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.shape, &[0]);
+ let bytes = band.contiguous_data().unwrap();
+ assert!(bytes.is_empty());
+ }
+
+ #[test]
+ fn test_start_band_rejects_zero_dim() {
+ // 0-D bands carry no spatial extent and no caller has a use for
+ // them. start_band_nd must reject an empty dim_names slice eagerly so
+ // the malformed band never reaches the buffer layer.
+ 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_nd(None, &[], &[], BandDataType::UInt8, None, None,
None)
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("0-dimensional"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[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(None, &[], &[], &[], BandDataType::UInt8,
None, None, None)
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("0-dimensional"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_view_validation_rejects_out_of_range_start() {
+ 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: 8,
+ step: 1,
+ steps: 1,
+ }];
+ let err = builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("out of range"),
+ "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(
+ None,
+ &["x"],
+ &[7],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("out of range"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_view_validation_rejects_duplicate_source_axis() {
+ 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: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ];
+ let err = builder
+ .start_band_with_view(
+ None,
+ &["a", "b"],
+ &[2, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("permutation"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_contiguous_data_identity_via_start_band_is_borrowed() {
+ // 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<u8> = (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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert!(matches!(bytes, Cow::Borrowed(_)));
+ assert_eq!(&*bytes, pixels.as_slice());
+ }
+
+ #[test]
+ fn test_contiguous_data_explicit_identity_view_is_borrowed() {
+ // 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 Cow::Borrowed fast path.
+ use std::borrow::Cow;
+
+ 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 = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["y", "x"],
+ &[2, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ let pixels: Vec<u8> = (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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert!(matches!(bytes, Cow::Borrowed(_)));
+ assert_eq!(&*bytes, pixels.as_slice());
+ }
+
+ #[test]
+ fn test_contiguous_data_zero_step_broadcast_2d() {
+ // 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(
+ None,
+ &["row", "col"],
+ &[1, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[10u8, 20, 30, 10, 20, 30, 10, 20, 30, 10, 20,
30]);
+ }
+
+ #[test]
+ fn test_contiguous_data_negative_step_full_reverse() {
+ // 1D source [0..8] with start=7, step=-1, steps=8 walks the source
+ // backwards. Byte stride must be negative; offset lands on the last
+ // source element.
+ 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: 7,
+ step: -1,
+ steps: 8,
+ }];
+ builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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, &[8]);
+ assert_eq!(buf.strides, &[-1]);
+ assert_eq!(buf.offset, 7);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[7u8, 6, 5, 4, 3, 2, 1, 0]);
+ }
+
+ #[test]
+ fn test_contiguous_data_negative_step_strided_reverse() {
+ // 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(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[6u8, 4, 2]);
+ }
+
+ #[test]
+ fn test_view_field_is_null_for_identity_band() {
+ // Schema invariant: identity views are stored as null list rows so
+ // the canonical "no slice" case costs no Arrow space. Confirm by
+ // poking the raw column.
+ use arrow_array::Array;
+
+ 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"], &[2, 2], None)
+ .unwrap();
+ builder
+ .start_band_nd(
+ None,
+ &["y", "x"],
+ &[2, 2],
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ builder.band_data_writer().append_value(vec![0u8; 4]);
+ builder.finish_band().unwrap();
+ builder.finish_raster().unwrap();
+
+ let array = builder.finish().unwrap();
+ let bands_list = array
+ .column(sedona_schema::raster::raster_indices::BANDS)
+ .as_any()
+ .downcast_ref::<ListArray>()
+ .unwrap();
+ let bands_struct = bands_list
+ .values()
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .unwrap();
+ let view_list = bands_struct
+ .column(sedona_schema::raster::band_indices::VIEW)
+ .as_any()
+ .downcast_ref::<ListArray>()
+ .unwrap();
+ assert_eq!(view_list.len(), 1);
+ assert!(
+ view_list.is_null(0),
+ "identity-view band should serialise as a null view row"
+ );
+ }
+
+ #[test]
+ fn test_band_spatial_dim_size_mismatch_errors() {
+ 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"], &[4, 4], None)
+ .unwrap();
+ // Band has "x" and "y" but x-size disagrees with top-level shape.
+ builder
+ .start_band_nd(
+ None,
+ &["y", "x"],
+ &[4, 8],
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ builder.band_data_writer().append_value(vec![0u8; 32]);
+ builder.finish_band().unwrap();
+
+ let err = builder.finish_raster().unwrap_err();
+ let msg = err.to_string();
+ assert!(
+ msg.contains("has size 8") && msg.contains("expected 4"),
+ "unexpected error: {msg}"
+ );
+ }
+
+ #[test]
+ fn test_contiguous_data_float32_fast_path() {
+ // Multi-byte dtype on the contiguous innermost-axis fast path:
+ // a 2D explicit-identity view over Float32 should still emit
+ // bytes by `extend_from_slice` and produce the exact source
+ // payload back. Catches a regression where the fast path
+ // 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, so contiguous_data() materialises through the
+ // fast path. Inner stride = dtype_size = 4 → fast path is taken.
+ let view = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["y", "x"],
+ &[3, 3], // 3x3 source
+ &view,
+ BandDataType::Float32,
+ None,
+ None,
+ None,
Review Comment:
A BandBuilder may help you quite a bit here since there are a lot of `None`
that a default constructor could fill in
##########
rust/sedona-raster/src/array.rs:
##########
@@ -829,11 +1192,137 @@ 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<Vec<(i64, i64, i64, i64)>>,
+ nulls: Option<Vec<bool>>,
+ ) -> ArrayRef {
+ let mut offsets: Vec<i32> = vec![0];
+ let mut sa: Vec<i64> = vec![];
+ let mut start: Vec<i64> = vec![];
+ let mut step: Vec<i64> = vec![];
+ let mut steps: Vec<i64> = 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::<Int64Type>::from(sa))
as ArrayRef,
+
Arc::new(arrow_array::PrimitiveArray::<Int64Type>::from(start)) as ArrayRef,
+ Arc::new(arrow_array::PrimitiveArray::<Int64Type>::from(step))
as ArrayRef,
+
Arc::new(arrow_array::PrimitiveArray::<Int64Type>::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_has_negative_steps() {
+ // Schema accepts negative Int64 in the steps field, but validate_view
+ // rejects it. The reader path must surface that as None — never
+ // hand back a band whose visible_shape would underflow.
+ let array = build_explicit_view_raster();
+ 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);
+ assert!(rasters.get(0).unwrap().band(0).is_err());
+ }
+
+ #[test]
+ fn band_returns_none_when_view_source_axis_out_of_range() {
+ let array = build_explicit_view_raster();
+ let bad_view = make_band_view_list(vec![vec![(5, 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());
+ }
+
+ #[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());
+ }
+
+ #[test]
+ fn band_returns_none_when_view_has_duplicate_source_axis() {
+ // Need a 2-D source_shape so two entries with source_axis=0 are
+ // legal in length but illegal as a permutation.
+ 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();
+ builder
+ .start_band_with_view(
+ None,
+ &["a", "b"],
+ &[2, 3],
+ &[
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ],
Review Comment:
I may be misunderstanding the `ViewEntry` system here, but is there a reason
why we encode `source_axis` instead of force the source axis to be the same
length as the shape that it's viewing?
##########
rust/sedona-raster/src/array.rs:
##########
@@ -86,14 +102,32 @@ impl<'a> BandRef for BandRefImpl<'a> {
}
fn data(&self) -> &[u8] {
- // Pre-N-D compatibility surface. Identity-view InDb bands → the
- // row-major in-line buffer (zero-copy borrow into the StructArray),
- // matching the pre-N-D behavior exactly. OutDb → `&[]` from the
- // empty `data` column, no panic. Non-identity views never reach
- // here — `RasterRefImpl::band()` rejects them upstream so the
- // raw column bytes always equal the visible bytes for any band
- // this reader produces.
- self.data_array.value(self.band_row)
+ // OutDb: no in-line bytes available. Returns `&[]` to match main's
+ // pre-N-D behavior — callers that care must check `is_indb()` first.
+ if !self.is_indb() {
+ return &[];
+ }
+ // Identity-view InDb: the column bytes ARE the row-major visible
+ // buffer. Borrow directly from Arrow — no allocation, no copy.
+ if self.is_identity_view {
+ return self.data_array.value(self.band_row);
+ }
+ // Non-identity view: walk strides and cache the row-major copy in
+ // `materialized` so repeat `.data()` calls reuse the same buffer.
+ // The walk is infallible by construction — `RasterRef::band()` runs
Review Comment:
You may have to `panic!()` here for now and add another function that
accepts externally managed scratch space.
##########
rust/sedona-raster/src/traits.rs:
##########
@@ -667,6 +673,188 @@ pub trait BandRef {
}
}
+/// Derive the visible (per-axis) shape from a view.
+///
+/// `view[k].steps` is the visible extent along view axis `k` after slicing /
+/// broadcasting. Callers should treat the returned shape as authoritative for
+/// the visible region; `source_shape` is only meaningful in conjunction with
+/// the per-entry `source_axis`.
+///
+/// `validate_view` guarantees `steps >= 0`, so the `as u64` cast is lossless
+/// when the input has already been validated. Callers that haven't validated
+/// yet should still call `validate_view` first.
+pub(crate) fn visible_shape_from_view(view: &[ViewEntry]) -> Vec<u64> {
+ view.iter().map(|v| v.steps as u64).collect()
+}
+
+/// True iff `view` is the canonical identity over a C-order source buffer:
+/// every visible axis `k` is a no-op (no permutation, no slice offset, no
+/// stride/reverse, full coverage of the corresponding source axis).
+///
+/// Identity views let the reader borrow the underlying `data` column
+/// directly through `BandRef::contiguous_data()` and `BandRef::data()`
+/// with no allocation or copy. Non-identity views must materialize a
+/// row-major copy on first byte access.
Review Comment:
```suggestion
/// with no allocation or copy. Non-identity views must materialize a
/// row-major copy on first byte access when using these functions
/// (use `nd_buffer()` to guarantee that a copy is never produced).
```
##########
rust/sedona-raster-gdal/src/gdal_dataset_provider.rs:
##########
@@ -69,6 +69,11 @@ pub(crate) struct RasterDataset<'a> {
_gdal_mem_source: Option<Rc<Dataset>>,
/// External datasets referenced by the VRT; kept alive for the lifetime
of this struct.
_gdal_outdb_sources: Vec<Rc<Dataset>>,
+ /// Reader-allocated band bytes that GDAL pointers in the MEM dataset may
+ /// reference (i.e. bytes returned by `BandRef::contiguous_data()` as
+ /// `Cow::Owned`, moved here without a copy). Kept alive for as long as
+ /// the MEM dataset that holds the pointers.
+ _owned_band_bytes: Vec<Vec<u8>>,
Review Comment:
I am not sure about this one...I think it would be better for now to reject
non-identity views with an error along the lines of `Use RS_ForceContinguous()`
(or something). Alternatively, any scalar function's "invoke" body can be in
charge of managing a temporary buffer that can be used for this purpose (that
may be reused between iterations instead of reallocating).
##########
rust/sedona-raster-gdal/src/gdal_common.rs:
##########
@@ -238,8 +253,39 @@ pub unsafe fn raster_ref_to_gdal_mem<R: RasterRef +
?Sized>(
let band_metadata = band.metadata();
let band_type = band_metadata.data_type()?;
let gdal_type = band_data_type_to_gdal(&band_type);
- let band_data = band.data();
- let data_ptr = band_data.as_ptr();
+ let band_data = band
+ .contiguous_data()
+ .map_err(|e| arrow_datafusion_err!(e))?;
+ // For Cow::Borrowed the pointer is into the StructArray (caller keeps
+ // it alive). For Cow::Owned we move the Vec into `owned_band_bytes`
+ // — no extra copy of the reader's materialization — and point GDAL
+ // at it; the Vec is kept alive alongside the returned Dataset.
+ let data_ptr: *const u8 = match band_data {
+ Cow::Borrowed(b) => b.as_ptr(),
+ Cow::Owned(v) => {
+ // SAFETY (Vec<Vec<u8>> pointer stability):
+ //
+ // GDAL holds a raw pointer into the inner `Vec<u8>`'s heap
Review Comment:
I think you should just reject these. The step where we resolve all of the
InDB and OutDB rasters into contiguous ranges that GDAL can operate on is
special and this is unlikely to be the way we want to end up doing it.
##########
rust/sedona-raster/src/array.rs:
##########
@@ -44,14 +48,26 @@ 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.
+ /// Per-visible-axis view, length = ndim
view_entries: Vec<ViewEntry>,
- /// Visible shape, length = ndim. Equals `source_shape` today.
+ /// Visible shape (== `[v.steps for v in view_entries]`), length = ndim
visible_shape: Vec<u64>,
- /// 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<i64>,
/// 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,
+ /// True iff this view is the identity over a C-order source buffer —
+ /// `contiguous_data()` can then borrow `data` directly. Relies on
+ /// `validate_view` having enforced `view.len() == source_shape.len()`;
+ /// otherwise a "shorter identity" could be wrongly accepted here.
+ is_identity_view: bool,
+ /// Lazy row-major copy of the visible bytes, materialized on the first
+ /// `data()` call against a non-identity view. Identity views borrow
+ /// directly from the Arrow column and never touch this cell.
+ materialized: std::cell::OnceCell<Vec<u8>>,
Review Comment:
I don't think you want this: force the `data()` function to accept a `&mut
Vec` and force all users of `data()` to manage their scratch space (or operate
only on contiguous views). Unless I'm misunderstanding this pattern, this will
make it easy to proliferate allocations that are difficult to track and may be
large.
##########
rust/sedona-raster/src/array.rs:
##########
@@ -134,29 +168,206 @@ impl<'a> BandRef for BandRefImpl<'a> {
}
// shape and strides are owned by NdBuffer (see its doc comment).
// Cloning here is cheap — both vecs are O(ndim), a handful of values.
+ // Cast offset i64 -> u64: safe because RasterRefImpl::band asserts
+ // byte_offset >= 0 before storing.
Review Comment:
Probably getting rid of the u64s will help here
##########
rust/sedona-raster/src/traits.rs:
##########
@@ -667,6 +673,188 @@ pub trait BandRef {
}
}
+/// Derive the visible (per-axis) shape from a view.
+///
+/// `view[k].steps` is the visible extent along view axis `k` after slicing /
+/// broadcasting. Callers should treat the returned shape as authoritative for
+/// the visible region; `source_shape` is only meaningful in conjunction with
+/// the per-entry `source_axis`.
+///
+/// `validate_view` guarantees `steps >= 0`, so the `as u64` cast is lossless
+/// when the input has already been validated. Callers that haven't validated
+/// yet should still call `validate_view` first.
+pub(crate) fn visible_shape_from_view(view: &[ViewEntry]) -> Vec<u64> {
+ view.iter().map(|v| v.steps as u64).collect()
+}
+
+/// True iff `view` is the canonical identity over a C-order source buffer:
+/// every visible axis `k` is a no-op (no permutation, no slice offset, no
+/// stride/reverse, full coverage of the corresponding source axis).
+///
+/// Identity views let the reader borrow the underlying `data` column
+/// directly through `BandRef::contiguous_data()` and `BandRef::data()`
+/// with no allocation or copy. Non-identity views must materialize a
+/// row-major copy on first byte access.
+///
+/// Callers should validate the view first; this function checks lengths
+/// defensively but trusts the inputs otherwise.
+pub(crate) fn is_identity_view(view: &[ViewEntry], source_shape: &[u64]) ->
bool {
+ if view.len() != source_shape.len() {
+ return false;
+ }
+ view.iter().enumerate().all(|(k, v)| {
+ v.source_axis == k as i64
+ && v.start == 0
+ && v.step == 1
+ && v.steps >= 0
+ && (v.steps as u64) == source_shape[k]
+ })
+}
+
+/// Compose `next_view` (a view spec over `input_view`'s visible axes)
+/// with `input_view` (a view spec over a band's `source_shape`), producing
+/// a single view over the same `source_shape` that represents the result
+/// of viewing the input through `next_view`.
+///
+/// `next_view[k].source_axis` indexes into `input_view`'s visible axes
+/// (`0..input_view.len()`), NOT into the underlying source axes. This
+/// lets callers (e.g. lazy slicing) describe the new view in terms of
+/// what they see, without knowing the input's internal layout.
+///
+/// Math: for each output axis `k`, walking through `input_view` resolves
+/// the source axis and translates start/step:
+/// - `out.source_axis = input_view[next.source_axis].source_axis`
+/// - `out.start = input.start + next.start * input.step`
+/// - `out.step = next.step * input.step`
+/// - `out.steps = next.steps`
+///
+/// Uses checked arithmetic at every step. Returns the composed view; the
+/// caller must `validate_view` it against the source_shape before use.
+pub(crate) fn compose_view(
+ input_view: &[ViewEntry],
+ next_view: &[ViewEntry],
+) -> Result<Vec<ViewEntry>, ArrowError> {
+ let mut out = Vec::with_capacity(next_view.len());
+ for (k, next) in next_view.iter().enumerate() {
+ if next.source_axis < 0 || (next.source_axis as usize) >=
input_view.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "compose_view: next_view[{k}].source_axis ({}) is out of range
\
+ for input_view with {} visible axes",
+ next.source_axis,
+ input_view.len()
+ )));
+ }
+ let input = &input_view[next.source_axis as usize];
+ let step = next.step.checked_mul(input.step).ok_or_else(|| {
+ ArrowError::InvalidArgumentError(format!(
+ "compose_view: step product overflows i64 at axis {k} \
+ (next.step={}, input.step={})",
+ next.step, input.step
+ ))
+ })?;
+ let start_offset = next.start.checked_mul(input.step).ok_or_else(|| {
+ ArrowError::InvalidArgumentError(format!(
+ "compose_view: next.start * input.step overflows i64 at axis
{k} \
+ (next.start={}, input.step={})",
+ next.start, input.step
+ ))
+ })?;
+ let start = input.start.checked_add(start_offset).ok_or_else(|| {
+ ArrowError::InvalidArgumentError(format!(
+ "compose_view: composed start overflows i64 at axis {k} \
+ (input.start={}, offset={})",
+ input.start, start_offset
+ ))
+ })?;
+ out.push(ViewEntry {
+ source_axis: input.source_axis,
+ start,
+ step,
+ steps: next.steps,
+ });
+ }
+ Ok(out)
+}
+
+/// Validate a `[ViewEntry]` against a band's `source_shape`.
+///
+/// Returns `Ok(())` if the view is well-formed under the rules:
+/// - `view.len() == source_shape.len()`.
+/// - `source_axis` values across `view` form a permutation of
+/// `0..source_shape.len()` (no axis duplicated, none missing).
+/// - `steps >= 0`.
+/// - When `steps > 0`: `start ∈ [0, source_shape[source_axis])`, and when
+/// `step != 0` the last addressed element
+/// `start + (steps - 1) * step` is also in that range.
+///
+/// Runs implicitly inside `RasterBuilder::with_view` (writer) and
+/// `RasterRef::band` (reader); external callers don't need to invoke it.
+pub(crate) fn validate_view(view: &[ViewEntry], source_shape: &[u64]) ->
Result<(), ArrowError> {
+ let ndim = source_shape.len();
+ if view.len() != ndim {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "view length ({}) must equal source_shape length ({ndim})",
+ view.len()
+ )));
+ }
+ let mut seen = vec![false; ndim];
+ for (k, v) in view.iter().enumerate() {
+ if v.source_axis < 0 || (v.source_axis as usize) >= ndim {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "view[{k}].source_axis = {} is out of range [0, {ndim})",
+ v.source_axis
+ )));
+ }
+ let sa = v.source_axis as usize;
+ if seen[sa] {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "view source_axis values must be a permutation of 0..{ndim}; \
+ axis {sa} appears more than once"
+ )));
+ }
+ seen[sa] = true;
+
+ if v.steps < 0 {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "view[{k}].steps = {} must be >= 0",
+ v.steps
+ )));
+ }
+ if v.steps > 0 {
+ let s = source_shape[sa] as i64;
Review Comment:
Should source_shape be an array of `i64`?
##########
rust/sedona-raster/src/array.rs:
##########
@@ -242,36 +457,127 @@ impl<'a> RasterRef for RasterRefImpl<'a> {
)))
})?;
- // 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.
- if !self.band_view_list.is_null(band_row) {
+ // Read view entries. Identity view is encoded as either a NULL row
+ // or an empty (non-null, zero-length) list — both synthesise the
+ // canonical identity from `source_shape`. A non-empty row is decoded
+ // from the `view` column. Treating empty-non-null and NULL the same
+ // makes the reader tolerant of producers that emit one form versus
+ // the other; the alternative is a misleading "view length 0 must
+ // equal source_shape length N" error from `validate_view`.
+ let view_is_identity_encoded = self.band_view_list.is_null(band_row)
+ || self.band_view_list.value_length(band_row) == 0;
+ let view_entries: Vec<ViewEntry> = if view_is_identity_encoded {
+ source_shape
+ .iter()
+ .enumerate()
+ .map(|(i, &s)| ViewEntry {
+ source_axis: i as i64,
+ start: 0,
+ step: 1,
+ steps: s as i64,
+ })
+ .collect()
+ } else {
+ 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;
+ (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()
+ };
+
+ // Full validation: length match, source_axis permutation, bounds,
+ // and steps >= 0. Anything malformed is schema-level corruption.
+ if let Err(e) = validate_view(&view_entries, source_shape) {
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"
+ "band {band_row} has malformed view: {e}"
),
)));
}
- let view_entries: Vec<ViewEntry> = source_shape
- .iter()
- .enumerate()
- .map(|(i, &s)| ViewEntry {
- source_axis: i as i64,
- start: 0,
- step: 1,
- steps: s as i64,
- })
- .collect();
- let visible_shape: Vec<u64> = source_shape.to_vec();
+ let ndim = view_entries.len();
+ let visible_shape = visible_shape_from_view(&view_entries);
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;
+
+ // Internal helper: arithmetic overflow during stride composition on a
+ // validated band is corruption beyond what validate_view catches, so
+ // route it through the standard internal-error framing.
+ let overflow_err = |msg: &str| {
Review Comment:
Should validate_view catch this?
##########
rust/sedona-raster/src/array.rs:
##########
@@ -134,29 +168,206 @@ impl<'a> BandRef for BandRefImpl<'a> {
}
// shape and strides are owned by NdBuffer (see its doc comment).
// Cloning here is cheap — both vecs are O(ndim), a handful of values.
+ // Cast offset i64 -> u64: safe because RasterRefImpl::band asserts
+ // byte_offset >= 0 before storing.
Ok(NdBuffer {
buffer: self.data_array.value(self.band_row),
shape: self.visible_shape.clone(),
strides: self.byte_strides.clone(),
- offset: self.byte_offset,
+ offset: self.byte_offset as u64,
data_type: self.data_type,
})
}
fn contiguous_data(&self) -> Result<Cow<'_, [u8]>, ArrowError> {
Review Comment:
I am not sure about this one anymore, either. You almost always want
externally managed scratch space because the implementations are almost always
looping over multiple rasters.
##########
rust/sedona-raster/src/array.rs:
##########
@@ -242,36 +457,127 @@ impl<'a> RasterRef for RasterRefImpl<'a> {
)))
})?;
- // 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.
- if !self.band_view_list.is_null(band_row) {
+ // Read view entries. Identity view is encoded as either a NULL row
+ // or an empty (non-null, zero-length) list — both synthesise the
+ // canonical identity from `source_shape`. A non-empty row is decoded
+ // from the `view` column. Treating empty-non-null and NULL the same
+ // makes the reader tolerant of producers that emit one form versus
+ // the other; the alternative is a misleading "view length 0 must
Review Comment:
This is mixing a few levels of abstraction in the `band()` accessor. Can you
split it up into smaller named methods with smaller scope to make it clearer
what `band()` is doing?
##########
rust/sedona-raster/src/array.rs:
##########
@@ -134,29 +168,206 @@ impl<'a> BandRef for BandRefImpl<'a> {
}
// shape and strides are owned by NdBuffer (see its doc comment).
// Cloning here is cheap — both vecs are O(ndim), a handful of values.
+ // Cast offset i64 -> u64: safe because RasterRefImpl::band asserts
+ // byte_offset >= 0 before storing.
Ok(NdBuffer {
buffer: self.data_array.value(self.band_row),
shape: self.visible_shape.clone(),
strides: self.byte_strides.clone(),
- offset: self.byte_offset,
+ offset: self.byte_offset as u64,
data_type: self.data_type,
})
}
fn contiguous_data(&self) -> Result<Cow<'_, [u8]>, ArrowError> {
- if !self.is_indb() {
- return Err(ArrowError::NotYetImplemented(
- "OutDb byte access via contiguous_data() is not yet
implemented; \
- backend-specific OutDb resolvers are tracked separately"
- .to_string(),
- ));
+ let buf = self.nd_buffer()?;
+ if self.is_identity_view {
+ // Identity view over a C-order source buffer: the source bytes
+ // ARE the visible bytes. Borrow them.
+ return Ok(Cow::Borrowed(buf.buffer));
}
- // Identity-view only today, so the data buffer is already row-major
- // over the visible region.
- Ok(Cow::Borrowed(self.data_array.value(self.band_row)))
+ // Use self.* layout fields rather than `buf.*` to avoid the
+ // i64 -> u64 -> i64 round-trip through NdBuffer for `byte_offset`.
+ // The visible shape and byte strides are precomputed once at
+ // construction; `buf.shape` / `buf.strides` are clones of those.
+ let out = materialize_strided(
+ buf.buffer,
+ &self.visible_shape,
+ &self.byte_strides,
+ self.byte_offset,
+ self.data_type.byte_size(),
+ );
+ Ok(Cow::Owned(out))
}
}
+/// 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 strided
+/// walk and the data buffer. `materialize_strided` is plain-arithmetic /
+/// unchecked-slice-indexing and relies 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: &[u64],
+ 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() {
+ let last_idx = i64::try_from(visible_shape[k] - 1).map_err(|_| {
+ ArrowError::InvalidArgumentError(format!("visible_shape[{k}] - 1
exceeds i64::MAX"))
+ })?;
+ 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(())
+}
+
+/// Walk a strided view over `buffer` and return its bytes in canonical
+/// row-major (C-order) layout. Shared between `contiguous_data` (returns
+/// the bytes through `Cow::Owned` + `Result`) and the `data()` shim
+/// (caches the result in `BandRefImpl::materialized`).
+///
+/// Uses checked arithmetic at every step so a pathological stride × index
+/// product can't wrap and silently pass the subsequent bound checks.
+/// Returns an error rather than panicking so the caller decides how to
+/// surface the failure.
+fn materialize_strided(
+ buffer: &[u8],
+ visible_shape: &[u64],
+ byte_strides: &[i64],
+ byte_offset: i64,
+ dtype_size: usize,
+) -> Vec<u8> {
+ // Preconditions (caller-enforced via `check_view_buffer_bounds` at
+ // `RasterRef::band()` construction):
+ // 1. Every byte offset addressed by the walk
+ // (`base + Σ outer_idx[k] * stride[k] + i * inner_stride +
dtype_size - 1`)
+ // lies within `[0, buffer.len())`.
+ // 2. All intermediate stride × index products and offset accumulations
+ // fit in i64.
+ //
+ // Both are verified by the precheck using the same checked arithmetic
+ // we'd otherwise repeat here, so this walk uses plain arithmetic and
+ // unchecked slice indexing within the buffer.
+ let ndim = visible_shape.len();
+ let total: u64 = visible_shape.iter().product();
+ if total == 0 {
+ return Vec::new();
+ }
+ let total = total as usize;
+ let mut out = Vec::with_capacity(total * dtype_size);
+ let base = byte_offset;
+
+ // Innermost-axis fast path. We always step the innermost (last)
+ // visible axis as the inner loop; everything outer drives the
+ // starting byte for that row.
+ let inner = ndim - 1;
+ let inner_steps = visible_shape[inner] as usize;
+ let inner_stride = byte_strides[inner];
+ // `dtype_size` is always >= 1 (every BandDataType has a positive
+ // byte size), so it doesn't need its own guard.
+ let row_bytes_contiguous = inner_stride == dtype_size as i64 &&
inner_steps > 0;
+
+ // Precompute a small index vector for outer axes (everything except
+ // the innermost). For 1D this is empty and we run a single pass.
+ let mut outer_idx = vec![0u64; ndim.saturating_sub(1)];
+ loop {
Review Comment:
This is potentially a bottleneck and would benefit from a dedicated
benchmark. Once you have this, you can check to see if skipping the zero
improves performance (you can use some of the unsafe slice/vec functions to
resize without zeroing, and to do raw copying of memory regions). I am guessing
that the looped calculations involving `start..start + len` are slow but
compilers are very smart these days and we need a benchmark to be sure.
##########
rust/sedona-raster/src/array.rs:
##########
@@ -134,29 +168,206 @@ impl<'a> BandRef for BandRefImpl<'a> {
}
// shape and strides are owned by NdBuffer (see its doc comment).
// Cloning here is cheap — both vecs are O(ndim), a handful of values.
+ // Cast offset i64 -> u64: safe because RasterRefImpl::band asserts
+ // byte_offset >= 0 before storing.
Ok(NdBuffer {
buffer: self.data_array.value(self.band_row),
shape: self.visible_shape.clone(),
strides: self.byte_strides.clone(),
- offset: self.byte_offset,
+ offset: self.byte_offset as u64,
data_type: self.data_type,
})
}
fn contiguous_data(&self) -> Result<Cow<'_, [u8]>, ArrowError> {
- if !self.is_indb() {
- return Err(ArrowError::NotYetImplemented(
- "OutDb byte access via contiguous_data() is not yet
implemented; \
- backend-specific OutDb resolvers are tracked separately"
- .to_string(),
- ));
+ let buf = self.nd_buffer()?;
+ if self.is_identity_view {
+ // Identity view over a C-order source buffer: the source bytes
+ // ARE the visible bytes. Borrow them.
+ return Ok(Cow::Borrowed(buf.buffer));
}
- // Identity-view only today, so the data buffer is already row-major
- // over the visible region.
- Ok(Cow::Borrowed(self.data_array.value(self.band_row)))
+ // Use self.* layout fields rather than `buf.*` to avoid the
+ // i64 -> u64 -> i64 round-trip through NdBuffer for `byte_offset`.
+ // The visible shape and byte strides are precomputed once at
+ // construction; `buf.shape` / `buf.strides` are clones of those.
+ let out = materialize_strided(
+ buf.buffer,
+ &self.visible_shape,
+ &self.byte_strides,
+ self.byte_offset,
+ self.data_type.byte_size(),
+ );
+ Ok(Cow::Owned(out))
}
}
+/// 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 strided
+/// walk and the data buffer. `materialize_strided` is plain-arithmetic /
+/// unchecked-slice-indexing and relies 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: &[u64],
+ 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() {
+ let last_idx = i64::try_from(visible_shape[k] - 1).map_err(|_| {
+ ArrowError::InvalidArgumentError(format!("visible_shape[{k}] - 1
exceeds i64::MAX"))
+ })?;
+ 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(())
+}
+
+/// Walk a strided view over `buffer` and return its bytes in canonical
+/// row-major (C-order) layout. Shared between `contiguous_data` (returns
+/// the bytes through `Cow::Owned` + `Result`) and the `data()` shim
+/// (caches the result in `BandRefImpl::materialized`).
+///
+/// Uses checked arithmetic at every step so a pathological stride × index
+/// product can't wrap and silently pass the subsequent bound checks.
+/// Returns an error rather than panicking so the caller decides how to
+/// surface the failure.
+fn materialize_strided(
+ buffer: &[u8],
+ visible_shape: &[u64],
+ byte_strides: &[i64],
+ byte_offset: i64,
+ dtype_size: usize,
+) -> Vec<u8> {
Review Comment:
The easiest way to let the caller manage the allocation would be to accept a
`&mut Vec<u8>` (which may or may not be empty, having possibly used it to
materialize a previous band) and have it return a `&[u8]` (the slice of the
provided scratch that was written to).
Another interface worth exposing is a visitor-based one (i.e.,
`visit_strided(..., |element_bytes| { ... })`. Or possibly a templated version
where you `visit_strided<32>(..., |element_f32| { ... })`. You can write the
version that appends to a buffer in terms of the visitor interface (the
compiler should inline the lambda).
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -374,6 +377,184 @@ 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`.
+ #[allow(clippy::too_many_arguments)]
+ pub(crate) fn start_band_with_view(
Review Comment:
Clippy has a good point here. Define `struct StartBandWithViewArgs<'a> { ...
}` with all `pub` fields, which will force callers to write
`start_band_with_view(StartBandWithViewArgs { <named params> })`.
You could also expose more than one function or have a BandBuilder of some
kind to separate these things into more than one call.
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -1717,222 +1902,1410 @@ mod tests {
}
#[test]
- fn test_start_band_rejects_zero_dim() {
- // 0-D bands carry no spatial extent and no caller has a use for
- // them. start_band_nd must reject an empty dim_names slice eagerly so
- // the malformed band never reaches the buffer layer.
- 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_nd(None, &[], &[], BandDataType::UInt8, None, None,
None)
- .unwrap_err();
- assert!(
- err.to_string().contains("0-dimensional"),
- "unexpected error: {err}"
- );
- }
-
- #[test]
- fn test_contiguous_data_identity_via_start_band_is_borrowed() {
- // 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.
+ 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"], &[3, 2], None)
+ .start_raster_nd(&transform, &["x", "y"], &[5, 4], None)
.unwrap();
+
+ let view = [
+ ViewEntry {
+ source_axis: 0,
Review Comment:
A helper struct to manage `Vec<ViewEntry>` would make it more likely that
other people will write reasonable tests. Or a macro (`view_entries![0:4,
1:5]`; use Python slice syntax since that's what you'd type in numpy to do
this).
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -1717,222 +1902,1410 @@ mod tests {
}
#[test]
- fn test_start_band_rejects_zero_dim() {
- // 0-D bands carry no spatial extent and no caller has a use for
- // them. start_band_nd must reject an empty dim_names slice eagerly so
- // the malformed band never reaches the buffer layer.
- 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_nd(None, &[], &[], BandDataType::UInt8, None, None,
None)
- .unwrap_err();
- assert!(
- err.to_string().contains("0-dimensional"),
- "unexpected error: {err}"
- );
- }
-
- #[test]
- fn test_contiguous_data_identity_via_start_band_is_borrowed() {
- // 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.
+ 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"], &[3, 2], None)
+ .start_raster_nd(&transform, &["x", "y"], &[5, 4], None)
.unwrap();
+
+ let view = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 4,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 5,
+ },
+ ];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
&["y", "x"],
- &[2, 3],
+ &[4, 5],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- let pixels: Vec<u8> = (0..6).collect();
- builder.band_data_writer().append_value(pixels.clone());
+ 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();
-
- // Visible shape comes from the synthesised identity view.
- assert_eq!(band.shape(), &[2, 3]);
- assert_eq!(band.raw_source_shape(), &[2, 3]);
-
+ assert_eq!(band.shape(), &[4, 5]);
+ assert_eq!(band.raw_source_shape(), &[4, 5]);
let buf = band.nd_buffer().unwrap();
- assert_eq!(buf.strides, &[3, 1]);
+ assert_eq!(buf.strides, &[5, 1]);
assert_eq!(buf.offset, 0);
-
- let bytes = band.contiguous_data().unwrap();
- assert!(matches!(bytes, Cow::Borrowed(_)));
- assert_eq!(&*bytes, pixels.as_slice());
}
#[test]
- fn test_view_field_is_null_for_identity_band() {
- // Schema invariant: identity views are stored as null list rows so
- // the canonical "no slice" case costs no Arrow space. Confirm by
- // poking the raw column.
- use arrow_array::Array;
-
+ fn test_view_slice_nd_buffer_and_contiguous_data() {
+ // 1D source of size 8 (UInt8), view (start=1, step=2, steps=3) selects
+ // elements at byte offsets 1, 3, 5. Source: 0..8.
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"], &[2, 2], None)
+ .start_raster_nd(&transform, &["x"], &[3], None)
.unwrap();
+
+ let view = [ViewEntry {
+ source_axis: 0,
+ start: 1,
+ step: 2,
+ steps: 3,
+ }];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
- &[2, 2],
+ &["x"],
+ &[8],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 4]);
+ 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 bands_list = array
- .column(sedona_schema::raster::raster_indices::BANDS)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- let bands_struct = bands_list
- .values()
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
- let view_list = bands_struct
- .column(sedona_schema::raster::band_indices::VIEW)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- assert_eq!(view_list.len(), 1);
- assert!(
- view_list.is_null(0),
- "identity-view band should serialise as a null view row"
- );
+ let rasters = RasterStructArray::new(&array);
+ let r = rasters.get(0).unwrap();
+ let band = r.band(0).unwrap();
+
+ assert_eq!(band.shape(), &[3]);
+ assert_eq!(band.raw_source_shape(), &[8]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.shape, &[3]);
+ assert_eq!(buf.strides, &[2]);
+ assert_eq!(buf.offset, 1);
+
+ // Materialised contiguous bytes should be [1, 3, 5].
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[1u8, 3, 5]);
+ assert!(matches!(bytes, std::borrow::Cow::Owned(_)));
}
#[test]
- fn test_band_spatial_dim_size_mismatch_errors() {
+ fn test_view_broadcast() {
+ // Broadcast: source size 1, step=0 → expose the same byte 4 times.
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"], &[4, 4], None)
+ .start_raster_nd(&transform, &["x"], &[4], None)
.unwrap();
- // Band has "x" and "y" but x-size disagrees with top-level shape.
+
+ let view = [ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 0,
+ steps: 4,
+ }];
builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
- &[4, 8],
+ &["x"],
+ &[1],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 32]);
+ builder.band_data_writer().append_value(vec![42u8]);
builder.finish_band().unwrap();
+ builder.finish_raster().unwrap();
- let err = builder.finish_raster().unwrap_err();
- let msg = err.to_string();
- assert!(
- msg.contains("has size 8") && msg.contains("expected 4"),
- "unexpected error: {msg}"
- );
+ 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]);
+ assert_eq!(buf.strides, &[0]);
+ assert_eq!(buf.offset, 0);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[42u8, 42, 42, 42]);
}
#[test]
- fn test_view_null_round_trips_through_arrow_ipc() {
- // Schema invariant: a band built via start_band_nd serialises with a
- // null view row, and the null must survive an Arrow IPC round-trip.
- // If a future change accidentally writes a non-null empty list
- // instead, downstream readers (DuckDB, PyArrow, sedona-py) will
- // disagree about whether the view is identity.
-
+ fn test_view_permutation_transpose() {
+ // 2×3 source (UInt8), values 0..6 in C-order:
+ // row 0: [0, 1, 2]
+ // row 1: [3, 4, 5]
+ // Transposed view exposes axes (cols, rows) → 3×2:
+ // row 0: [0, 3]
+ // row 1: [1, 4]
+ // row 2: [2, 5]
let mut builder = RasterBuilder::new(1);
let transform = [0.0, 1.0, 0.0, 0.0, 0.0, -1.0];
+ // The transposed visible shape on the spatial axes would conflict with
+ // a 2D spatial grid; declare a single non-spatial dim "i" so the
+ // strict spatial check is trivially satisfied.
+ builder.start_raster_nd(&transform, &[], &[], None).unwrap();
+
+ let view = [
+ // visible axis 0 reads source axis 1 (cols), full extent 3
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ // visible axis 1 reads source axis 0 (rows), full extent 2
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ];
builder
- .start_raster_nd(&transform, &["x", "y"], &[3, 2], None)
- .unwrap();
- builder
- .start_band_nd(
+ .start_band_with_view(
None,
- &["y", "x"],
+ &["a", "b"],
&[2, 3],
+ &view,
BandDataType::UInt8,
None,
None,
None,
)
.unwrap();
- builder.band_data_writer().append_value(vec![0u8; 6]);
+ builder
+ .band_data_writer()
+ .append_value(vec![0u8, 1, 2, 3, 4, 5]);
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(
- "raster",
- array.data_type().clone(),
- true,
- )) as arrow_schema::FieldRef]));
- let batch = RecordBatch::try_new(schema.clone(),
vec![Arc::new(array.clone())]).unwrap();
+ let rasters = RasterStructArray::new(&array);
+ let r = rasters.get(0).unwrap();
+ let band = r.band(0).unwrap();
- let mut buf: Vec<u8> = Vec::new();
- {
- let mut writer = StreamWriter::try_new(&mut buf,
schema.as_ref()).unwrap();
- writer.write(&batch).unwrap();
- writer.finish().unwrap();
- }
+ assert_eq!(band.shape(), &[3, 2]);
+ assert_eq!(band.raw_source_shape(), &[2, 3]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.strides, &[1, 3]); // visible axis 0 → source col
stride; visible axis 1 → source row stride
- let cursor = Cursor::new(buf);
- let reader = StreamReader::try_new(cursor, None).unwrap();
- let batches: Vec<_> = reader.collect::<Result<Vec<_>, _>>().unwrap();
- assert_eq!(batches.len(), 1);
- let restored_struct = batches[0]
- .column(0)
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[0u8, 3, 1, 4, 2, 5]);
+ }
- let bands_list = restored_struct
- .column(sedona_schema::raster::raster_indices::BANDS)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- let bands_struct = bands_list
- .values()
- .as_any()
- .downcast_ref::<StructArray>()
- .unwrap();
- let view_list = bands_struct
- .column(sedona_schema::raster::band_indices::VIEW)
- .as_any()
- .downcast_ref::<ListArray>()
- .unwrap();
- assert_eq!(view_list.len(), 1);
- assert!(
- view_list.is_null(0),
- "identity-view band must remain a null view row after IPC
round-trip"
- );
+ #[test]
+ fn test_view_empty_axis() {
+ // steps=0 → empty visible axis. contiguous_data must succeed and
+ // return an empty buffer.
+ 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: 1,
+ steps: 0,
+ }];
+ builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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();
+ assert_eq!(band.shape(), &[0]);
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.shape, &[0]);
+ let bytes = band.contiguous_data().unwrap();
+ assert!(bytes.is_empty());
+ }
+
+ #[test]
+ fn test_start_band_rejects_zero_dim() {
+ // 0-D bands carry no spatial extent and no caller has a use for
+ // them. start_band_nd must reject an empty dim_names slice eagerly so
+ // the malformed band never reaches the buffer layer.
+ 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_nd(None, &[], &[], BandDataType::UInt8, None, None,
None)
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("0-dimensional"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[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(None, &[], &[], &[], BandDataType::UInt8,
None, None, None)
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("0-dimensional"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_view_validation_rejects_out_of_range_start() {
+ 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: 8,
+ step: 1,
+ steps: 1,
+ }];
+ let err = builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("out of range"),
+ "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(
+ None,
+ &["x"],
+ &[7],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("out of range"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_view_validation_rejects_duplicate_source_axis() {
+ 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: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ];
+ let err = builder
+ .start_band_with_view(
+ None,
+ &["a", "b"],
+ &[2, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("permutation"),
+ "unexpected error: {err}"
+ );
+ }
+
+ #[test]
+ fn test_contiguous_data_identity_via_start_band_is_borrowed() {
+ // 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<u8> = (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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert!(matches!(bytes, Cow::Borrowed(_)));
+ assert_eq!(&*bytes, pixels.as_slice());
+ }
+
+ #[test]
+ fn test_contiguous_data_explicit_identity_view_is_borrowed() {
+ // 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 Cow::Borrowed fast path.
+ use std::borrow::Cow;
+
+ 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 = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["y", "x"],
+ &[2, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ let pixels: Vec<u8> = (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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert!(matches!(bytes, Cow::Borrowed(_)));
+ assert_eq!(&*bytes, pixels.as_slice());
+ }
+
+ #[test]
+ fn test_contiguous_data_zero_step_broadcast_2d() {
+ // 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(
+ None,
+ &["row", "col"],
+ &[1, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[10u8, 20, 30, 10, 20, 30, 10, 20, 30, 10, 20,
30]);
+ }
+
+ #[test]
+ fn test_contiguous_data_negative_step_full_reverse() {
+ // 1D source [0..8] with start=7, step=-1, steps=8 walks the source
+ // backwards. Byte stride must be negative; offset lands on the last
+ // source element.
+ 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: 7,
+ step: -1,
+ steps: 8,
+ }];
+ builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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, &[8]);
+ assert_eq!(buf.strides, &[-1]);
+ assert_eq!(buf.offset, 7);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[7u8, 6, 5, 4, 3, 2, 1, 0]);
+ }
+
+ #[test]
+ fn test_contiguous_data_negative_step_strided_reverse() {
+ // 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(
+ None,
+ &["x"],
+ &[8],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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);
+
+ let bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &[6u8, 4, 2]);
+ }
+
+ #[test]
+ fn test_view_field_is_null_for_identity_band() {
+ // Schema invariant: identity views are stored as null list rows so
+ // the canonical "no slice" case costs no Arrow space. Confirm by
+ // poking the raw column.
+ use arrow_array::Array;
+
+ 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"], &[2, 2], None)
+ .unwrap();
+ builder
+ .start_band_nd(
+ None,
+ &["y", "x"],
+ &[2, 2],
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ builder.band_data_writer().append_value(vec![0u8; 4]);
+ builder.finish_band().unwrap();
+ builder.finish_raster().unwrap();
+
+ let array = builder.finish().unwrap();
+ let bands_list = array
+ .column(sedona_schema::raster::raster_indices::BANDS)
+ .as_any()
+ .downcast_ref::<ListArray>()
+ .unwrap();
+ let bands_struct = bands_list
+ .values()
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .unwrap();
+ let view_list = bands_struct
+ .column(sedona_schema::raster::band_indices::VIEW)
+ .as_any()
+ .downcast_ref::<ListArray>()
+ .unwrap();
+ assert_eq!(view_list.len(), 1);
+ assert!(
+ view_list.is_null(0),
+ "identity-view band should serialise as a null view row"
+ );
+ }
+
+ #[test]
+ fn test_band_spatial_dim_size_mismatch_errors() {
+ 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"], &[4, 4], None)
+ .unwrap();
+ // Band has "x" and "y" but x-size disagrees with top-level shape.
+ builder
+ .start_band_nd(
+ None,
+ &["y", "x"],
+ &[4, 8],
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ builder.band_data_writer().append_value(vec![0u8; 32]);
+ builder.finish_band().unwrap();
+
+ let err = builder.finish_raster().unwrap_err();
+ let msg = err.to_string();
+ assert!(
+ msg.contains("has size 8") && msg.contains("expected 4"),
+ "unexpected error: {msg}"
+ );
+ }
+
+ #[test]
+ fn test_contiguous_data_float32_fast_path() {
+ // Multi-byte dtype on the contiguous innermost-axis fast path:
+ // a 2D explicit-identity view over Float32 should still emit
+ // bytes by `extend_from_slice` and produce the exact source
+ // payload back. Catches a regression where the fast path
+ // 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, so contiguous_data() materialises through the
+ // fast path. Inner stride = dtype_size = 4 → fast path is taken.
+ let view = [
+ ViewEntry {
+ source_axis: 0,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["y", "x"],
+ &[3, 3], // 3x3 source
+ &view,
+ BandDataType::Float32,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ let source: Vec<f32> = (0..9).map(|i| i as f32).collect();
+ let source_bytes: Vec<u8> = 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 bytes = band.contiguous_data().unwrap();
+ assert!(matches!(bytes, std::borrow::Cow::Owned(_)));
+ assert_eq!(&*bytes, &source_bytes[0..24]);
+ }
+
+ #[test]
+ fn test_contiguous_data_outer_axis_slice_3d() {
+ // 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. Innermost
+ // axis is contiguous (step=1, dtype=1) so the fast path emits 6
+ // bytes per outer iteration via extend_from_slice.
+ 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 = [
+ ViewEntry {
+ source_axis: 0,
+ start: 1,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 0,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 2,
+ start: 0,
+ step: 1,
+ steps: 3,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["t", "y", "x"],
+ &[3, 2, 3],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ let source: Vec<u8> = (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 bytes = band.contiguous_data().unwrap();
+ assert_eq!(&*bytes, &source[6..18]);
+ }
+
+ #[test]
+ fn test_contiguous_data_strided_inner_falls_back() {
+ // Inner stride != dtype_size forces the elementwise fallback. View
+ // takes every other column on a 1D UInt16 source. Verifies the
+ // slow path still emits correct bytes.
+ 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: 2,
+ steps: 3,
+ }];
+ builder
+ .start_band_with_view(
+ None,
+ &["x"],
+ &[6],
+ &view,
+ BandDataType::UInt16,
+ None,
+ None,
+ None,
+ )
+ .unwrap();
+ let source: Vec<u16> = vec![10, 20, 30, 40, 50, 60];
+ let source_bytes: Vec<u8> = source.iter().flat_map(|v|
v.to_le_bytes()).collect();
+ builder.band_data_writer().append_value(source_bytes);
+ 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 bytes = band.contiguous_data().unwrap();
+ let expected: Vec<u8> = [10u16, 30, 50]
+ .iter()
+ .flat_map(|v| v.to_le_bytes())
+ .collect();
+ assert_eq!(&*bytes, expected.as_slice());
+ }
+
+ #[test]
+ fn test_nd_buffer_multidim_non_zero_starts() {
+ // 3D source [T=4, Y=3, X=5], slice T from 1, Y from 1, X identity.
+ // visible = [3, 2, 5]. byte_offset must equal 1*Y*X + 1*X = 1*15 +
1*5 = 20.
+ 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, 2], None)
+ .unwrap();
+ let view = [
+ ViewEntry {
+ source_axis: 0,
+ start: 1,
+ step: 1,
+ steps: 3,
+ },
+ ViewEntry {
+ source_axis: 1,
+ start: 1,
+ step: 1,
+ steps: 2,
+ },
+ ViewEntry {
+ source_axis: 2,
+ start: 0,
+ step: 1,
+ steps: 5,
+ },
+ ];
+ builder
+ .start_band_with_view(
+ None,
+ &["t", "y", "x"],
+ &[4, 3, 5],
+ &view,
+ BandDataType::UInt8,
+ None,
+ None,
+ 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();
+ let buf = band.nd_buffer().unwrap();
+ assert_eq!(buf.shape, &[3, 2, 5]);
+ assert_eq!(buf.strides, &[15, 5, 1]);
+ assert_eq!(buf.offset, 20);
+ }
+
+ #[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
Review Comment:
These kinds of tests you probably don't want in this file. This file is
about serializing stuff into Arrow and it's tricky to see what's going on...I'm
not sure what the perfect abstraction is here but it should probably live in
`src/views.rs` with tests and some pub functions that can be benchmarked.
Proabably `struct ViewEntries { inner: Vec<ViewEntry> }` with associated
methods would make this a bit cleaner than the `&[ViewEntry]` + free functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
