[WIP] feat(raster): view machinery for non-identity band views - #813
[WIP] feat(raster): view machinery for non-identity band views#813james-willis wants to merge 12 commits into
Conversation
349c957 to
91966ed
Compare
`raster_ref_to_gdal_mem` previously returned a `Result<Dataset>` and
guarded against `BandRef::contiguous_data()` returning `Cow::Owned`
with a runtime tripwire ("Internal: contiguous_data must be borrowed
for is_2d bands; got owned"). The check was correct — handing GDAL a
pointer into a `Vec<u8>` that drops at the end of the iteration would
dangle — but it ties an internal invariant ("`is_2d` ⇒ Borrowed") to
incidental properties of today's reader. Any future copy path in the
reader (compression, BinaryView block-boundary stitching, alignment
fix-up, sliced/broadcast/transposed views from apache#813 / apache#750) would
detonate the tripwire on perfectly valid 2-D rasters.
Change: return `Result<(Dataset, Vec<Vec<u8>>)>`. On `Cow::Borrowed`
the GDAL band still points directly at the StructArray buffer
(zero-copy). On `Cow::Owned` we move the `Vec<u8>` out of the Cow
without copying — the reader's existing materialization is the only
allocation — and stash it in the returned vector. The caller (the
provider in `gdal_dataset_provider.rs`) parks it in a new
`RasterDataset::_owned_band_bytes` field that lives as long as the
MEM dataset that holds the pointers.
`raster_ref_to_gdal_empty` discards the always-empty vector.
9e5ae44 to
7f2f79a
Compare
d27d8d1 to
e890f90
Compare
paleolimbot
left a comment
There was a problem hiding this comment.
This is very cool!
I think I got there by the end in the inline comments, but I think a struct ViewEntries with associated functions in src/view_entries.rs where the non serialize-to-arrow and deserialize-from-arrow logic behind this lives will make this clearer.
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.
| None, | ||
| &["y", "x"], | ||
| &[3, 3], // 3x3 source | ||
| &view, | ||
| BandDataType::Float32, | ||
| None, | ||
| None, | ||
| None, |
There was a problem hiding this comment.
A BandBuilder may help you quite a bit here since there are a lot of None that a default constructor could fill in
There was a problem hiding this 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:
| #[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 |
There was a problem hiding this 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.
There was a problem hiding this comment.
I reduce the tests here. they should now be just builder-integration specific. will confirm before resolving this comment
|
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)
|
|
Possibly? I know it seems like a lot of comments but what I'm getting at is: 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: 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 |
b81d5f1 to
9267b04
Compare
31878b5 to
4564e8b
Compare
423b516 to
23d87ac
Compare
Brings the N-D dimension query/manipulation functions branch up to current main (48 commits: N-D lazy loading, Zarr, byte-access surface change, etc.). - lib.rs module list: keep rs_dim_band + rs_dimensions (this branch) alongside rs_ensure_loaded (main). - Migrate the manipulation functions off the removed BandRef::contiguous_data() to nd_buffer() + NdBuffer::as_contiguous() (apache#915). These functions materialise eagerly via start_band_nd, so identity-view source bands read contiguous bytes directly; no dependency on the view machinery (apache#813).
e1fce6f to
29e9d89
Compare
Integrate the view-spec layer into the raster type. The reader's band() composes a band's view into byte strides + offset (with overflow and buffer-bounds checks) so a non-identity view decodes instead of being rejected; nd_buffer() exposes the strided region and as_contiguous() borrows it zero-copy when packed. The builder gains start_band_with_view and with_view to construct sliced / broadcast / permuted / stacked views. Stacked on the ViewEntries module PR.
29e9d89 to
0e940ae
Compare
Per review, band() mixed several levels of abstraction. Extract resolve_band_row (index -> bounds-checked row), compose_band_layout (read + validate view, compose byte strides/offset), and check_band_buffer_bounds (InDb data-buffer bounds), bundling the composed result in a BandLayout struct. band() now reads as resolve -> decode dtype -> compose -> construct. No behavior change.
… add indb_band_meta helper Per review, builder.rs (Arrow serialization) carried tests that actually exercise reader/NdBuffer byte-layout semantics. Move the 9 nd_buffer/ as_contiguous/strides/contiguity tests (broadcast + negative-step rejection, outer-axis-slice contiguity, permutation) to array.rs alongside the other reader tests; keep builder-API and view-serialization tests (start_band_with_view, view-field-null, IPC round-trip) in builder.rs. Add a small indb_band_meta(datatype) test helper to cut repeated InDb BandMetadata literals (3 call sites). No behavior change.
…ith_view shares buffer zero-copy
Add a band-driven derive API: BandRef::copy_into(builder, BandOverrides{..})
writes a derived band, inheriting unspecified fields from the source and
carrying its bytes over. The data step is a separate append_data_into trait
method — default copies via append_value; BandRefImpl overrides it to share
the source row's backing Buffer zero-copy (append_band_data_from), keeping the
Arrow buffer plumbing encapsulated (no raw Buffer accessor on the trait).
with_view now composes the view and delegates to copy_into, so InDb derivation
no longer copies the source bytes per call — it shares the backing buffer
(verified by a ptr-equality test). Implements the RasterBuilder copy-ergonomics
groundwork (DB-81); the copy_raster_from sibling and the RS_EnsureLoaded
view-preserving passthrough follow.
…ero-copy)
Add a band-driven derive API: BandRef::copy_into(builder, BandOverrides{..})
writes a derived band, inheriting unspecified fields (dim names, shape, data
type, nodata, OutDb pointers) from the source and carrying its bytes over. The
data step is a separate append_data_into trait method — default copies via
append_value; BandRefImpl overrides it to share the source row's backing Buffer
zero-copy (append_band_data_from), keeping the Arrow buffer plumbing
encapsulated (no raw Buffer accessor on the trait).
Identity-view only: the derived band uses start_band_nd. View-carrying
overrides land with the view machinery. First step of DB-81; consumers
(RS_EnsureLoaded rebuild, etc.) migrate next.
copy_into emits an identity-view band and copies the source's visible bytes assuming offset 0 / canonical strides. Guard that precondition at the single dispatch point (covering both the default and the Arrow append_band_data_from data paths): a non-identity source view now errors loudly instead of silently copying mislocated bytes. Carrying views is a follow-up (the view machinery).
Replace the in-copy_into non-identity guard with a future-proof seam: - Add RasterBuilder::start_band_nd_with_view, which persists an explicit view (identity -> the canonical null sentinel; non-identity -> errors, gated until view persistence lands, apache#897). - copy_into now composes BandOverrides.view onto the source's own view and forwards the result, so callers express overrides in the source's visible coordinates and never manage composition themselves. - Add BandOverrides.view. When non-identity view persistence lands, only the builder + reader + loader change — copy_into, append_data_into, and BandOverrides are frozen.
…::try_new main renamed RasterStructArray::new -> try_new; update the copy_into tests (merged in from this branch) to match.
# Conflicts: # rust/sedona-raster/src/array.rs # rust/sedona-raster/src/builder.rs
# Conflicts: # rust/sedona-raster/src/array.rs # rust/sedona-raster/src/builder.rs # rust/sedona-raster/src/traits.rs
|
subsumed by #1113 |
Summary
Integrates the view-machinery layer into the raster type. Bands can be constructed and read with non-identity
viewentries — slices, broadcasts, axis permutations, and reverse-iteration. The byte-access surface is a single zero-copy accessor,nd_buffer(), which exposes the source buffer plus the visible region's shape/strides/offset; consumers that need flat row-major bytes borrow them viaNdBuffer::as_contiguous(), which errors on strided layouts rather than allocating.Builds on two pieces that already landed on
main: theViewEntriesview-spec module (validate/compose/visible_shape/is_identity, #934) and the i64source_shapecolumn (#927). This PR wires them into band construction and reads.What's in this PR
Three layers, plus the GDAL bridge:
Trait surface — zero-copy byte access (
rust/sedona-raster/src/traits.rs)NdBuffer::is_contiguous()— pure function of(shape, strides, data_type). C-order packed-stride check, innermost-first, offset-agnostic so an outer-axis slice from a nonzero offset still counts as contiguous. Broadcast (stride 0), reverse (negative stride), permuted, gapped, and inner-strided layouts are not contiguous; a zero-extent axis is trivially contiguous.NdBuffer::as_contiguous() -> Result<&[u8]>— borrows the visible bytes zero-copy when contiguous, else errors directing the caller toRS_EnsureContiguous(RS_EnsureContiguous UDF: materialize strided band views to contiguous bytes via an explicit plan node #899). Never allocates.Builder (
rust/sedona-raster/src/builder.rs)start_band_with_view()API (args bundled inStartBandWithViewArgs).with_view()API (args bundled inWithViewArgs) — compose a new view over an existing band.start_band, 0-D rejection, view-step validation, theviewcolumn being NULL for identity bands, and Arrow IPC round-trip.Reader (
rust/sedona-raster/src/array.rs)RasterRefImpl::bandresolves the band row, decodes the data type, and composes view -> byte strides + byte offset (compose_band_layout/check_band_buffer_bounds) with checked arithmetic; malformed views and arithmetic overflows route throughsedona_internal_datafusion_err!so they surface with the standard "SedonaDB internal error" framing.nd_buffer()returns the source buffer + shape/strides/offset (zero-allocation strided access) and is the sole byte accessor. OutDb bands return an error fromnd_buffer()(backend resolvers are tracked separately).nd_bufferstrides across dtypes, broadcast and negative-step rejection, and permutation+slice composition.GDAL bridge (
rust/sedona-raster-gdal/src/{gdal_common,gdal_dataset_provider}.rs)raster_ref_to_gdal_memborrows each band viand_buffer()?.as_contiguous()?.is_2d()already requires an identity view, so the borrow always succeeds — no owned-bytes copy is threaded through the dataset provider.