refactor(rust/sedona-raster): make ViewEntries the interface for passing views - #1196
Merged
Conversation
james-willis
force-pushed
the
james/viewentries-interface
branch
from
August 27, 2026 19:11
1b3de73 to
5ddcfb1
Compare
james-willis
force-pushed
the
james/viewentries-interface
branch
from
August 27, 2026 20:32
5ddcfb1 to
7dd2c1b
Compare
…ing views
`ViewEntries` existed as the container type but nothing actually passed it
around: `BandRef::view()` returned `&[ViewEntry]`, and every struct field
carrying a view held a bare slice. So each operation needing the type
re-wrapped a slice, and `compose` — taking `&Self` on both sides — forced two
allocations to read two slices:
let source_view = ViewEntries::new(self.view().to_vec());
let effective_view = match overrides.view {
Some(v) => source_view.compose(&ViewEntries::new(v.to_vec()))?,
None => source_view,
};
becomes
let effective_view = match overrides.view {
Some(v) => self.view().compose(v)?,
None => self.view().clone(),
};
Changes:
* `BandRef::view()` returns `&ViewEntries`. `BandRefImpl` already stored one
and called `as_slice()` on the way out, so this removes a conversion.
* `compose`'s `next` takes `impl AsRef<[ViewEntry]>`, so a `ViewEntries` or a
bare slice both work and existing call sites are unchanged.
* `StartBandArgs::view`, `WithViewArgs::view`, `BandOverrides::view`,
`RasterLoadRequest::view` and `RasterLoadResult::view` hold `ViewEntries`.
* Delete `rs_ensure_loaded::view_is_identity`, which duplicated
`ViewEntries::is_identity` and had silently diverged from it: the local copy
short-circuited `view.is_empty()` to true, so an empty view counted as
identity over *any* source shape. An empty view is only identity for a
0-dimensional raster, which is what the method already implements. Nothing
in production produced an empty view — `start_band` rejects 0-dimensional
bands — so the divergence was only reachable from loader tests that passed
one. Those now pass a real identity view over their source shape.
No behaviour change to composition, validation, or persistence — only the
type views travel in, plus the identity check now having one definition.
james-willis
force-pushed
the
james/viewentries-interface
branch
from
August 27, 2026 20:53
7dd2c1b to
16ce9ae
Compare
james-willis
requested review from
jiayuasu and
paleolimbot
and removed request for
prantogg
August 27, 2026 20:55
jiayuasu
approved these changes
Aug 28, 2026
Member
|
LGTM! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The smell
Two heap allocations to read two slices. It looks like a workaround because it is one.
Why it was there
ViewEntriesexisted as the container type, but nothing actually passed it around.BandRef::view()returned&[ViewEntry], and every struct field carrying a view held a bare slice:BandRef::view()&[ViewEntry]StartBandArgs::viewOption<&[ViewEntry]>WithViewArgs::view&[ViewEntry]BandOverrides::viewOption<&[ViewEntry]>RasterLoadRequest::view&[ViewEntry]RasterLoadResult::viewVec<ViewEntry>So slices were the real currency and
ViewEntrieswas a method bag.composewas the only operation demanding the owned type on both sides — which is why every caller had to allocate its way in.Two supporting details:
BandRefImplalready stored aViewEntriesand called.as_slice()on the way out. Returning the container removes a conversion rather than adding one.source.view().compose(&next)?, butview()returned a slice, which has nocompose. They described the API this PR builds.Changes
BandRef::view() -> &ViewEntriescompose'snexttakesimpl AsRef<[ViewEntry]>, so aViewEntriesor a bare slice both work — existinga.compose(&b)call sites are untouchedViewEntriesThe motivating site is now:
A duplicate identity check, silently diverged
Chasing the same thread turned up
rs_ensure_loaded::view_is_identity, a private copy ofViewEntries::is_identity— which had drifted from it:source_shape = [2, 3]view_is_identitytrue(short-circuits onis_empty())ViewEntries::is_identityfalse(length mismatch)An empty view is identity only for a 0-dimensional raster, which is what the method already implements — the local copy was wrong.
It was unreachable in production:
RasterLoadResult::unresolvedclones the request's view, the request's view is the band's view, andstart_bandrejects 0-dimensional bands outright. The only things exercising the divergence were loader tests passing an empty view alongside a 2-D or 3-Dsource_shape— a state that cannot occur. Those fixtures now pass a real identity view over their source shape, and the helper is deleted in favour of the method.No behaviour change
Composition, validation, and persistence are untouched. The only semantic change is that the identity check now has one definition instead of two that disagreed.
fmt clean, clippy clean (no unused imports),
sedona-raster-gdalbuilds.Note
#1158 is stacked on this branch — it converts
BandOverridesto a trinaryOverride<T>, which composes cleanly with the type change here.