Copilot commented on code in PR #737:
URL: https://github.com/apache/sedona-db/pull/737#discussion_r2997575846
##########
rust/sedona-spatial-join/src/prepare.rs:
##########
@@ -440,6 +448,7 @@ impl SpatialJoinComponentsBuilder {
self.probe_threads_count,
build_partitions,
SpatialJoinBuildMetrics::new(0, &self.metrics),
+ Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
This call hard-codes `DefaultSpatialJoinProvider` instead of using the
configurable `self.join_provider`, so custom index/evaluated-array behavior
won’t be applied for single-partition joins. Pass `self.join_provider.clone()`
here. (Also `DefaultSpatialJoinProvider {}` won’t compile because it’s a unit
struct.)
```suggestion
self.join_provider.clone(),
```
##########
rust/sedona-spatial-join/src/index/partitioned_index_provider.rs:
##########
@@ -572,6 +582,7 @@ mod tests {
1,
vec![build_partition],
SpatialJoinBuildMetrics::new(0, &metrics),
+ Arc::new(DefaultSpatialJoinProvider {}),
);
Review Comment:
Test code constructs `DefaultSpatialJoinProvider` as
`DefaultSpatialJoinProvider {}`, but it is a unit struct and this won’t
compile. Use `DefaultSpatialJoinProvider` in the `Arc::new(...)` calls.
##########
rust/sedona-spatial-join/src/index/default_spatial_index_builder.rs:
##########
@@ -232,17 +234,24 @@ impl SpatialIndexBuilder for DefaultSpatialIndexBuilder {
refiner_mem_usage + knn_components_mem_usage + rtree_mem_usage
}
- fn finish(mut self) -> Result<SpatialIndexRef> {
+ fn operand_evaluator(&self) -> Arc<dyn OperandEvaluator> {
+ create_operand_evaluator(
+ &self.spatial_predicate,
+ Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
`operand_evaluator()` is hard-coded to use `DefaultSpatialJoinProvider`, so
a custom `SpatialJoinProvider` (e.g., overriding bounding-rect computation)
won’t be used during build-side evaluation/collection. The builder should carry
(or be constructed with) the provider and use it here instead of always
allocating a default provider. Also `DefaultSpatialJoinProvider {}` won’t
compile for a unit struct.
```suggestion
Arc::new(DefaultSpatialJoinProvider),
```
##########
rust/sedona-spatial-join/src/prepare.rs:
##########
@@ -477,6 +486,7 @@ impl SpatialJoinComponentsBuilder {
self.probe_threads_count,
merged_spilled_partitions,
SpatialJoinBuildMetrics::new(0, &self.metrics),
+ Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
This multi-partition path also hard-codes `DefaultSpatialJoinProvider`,
bypassing the configurable `self.join_provider` (needed for Geography/GPU
overrides). Pass `self.join_provider.clone()` here. (Also
`DefaultSpatialJoinProvider {}` won’t compile because it’s a unit struct.)
```suggestion
self.join_provider.clone(),
```
##########
rust/sedona-spatial-join/src/lib.rs:
##########
@@ -18,6 +18,7 @@
pub mod evaluated_batch;
pub mod exec;
mod index;
+mod join_provider;
pub mod operand_evaluator;
Review Comment:
The PR description indicates this extension point should enable
GPU/Geography join implementations in separate crates, but `join_provider` is
not exported (`mod join_provider;` and the trait is `pub(crate)`). If external
crates need to implement/providers, consider making the module `pub` and the
trait `pub` (and re-exporting it from `lib.rs`).
##########
rust/sedona-spatial-join/src/index/default_spatial_index.rs:
##########
@@ -111,7 +111,11 @@ impl DefaultSpatialIndex {
options: SpatialJoinOptions,
probe_threads_counter: AtomicUsize,
) -> Self {
- let evaluator = create_operand_evaluator(&spatial_predicate,
options.clone());
+ let evaluator = create_operand_evaluator(
+ &spatial_predicate,
+ Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
`DefaultSpatialJoinProvider` is defined as a unit struct, so
`DefaultSpatialJoinProvider {}` will not compile. Construct it as
`DefaultSpatialJoinProvider` when passing into `Arc::new(...)`.
```suggestion
Arc::new(DefaultSpatialJoinProvider),
```
##########
rust/sedona-spatial-join/src/index/partitioned_index_provider.rs:
##########
@@ -613,6 +624,7 @@ mod tests {
1,
spilled_partitions,
SpatialJoinBuildMetrics::new(0, &metrics),
+ Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
Test code constructs `DefaultSpatialJoinProvider` as
`DefaultSpatialJoinProvider {}`, but it is a unit struct and this won’t
compile. Use `DefaultSpatialJoinProvider` in the `Arc::new(...)` call.
```suggestion
Arc::new(DefaultSpatialJoinProvider),
```
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -172,6 +175,7 @@ impl SpatialJoinExec {
cache,
once_async_spatial_join_components: Arc::new(Mutex::new(None)),
seed,
+ join_provider: Arc::new(DefaultSpatialJoinProvider {}),
Review Comment:
`DefaultSpatialJoinProvider` is a unit struct (`struct
DefaultSpatialJoinProvider;`), so constructing it as
`DefaultSpatialJoinProvider {}` will not compile. Use
`DefaultSpatialJoinProvider` (or `DefaultSpatialJoinProvider::default()` if
implemented) when wrapping in `Arc::new(...)`.
```suggestion
join_provider: Arc::new(DefaultSpatialJoinProvider),
```
--
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]
