james-willis opened a new pull request, #1196:
URL: https://github.com/apache/sedona-db/pull/1196
## The smell
```rust
ViewEntries::new(input.view().to_vec()).compose(&ViewEntries::new(view.to_vec()))?
```
Two heap allocations to read two slices. It looks like a workaround because
it is one.
## Why it was there
`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:
| | before |
|---|---|
| `BandRef::view()` | `&[ViewEntry]` |
| `StartBandArgs::view` | `Option<&[ViewEntry]>` |
| `WithViewArgs::view` | `&[ViewEntry]` |
| `BandOverrides::view` | `Option<&[ViewEntry]>` |
| `RasterLoadRequest::view` | `&[ViewEntry]` |
| `RasterLoadResult::view` | `Vec<ViewEntry>` |
So slices were the real currency and `ViewEntries` was a method bag.
`compose` was the only operation demanding the owned type on *both* sides —
which is why every caller had to allocate its way in.
Two supporting details:
- `BandRefImpl` **already stored** a `ViewEntries` and called `.as_slice()`
on the way out. Returning the container removes a conversion rather than adding
one.
- Both doc comments describing this API were **uncompilable as written** —
they said `source.view().compose(&next)?`, but `view()` returned a slice, which
has no `compose`. They described the API this PR builds.
## Changes
- `BandRef::view() -> &ViewEntries`
- `compose`'s `next` takes `impl AsRef<[ViewEntry]>`, so a `ViewEntries` or
a bare slice both work — existing `a.compose(&b)` call sites are untouched
- The six fields above all carry `ViewEntries`
- New `EMPTY_VIEW`: a `&'static` empty view for call sites that must supply
one but have no view to describe. `Vec::new` allocates nothing, so the loader
tests that passed `&[]` need no local binding
The motivating site is now:
```rust
let effective_view = match overrides.view {
Some(v) => self.view().compose(v)?,
None => self.view().clone(),
};
```
## No behaviour change
Purely the type views travel in. Composition semantics, validation, and
persistence are all untouched.
```
sedona-raster 183 passed; 0 failed
sedona-raster-functions 256 passed; 0 failed
sedona-raster-zarr 66 passed; 0 failed
```
fmt and clippy clean; `sedona-raster-gdal` builds.
## Note
#1158 will be rebased on top of this — it converts `BandOverrides` to a
trinary `Override<T>`, which composes cleanly with the type change 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
