paleolimbot commented on code in PR #934:
URL: https://github.com/apache/sedona-db/pull/934#discussion_r3383888132
##########
rust/sedona-raster/src/traits.rs:
##########
@@ -118,26 +118,11 @@ impl<'a> NdBuffer<'a> {
}
}
-/// One per-dimension entry of a band's logical view. Describes how a
-/// visible axis maps onto an axis of the underlying source buffer.
-///
-/// - `source_axis`: index into the band's `source_shape` that this visible
-/// axis reads from. Across a band's full view, `source_axis` values must
-/// form a permutation of `0..ndim` — axis-dropping and axis-introducing
-/// views are not supported today.
-/// - `start`: starting index along the source axis (in elements, not bytes).
-/// - `step`: stride between consecutive visible elements along the source
-/// axis. `step == 0` means broadcast (the same source element is
-/// exposed `steps` times); negative `step` means reverse iteration.
-/// - `steps`: number of visible elements along this axis. `steps == 0` is
-/// allowed (empty axis).
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct ViewEntry {
- pub source_axis: i64,
- pub start: i64,
- pub step: i64,
- pub steps: i64,
-}
+// `ViewEntry` (and the `ViewEntries` newtype with its validation /
+// composition / visible-shape machinery) lives in `view_entries.rs`.
+// Re-exported here so the `crate::traits::ViewEntry` import path keeps
+// working.
+pub use crate::view_entries::ViewEntry;
Review Comment:
llms love to leave these pub uses around but I'd rather remove them (these
are all internal crates)
##########
rust/sedona-raster/src/view_entries.rs:
##########
@@ -0,0 +1,594 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! View entries — per-axis slice / broadcast / permutation specs for an
+//! N-D raster band. The [`ViewEntries`] newtype owns a `Vec<ViewEntry>`
+//! and is the entry point for all view-machinery operations
+//! (validation, identity check, visible-shape derivation, composition).
+
+use arrow_schema::ArrowError;
+
+/// One per-dimension entry of a band's logical view. Describes how a
+/// visible axis maps onto an axis of the underlying source buffer.
+///
+/// - `source_axis`: index into the band's `source_shape` that this visible
+/// axis reads from. Across a band's full view, `source_axis` values must
+/// form a permutation of `0..ndim` — axis-dropping and axis-introducing
+/// views are not supported today.
+/// - `start`: starting index along the source axis (in elements, not bytes).
+/// - `step`: stride between consecutive visible elements along the source
+/// axis. `step == 0` means broadcast (the same source element is
+/// exposed `steps` times); negative `step` means reverse iteration.
+/// - `steps`: number of visible elements along this axis. `steps == 0` is
+/// allowed (empty axis).
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct ViewEntry {
+ pub source_axis: i64,
+ pub start: i64,
+ pub step: i64,
+ pub steps: i64,
+}
+
+/// A band's full view, one [`ViewEntry`] per visible axis.
+///
+/// Use [`ViewEntries::new`] to wrap an existing `Vec<ViewEntry>` or
+/// [`ViewEntries::identity_for_shape`] to build the canonical
+/// no-op view over a given source shape. Operations on the view live
+/// as methods on this type — `validate`, `visible_shape`,
+/// `is_identity`, `compose`. The newtype gives Vec<ViewEntry> a name
+/// and a single place to attach helpers and tests.
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
+pub struct ViewEntries(Vec<ViewEntry>);
+
+impl ViewEntries {
+ /// Wrap a pre-built vector of entries. The view is not validated
+ /// here — call [`Self::validate`] before relying on it.
+ pub fn new(inner: Vec<ViewEntry>) -> Self {
+ Self(inner)
+ }
Review Comment:
Should there be a `try_new()` that does both?
##########
rust/sedona-raster/src/view_entries.rs:
##########
@@ -0,0 +1,594 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! View entries — per-axis slice / broadcast / permutation specs for an
+//! N-D raster band. The [`ViewEntries`] newtype owns a `Vec<ViewEntry>`
+//! and is the entry point for all view-machinery operations
+//! (validation, identity check, visible-shape derivation, composition).
+
+use arrow_schema::ArrowError;
+
+/// One per-dimension entry of a band's logical view. Describes how a
+/// visible axis maps onto an axis of the underlying source buffer.
+///
+/// - `source_axis`: index into the band's `source_shape` that this visible
+/// axis reads from. Across a band's full view, `source_axis` values must
+/// form a permutation of `0..ndim` — axis-dropping and axis-introducing
+/// views are not supported today.
+/// - `start`: starting index along the source axis (in elements, not bytes).
+/// - `step`: stride between consecutive visible elements along the source
+/// axis. `step == 0` means broadcast (the same source element is
+/// exposed `steps` times); negative `step` means reverse iteration.
+/// - `steps`: number of visible elements along this axis. `steps == 0` is
+/// allowed (empty axis).
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct ViewEntry {
+ pub source_axis: i64,
+ pub start: i64,
+ pub step: i64,
+ pub steps: i64,
+}
+
+/// A band's full view, one [`ViewEntry`] per visible axis.
+///
+/// Use [`ViewEntries::new`] to wrap an existing `Vec<ViewEntry>` or
+/// [`ViewEntries::identity_for_shape`] to build the canonical
+/// no-op view over a given source shape. Operations on the view live
+/// as methods on this type — `validate`, `visible_shape`,
+/// `is_identity`, `compose`. The newtype gives Vec<ViewEntry> a name
+/// and a single place to attach helpers and tests.
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
+pub struct ViewEntries(Vec<ViewEntry>);
+
+impl ViewEntries {
+ /// Wrap a pre-built vector of entries. The view is not validated
+ /// here — call [`Self::validate`] before relying on it.
+ pub fn new(inner: Vec<ViewEntry>) -> Self {
+ Self(inner)
+ }
+
+ /// Build the canonical identity view over `source_shape`:
+ /// `[(source_axis=k, start=0, step=1, steps=source_shape[k])]` for
+ /// each k. Always self-validates.
+ pub fn identity_for_shape(source_shape: &[i64]) -> Self {
+ Self(
+ source_shape
+ .iter()
+ .enumerate()
+ .map(|(k, &s)| ViewEntry {
+ source_axis: k as i64,
+ start: 0,
+ step: 1,
+ steps: s,
+ })
+ .collect(),
+ )
+ }
+
+ pub fn len(&self) -> usize {
+ self.0.len()
+ }
+
+ pub fn is_empty(&self) -> bool {
+ self.0.is_empty()
+ }
+
+ pub fn as_slice(&self) -> &[ViewEntry] {
+ &self.0
+ }
+
+ pub fn iter(&self) -> std::slice::Iter<'_, ViewEntry> {
+ self.0.iter()
+ }
+
+ /// Visible shape derived from `[v.steps for v in self]`. `validate`
+ /// guarantees `steps >= 0`, so callers can treat the result as
+ /// non-negative after a successful validation.
+ pub fn visible_shape(&self) -> Vec<i64> {
+ self.0.iter().map(|v| v.steps).collect()
+ }
+
+ /// True iff this 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 are always contiguous, so the reader borrows the
+ /// underlying `data` column directly through `NdBuffer::as_contiguous()`
+ /// with no allocation or copy. Non-identity views may still be
+ /// contiguous (e.g. an outer-axis slice); strided ones are rejected by
+ /// `as_contiguous()` and must be repacked via an explicit plan node.
+ pub fn is_identity(&self, source_shape: &[i64]) -> bool {
+ if self.0.len() != source_shape.len() {
+ return false;
+ }
+ self.0.iter().enumerate().all(|(k, v)| {
+ v.source_axis == k as i64 && v.start == 0 && v.step == 1 &&
v.steps == source_shape[k]
+ })
+ }
+
+ /// Validate against a band's `source_shape`. `Ok(())` iff:
+ ///
+ /// - `self.len() == source_shape.len()`.
+ /// - `source_axis` values across `self` form a permutation of
+ /// `0..source_shape.len()` (no axis duplicated, none missing).
+ /// - Every `source_shape[k] >= 0`.
+ /// - Every `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.
+ pub fn validate(&self, source_shape: &[i64]) -> Result<(), ArrowError> {
+ let ndim = source_shape.len();
+ if self.0.len() != ndim {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "view length ({}) must equal source_shape length ({ndim})",
+ self.0.len()
+ )));
Review Comment:
Not for this PR, but we have probably reached the limit on how far we have
pushed the ArrowError in this crate. We can either have a RasterError struct
like we do for seonda-geometry, or we can have a sedona-error crate with a
`datafusion` and/or `arrow` feature that handles the impl From to reduce the
number of `map_err`s needed (they collapse to `?` when From is implemented).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
