Skip to content

perf(rust/sedona-spatial-join-raster): pin raster operand to the probe side - #1074

Merged
james-willis merged 3 commits into
apache:mainfrom
james-willis:jw/raster-join-probe-side
Aug 3, 2026
Merged

perf(rust/sedona-spatial-join-raster): pin raster operand to the probe side#1074
james-willis merged 3 commits into
apache:mainfrom
james-willis:jw/raster-join-probe-side

Conversation

@james-willis

Copy link
Copy Markdown
Contributor

Stacked on #1073 — the diff shows both branches' commits until #1073 merges, then reduces to this branch's single commit.

Pins the raster operand of a raster–vector spatial join to the probe (streamed) side so its BinaryView band buffers stay uncopied, short-lived, and shared across the rows a raster fans out to — instead of being buffered and gc-compacted on the build side.

@github-actions
github-actions Bot requested a review from paleolimbot July 23, 2026 21:30
@james-willis
james-willis force-pushed the jw/raster-join-probe-side branch from 9d9de9f to d9cc755 Compare July 24, 2026 00:04
@jiayuasu

Copy link
Copy Markdown
Member

This is only necessary when we have RS_Intersects followed by a zonal states, right?

The RS_Intersects only uses the metadata of raster so pinning raster to the probe side probably does not matter much?

@james-willis

Copy link
Copy Markdown
Contributor Author

This is only necessary when we have RS_Intersects followed by a zonal states, right?

I guess you could have other operators like udf or AsRaster? I thought it would be fine to keep this simple and not condition the logic.

Would you rather leave the standard build-side logic in most cases and use some optimized rule to pipe in a probe side parameter to the join node? I feel that adds a lot of complexity and im not sure of the value

@jiayuasu

jiayuasu commented Jul 24, 2026

Copy link
Copy Markdown
Member

Correct, I would rather keep the current behavior as it uses statistics to detect the best side to be the probe side.

In fact, I think I should correct our discussion today as follows:

When there is RS_ZonalStats(vec, ras) operator, assume we have input record batches, each of which is as follows (please adapt this to the DataFusion language).

Vec1, Ras1
Vec2, Ras2
Vec3, Ras3
Vec1, Ras2
Vec2, Ras1

If we just perform the operator in the normal order, you will need to either load Ras1 twice or pin Ras1 in memory cache until the end of the query.

Instead, we should have a logical / physical optimization rules (or a new algorithm?) to sort the records in the batch by Ras ID. We will have something as follows

Vec1, Ras1
Vec2, Ras1
Vec2, Ras2
Vec1, Ras2
Vec3, Ras3

I think this has nothing to do with spatial join. It is a pure zonal stats optimization. I think Kristin might have already done this in his Zonal stats implementation. Please double check.

@james-willis

Copy link
Copy Markdown
Contributor Author

Kristins work does not sort the input of the RS functions, but also that doesn't matter when we aren't using some kind of cache.

The reason we want the raster as the probe side is that so we can load before the join. We need to load before duplicating to avoid duplicate loads. If we dupe before load we dont share a BinaryView. if we load before we dupe theyll share the buffer backing the binary view

@jiayuasu

Copy link
Copy Markdown
Member

In vector raster join, we never load the raster data, don't we? We just do the join using the convex hull of the raster metadata and pixels are never used

@paleolimbot

paleolimbot commented Jul 24, 2026

Copy link
Copy Markdown
Member

I would rather keep the current behavior as it uses statistics to detect the best side to be the probe side.

I'll note that we've had a lot of trouble with our current heuristic, which only uses row count or byte size to determine the best probe side. We should use more advanced stats to choose this but we don't (I think! Worth double checking.). In fact, the only non-committer contributions to the spatial join have been to add an option to disable our heuristic 🙂

The current heuristic is to pick the smallest side to index, on the bet that the largest side will have to spill and therefore be slower. On those grounds, we should always be indexing the raster side. A better heuristic is probably to index the geometry side, because those geometries are likely to be more complex and benefit from preparation. It sounds like indexing on the geometry side is also better for the case where we have materialized rasters.

@jiayuasu

jiayuasu commented Jul 24, 2026

Copy link
Copy Markdown
Member

@paleolimbot Either way, the raster join only uses the convex hull of the raster and then it becomes a vector - vector join. Then we don't need to have any special treatment for raster. You can argue that raster data's hull is simpler but in that case, we need to have better stats in vector vector spatial join to fix that.

@paleolimbot

Copy link
Copy Markdown
Member

Sure, but it sounds like then we loose zero copy for (maybe tiled) numpy arrays that hit the zonal stats.

We also need better stats (in particular for pushdown between sides of a join, but also to possibly make a better decision on which side to build on), but that shouldn't block the best choice for the implementation we have today.

@james-willis

Copy link
Copy Markdown
Contributor Author

We need special treatment for raster because we don't want to load the same image from the network many times.

Thats the idea of this PR:

  1. first we load so when theres 0 dupes of rasters we've already created the binary view
  2. then we join so when we copy the raster from the input to the output we reuse the backing buffer of the binary view. We use the probe side so rasters sharing a buffer backing their binary view tend to wind up in the same batch of downstream
  3. finally run zonal stats. all copies of the same raster come through the same batch and thus all references to the same buffer are released. This keeps peak ram usage down.

The alternative is a cache.

@james-willis
james-willis force-pushed the jw/raster-join-probe-side branch from d9cc755 to e5e0e8a Compare July 24, 2026 16:55
@paleolimbot

Copy link
Copy Markdown
Member

If I'm reading this correctly, there's danger that by doing that we're also loading a lot of unnecessary images (i.e., images that never touch a feature). If we keep them lazy there's maybe chance we can load some of them (and, crucially, drop the loaded versions of them) at a time?

@james-willis
james-willis force-pushed the jw/raster-join-probe-side branch from 489a352 to 36b0427 Compare July 24, 2026 17:43
@james-willis

Copy link
Copy Markdown
Contributor Author

@paleolimbot yes that is true.

@james-willis
james-willis force-pushed the jw/raster-join-probe-side branch from 36b0427 to ab10acc Compare July 24, 2026 19:12
…e side

Raster relation joins always place the raster operand on the probe (right)
side of SpatialJoinExec, overriding the row-count reordering heuristic. The
build side is fully buffered and compacts view arrays on ingest, copying
raster BinaryView band payloads and defeating the zero-copy design; the
probe side streams without compaction and assembles output with
arrow::compute::take, which shares view-array data buffers. When the raster
is the build-side input, swap_inputs() moves it to the probe side (inverting
the predicate and reordering output) and carries the raster provider onto
the rebuilt exec.
…de pinning

Explains why the raster operand is pinned to the probe side and links the
open question of whether to pin unconditionally or only when the vector
side isn't vastly larger.
@james-willis
james-willis force-pushed the jw/raster-join-probe-side branch from ab10acc to 0e0c99c Compare July 30, 2026 00:15
@james-willis
james-willis marked this pull request as ready for review July 30, 2026 17:13

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

This nicely fixes the "the index build must happen in a common CRS" problem in addition to ensuring materialized rasters are not copied.

@james-willis
james-willis merged commit 2001e10 into apache:main Aug 3, 2026
16 checks passed
@james-willis
james-willis deleted the jw/raster-join-probe-side branch August 3, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants