feat(rust/sedona-spatial-join): Integrate libgpuspatial into sedona-spatial-join#722
feat(rust/sedona-spatial-join): Integrate libgpuspatial into sedona-spatial-join#722pwrliang wants to merge 4 commits intoapache:mainfrom
Conversation
|
@paleolimbot @Kontinuation Could you take a look when you are available? |
| fn merge_stats(&mut self, stats: GeoStatistics) -> &mut Self { | ||
| self.stats.merge(&stats); | ||
| self | ||
| } |
There was a problem hiding this comment.
GPU join does not make use of geo statistics. We can remove merge_stats and stats field altogether.
| arrow::compute::concat_batches(&schema, all_record_batches).map_err(|e| { | ||
| DataFusionError::Execution(format!("Failed to concatenate left batches: {}", e)) | ||
| })?; |
There was a problem hiding this comment.
There is no need to .map_err for ArrowError, it could be converted to DataFusionError::ArrowError automatically.
|
|
||
| let sedona_type = self.indexed_batches[0].geom_array.sedona_type.clone(); | ||
|
|
||
| if self.options.gpu.concat_build { |
There was a problem hiding this comment.
Do we have tests covering the gpu.concat_build: false case?
| if wkb.geometry_type() == GeometryType::Point { | ||
| Some(Rect::new( | ||
| coord!(x: min.x as f32, y: min.y as f32), | ||
| coord!(x: max.x as f32, y: max.y as f32), | ||
| )) |
There was a problem hiding this comment.
Will this introduce false negatives since the resulting bounding box is not guaranteed to cover the original bounding box?
pwrliang
force-pushed
the
integrate/libgpuspatial
branch
from
1d3b96b to
41b97c8
Compare
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you for working on this!
My main concern with this PR is that the GPU details are very entangled with the non-GPU details. How about:
sedona-spatial-join-common(defines the traits you nicely separated in the last PR and aSedonaSpatialJoinExtensiontrait)sedona-spatial-join-gpu(implements the traits in sedona-spatial-join-common)sedona-spatial-join(uses anArc<dyn SedonaSpatialJoinExtension>object to decide if an extension join should be used in place of the normal join). The hook to register a join extension can be a global function for now and you can use a mock join extension built from the default implementation index/refiner to test it.sedona(when built with the gpu feature, registers the Gpu join with sedona-spatial-join)
There are a few things I think this will help with:
- We may want to add other join accelerators for other hardware (e.g., Apple GPU) or other data types (Geography x H3 cell join), or possibly Raster/Vector join.
- It will make it easier for you to have clear ownership over a subdirectory so you can iterate faster.
- It makes the
sedonacrate the single place where features are assembled (to avoid two levels of feature flags, the consequences of which you've started to identify here with the "all except GPU" feature).
I do think it's important to get this right early while we are all here and have time to dedicate to this!
Thanks for the suggestions. If I understand correctly, this design implies that |
I don't think we need to (although we can if it is easier)....I had in mind that the It may be worth trying to separate that all within sedona-spatial-join first to see what it will look like (and then move whatever has to be moved to sedona-spatial-join-common, and then move whatever has to be moved into sedona-spatial-join-gpu. |
I think the main difference is that (1) the index is now configurable where it wasn't before and (2) tg, geo, and geos are trivial dependencies to satisfy. I'm happy to give this a go on Monday! |
3 participants
This PR integrates libgpuspatial by providing a
GPUSpatialIndexandGPUSpatialIndexBuilder. The PR should pass the CI after its predecessor PRs (#717, #718, #719, #721) are merged.