james-willis commented on code in PR #749:
URL: https://github.com/apache/sedona-db/pull/749#discussion_r3120229476
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -55,32 +54,37 @@ impl RasterSchema {
)))
}
- /// Individual band schema
+ /// Individual band schema — flattened N-D band with dimension metadata
pub fn band_type() -> DataType {
DataType::Struct(Fields::from(vec![
- Field::new(column::METADATA, Self::band_metadata_type(), false),
- Field::new(column::DATA, Self::band_data_type(), false),
+ Field::new(column::NAME, DataType::Utf8, true),
+ Field::new(column::DIM_NAMES, Self::dim_names_type(), false),
+ Field::new(column::SHAPE, Self::shape_type(), false),
+ Field::new(column::DATATYPE, DataType::UInt32, false),
+ Field::new(column::NODATA, DataType::Binary, true),
+ Field::new(column::STRIDES, Self::strides_type(), false),
+ Field::new(column::OFFSET, DataType::UInt64, false),
+ Field::new(column::OUTDB_URI, DataType::Utf8, true),
+ Field::new(column::DATA, DataType::BinaryView, false),
Review Comment:
Thinking about this more, I need to correct something in my earlier stacking
comment. I said stacking breaks at *shuffles*, but that was too optimistic — in
DataFusion, stacking breaks at **every UDF boundary**,
shuffle or not. Every scalar UDF receives `ColumnarValue` (Arrow) and
returns `ColumnarValue`, so `Arc<dyn BandRef>` wrappers can't live across
`RS_A(RS_B(...))` without extra machinery. The stacking model is
still the right *internal* data model — it's what lets a single kernel
compose lazy views — but "stacking survives normal UDF chaining for free" was
wrong.
That correction actually makes the strides/offset story cleaner, because
now it's the same question in both places: *how do we compress a stack into
something Arrow can carry to the next UDF?*
**View-producing stacks compress losslessly.** Any composition of
`StridedBandRef`-shaped operations — `RS_Slice`, `RS_SliceRange`,
`RS_DimToBand`, transposes, constant broadcasts (stride=0) — collapses exactly
into four integers (strides, offset, shape) plus an unmodified `outdb_uri`. So
`RS_Slice(RS_DimToBand(zarr_raster, 'wavelength'), 'time', 0)` serializes to
the next UDF with `outdb_uri` preserved, strides/offset/shape updated, empty
`data`. Full laziness survives the Arrow round-trip; the downstream
`ZarrBandRef` reconstitutes on read. This is exactly what strides/offset buys
us, and it's why I want to keep those fields as typed ints rather than a subset
string.
**Compute-producing stacks compress lossily.** The moment a
`CoarsenBandRef` (or Normalize, MapAlgebra, Resample) is in the stack, we're
producing new pixel values — there's no four-integer view that represents
"coarsened." At the next UDF boundary it has to materialize. Under Phase 1 that
means `RS_Width(RS_Coarsen(r, 2))` pays full materialization cost even though
`Width` only needs `shape`. A `subset: Utf8` doesn't help here either — a
subset DSL can encode ranges/strides/index lists, but not "coarsened."
**The clean future-work answer is operator fusion at plan time.** An
`ExprRewriter` rule detects chains like `RS_Width(RS_Coarsen(RS_Rechunk(r)))`
and rewrites them to a single `RasterPipeline` meta-UDF. Inside
one kernel, the stacking model works exactly as I first described —
`StridedBandRef` → `CoarsenBandRef` → `.shape()` — and Arrow serialization only
happens at the outermost boundary. Fusion is what makes a
multi-UDF chain behave like the single-UDF-internal case where stacking
was always going to work. No schema change, no trait change; can land whenever
the compute-chain pattern shows up as a real bottleneck.
So the revised picture:
- Phase 1 as drafted: view-producing stacks stay lazy across UDF
boundaries via strides/offset + preserved `outdb_uri`. Compute-producing stacks
materialize at the boundary.
- Future: operator fusion lets compute-producing stacks stay lazy too,
without changing the schema we land now.
On `subset: Utf8` specifically — tempting, but the trade-offs point the
other way:
- Parser/validator surface: any DSL needs parsing, error messages, docs,
version stability. Strides/offset are four integers.
- No laziness advantage over strides/offset for view cases, no help at all
for compute cases — the real axis is fusion, not subset expressiveness.
- Interop: strides/offset is the pybuffer / numpy / Arrow FFI vocabulary —
a numpy consumer memory-maps our buffer directly. A DSL needs translation.
- The one thing strides/offset can't express is non-contiguous index lists
(e.g. `y ∈ {2, 3, 5}`); I'd handle that with materialize-then-stride when it
comes up.
If we hit a case strides/offset can't express *and* fusion can't handle,
we can revisit. But for Phase 1 I'd rather keep the schema narrow and treat
fusion as deferred work
--
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]
