You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "james-willis (via GitHub)" <gi...@apache.org> on 2026/05/05 00:21:08 UTC
[PR] feat(raster): view machinery for non-identity band views [sedona-db]
james-willis opened a new pull request, #813:
URL: https://github.com/apache/sedona-db/pull/813
## Summary
Reintroduces view machinery on top of #749 so callers can construct and read bands with non-identity `view` entries. #749 itself only writes the canonical identity (null view row) and rejects non-null views at read time; this PR re-enables the composition path that the slice/manipulation functions in #750 depend on.
## Stack
- #812 → #749 → **this PR** → #750
- **Visual base for review**: branch `jw/nd-raster-type` (= #749 tip). GitHub forces the base to `apache/main`, so this PR's diff against main includes #749's commits. To see just this PR's delta, compare against `jw/nd-raster-type`.
## What's in this PR
Single commit, three files:
- `rust/sedona-raster/src/traits.rs`
- `validate_view()` + 22 unit tests (length checks, axis range, negative step, broadcast / step=0, i64 overflow guards, etc.)
- `rust/sedona-raster/src/builder.rs`
- `start_band_with_view()` builder API
- ~12 view-construction tests: slice, broadcast, transpose, negative-step, multi-band mixed views, Arrow IPC round-trip
- `rust/sedona-raster/src/array.rs`
- View → byte-stride composition in `nd_buffer()`
- View-aware `contiguous_data()` `Cow::Owned` strided-copy slow path
- Reader tests for explicit views (negative steps, OOB axis, length mismatch, malformed view rejection)
## Behavior change vs #749
- A band with a non-null `view` row now decodes via the composition path instead of being rejected as a read error. Identity-view (null view row) writer/reader behavior is unchanged.
## Test plan
- [x] `cargo build -p sedona-schema -p sedona-raster -p sedona-raster-gdal -p sedona-raster-functions -p sedona-testing`
- [x] `cargo test -p sedona-raster` — 91 tests pass (includes the 22 `validate_view` tests, 12 builder view tests, 4 array view tests)
- [x] `cargo clippy -p sedona-raster --all-targets -- -D warnings`
- [x] `cargo fmt --all --check`
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #813:
URL: https://github.com/apache/sedona-db/pull/813#issuecomment-4467630393
Somewhat offtopic but are you going to have the same concern with lazy outdb loading as well? (Outdb lazy loading + zarr loader is going to be higher on my priority list next week than this PR)
> The main issue I have with the implementation here is that it relies on hidden Vec<u8>s that aren't reusable. To manage memory properly we are going to have to register some scratch spaces with the session's memory pool and reuse them between iterations of a loop. Hiding them within various structures for convenience is going to make it tricky to implement a correct pattern going forward.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337883796
##########
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:
we do force them to be the same length. The source_axis field is essential to support permutation.
The ordering tells us how to permute the view. the source_axis tells us where to get the data from.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3338841327
##########
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:
validate_view does not know the dtype_byte_size so it cannot the dtype-scaled product of source strides. Only compose_byte_strides computes that so we put this check here.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337875739
##########
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:
added a guard before the cast.
source_axis is a i64 to consolidate all these fields to that one type.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #813:
URL: https://github.com/apache/sedona-db/pull/813#issuecomment-4468735315
Possibly? I know it seems like a lot of comments but what I'm getting at is:
```rust
let mut scratch = Vec::new();
for raster in rasters {
let data_slice = raster.band(0)?.contiguous_data(&mut scratch)?;
do_stuff(data_slice);
}
```
Or on the day we need to track memory:
```rust
let mut max_capacity = 0;
for raster in rasters {
max_capacity = max_capacity.max(raster.band(0)?.contiguous_data_alloc_size());
}
if max_capacity > config_options.raster.max_scratch_alloc {
// error
}
// If this ends up being a lot, frequently, we may also want to use a MemoryReservation to track it
// Probably this helps contiguous_data be faster since in theory there is only one heap allocation.
// You could probably use some of the unsafe modifiers to also ensure that there are no bounds
// checks in your materializer slowing things down
let mut scratch = Vec::with_capacity(max_capacity);
for raster in rasters {
let data_slice = raster.band(0)?.contiguous_data(&mut scratch)?;
do_stuff(data_slice);
}
```
I'm not sure exactly how loading works yet but you might want something similar (with the hiccup that you need some number of async workers doing IO so you also need the same number of scratch buffers).
(I have the same comment for pretty much everything that heap allocates in a loop, which is why, for example, our geometry writers write WKB directly into the Arrow output instead of returning `Vec<u8>`).
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #813:
URL: https://github.com/apache/sedona-db/pull/813#issuecomment-5172756878
subsumed by #1113
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3305829536
##########
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:
done
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3305876531
##########
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:
I cleaned out some of the u64s. These are the ones I left in the raster ecosystem:
1. BandRef::width() -> u64 / height() -> u64 (and the RasterMetadata shim equivalents) — 2D-compat accessors. Changing these is API-breaking across sedona-functions, sedona-raster-gdal, callers in tests, etc.
2. BandRef::raw_source_shape() -> &[u64] and dim_size() -> Option<u64> — same reason; they sit on the public trait.
3. source_shape, spatial_shape Arrow columns type → could be done as a follow up.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337870438
##########
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:
moot with the remove of these transparent copies
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis closed pull request #813: [WIP] feat(raster): view machinery for non-identity band views
URL: https://github.com/apache/sedona-db/pull/813
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337527107
##########
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:
maybe we use the same strategy from the OutDB loader PR and have some RS_EnsureContiguous function.
##########
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:
yeah this EnsureContiguous direction will be the way we go. I stripped out data & contiguous_data from the API to match with this change.
for now these will just error
##########
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'm going to tweak the whole PR to remove anything that needs a net-new buffer or scratch vector to do its work. instead we will do something similar to what we did with outdb: `RS_EnsureContiguous` will create a new raster with contiguous bands.
##########
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:
the EnsureContiguous is the approach I decided on. the NdBuffer will give a sedona internal error when as_contiguous is called on a non-contiguous buffer.
##########
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:
removed.
##########
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:
I just delete this method instead - deferred to when we do the implementation of RS_EnsureContiguous.
##########
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:
delete this method - comment is moot for now
##########
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:
I reduce the tests here. they should now be just builder-integration specific. will confirm before resolving this comment
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337884655
##########
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:
done
##########
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:
done
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3338803987
##########
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:
I am generally unhappy with the band builder interfaces; I agree I am making them substantially worse. Not sure how you feel about deferring this but I started putting items for band builder improvements in this ticket:
https://github.com/apache/sedona-db/issues/896
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3337885503
##########
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:
done
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3305853031
##########
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:
only if we want to change the arrow type. for now I dont want to touch that.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] feat(raster): view machinery for non-identity band views [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #813:
URL: https://github.com/apache/sedona-db/pull/813#discussion_r3305893678
##########
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:
addressed in another comment
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org