You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "Kontinuation (via GitHub)" <gi...@apache.org> on 2025/11/05 16:44:53 UTC
[PR] chore: Refactored spatial join code for easier implementing new features [sedona-db]
Kontinuation opened a new pull request, #277:
URL: https://github.com/apache/sedona-db/pull/277
This PR refactors the structure of spatial join code to make it easier to add new features, such as out-of-core spatial joins. The main changes are:
* Breaks the largest source file index.rs in rust/sedona-spatial-join into several smaller modules:
* `collect`: Collecting build side batches from the build side stream and evaluating the expressions for computing the joined spatial column
* `index`: Splitted into spatial index, spatial index builder and a knn_adapter module for geometry distance computation.
* Move top-level utility modules into `utils` module
* Memory reservation for prepared geometries held by the refiners happens only once before probing. The refiners needs to estimate the maximum amount of memory for holding prepared geometry objects. This is for avoiding reserving memory in probe phase, where memory reservation is very likely to happen when there are downstream memory-consuming operators.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2501690607
##########
rust/sedona-spatial-join/src/refine/geos.rs:
##########
@@ -321,6 +319,12 @@ impl GeosRefiner {
}
}
+fn estimate_prep_geom_in_mem_size(wkb: &Wkb<'_>) -> usize {
+ // TODO: This is a rough estimate of the memory usage of the prepared geometry and
+ // may not be accurate.
Review Comment:
Filed #281 and updated comment
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2501691512
##########
rust/sedona-spatial-join/src/index/spatial_index.rs:
##########
@@ -0,0 +1,1487 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::{
+ atomic::{AtomicUsize, Ordering},
+ Arc,
+};
+
+use arrow_array::RecordBatch;
+use arrow_schema::SchemaRef;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::{MemoryPool, MemoryReservation};
+use geo_index::rtree::distance::{DistanceMetric, GeometryAccessor};
+use geo_index::rtree::{sort::HilbertSort, RTree, RTreeBuilder, RTreeIndex};
+use geo_index::IndexableNum;
+use geo_types::{Point, Rect};
+use parking_lot::Mutex;
+use sedona_expr::statistics::GeoStatistics;
+use sedona_geo::to_geo::item_to_geometry;
+use sedona_geo_generic_alg::algorithm::Centroid;
+use wkb::reader::Wkb;
+
+use crate::{
+ collect::BuildSideBatch,
+ index::{
+ knn_adapter::{KnnComponents, SedonaKnnAdapter},
+ IndexQueryResult, QueryResultMetrics,
+ },
+ operand_evaluator::{create_operand_evaluator, OperandEvaluator},
+ refine::{create_refiner, IndexQueryResultRefiner},
+ spatial_predicate::SpatialPredicate,
+ utils::concurrent_reservation::ConcurrentReservation,
+};
+use arrow::array::BooleanBufferBuilder;
+use sedona_common::{option::SpatialJoinOptions, ExecutionMode};
+
+pub(crate) struct SpatialIndex {
+ schema: SchemaRef,
+
+ /// The spatial predicate evaluator for the spatial predicate.
+ evaluator: Arc<dyn OperandEvaluator>,
+
+ /// The refiner for refining the index query results.
+ refiner: Arc<dyn IndexQueryResultRefiner>,
+
+ /// Memory reservation for tracking the memory usage of the refiner
+ refiner_reservation: ConcurrentReservation,
+
+ /// R-tree index for the geometry batches. It takes MBRs as query windows and returns
+ /// data indexes. These data indexes should be translated using `data_id_to_batch_pos` to get
+ /// the original geometry batch index and row index, or translated using `prepared_geom_idx_vec`
+ /// to get the prepared geometries array index.
+ rtree: RTree<f32>,
+
+ /// Indexed batches containing evaluated geometry arrays. It contains the original record
+ /// batches and geometry arrays obtained by evaluating the geometry expression on the build side.
+ indexed_batches: Vec<BuildSideBatch>,
+ /// An array for translating rtree data index to geometry batch index and row index
+ data_id_to_batch_pos: Vec<(i32, i32)>,
+
+ /// An array for translating rtree data index to consecutive index. Each geometry may be indexed by
+ /// multiple boxes, so there could be multiple data indexes for the same geometry. A mapping for
+ /// squashing the index makes it easier for persisting per-geometry auxiliary data for evaluating
+ /// the spatial predicate. This is extensively used by the spatial predicate evaluators for storing
+ /// prepared geometries.
+ geom_idx_vec: Vec<usize>,
+
+ /// Shared bitmap builders for visited left indices, one per batch
+ visited_left_side: Option<Mutex<Vec<BooleanBufferBuilder>>>,
+
+ /// Counter of running probe-threads, potentially able to update `bitmap`.
+ /// Each time a probe thread finished probing the index, it will decrement the counter.
+ /// The last finished probe thread will produce the extra output batches for unmatched
+ /// build side when running left-outer joins. See also [`report_probe_completed`].
+ probe_threads_counter: AtomicUsize,
+
+ /// Shared KNN components (distance metrics and geometry cache) for efficient KNN queries
+ knn_components: KnnComponents,
+
+ /// Memory reservation for tracking the memory usage of the spatial index
+ /// Cleared on `SpatialIndex` drop
+ #[expect(dead_code)]
+ reservation: MemoryReservation,
+}
+
+impl SpatialIndex {
+ pub fn empty(
+ spatial_predicate: SpatialPredicate,
+ schema: SchemaRef,
+ options: SpatialJoinOptions,
+ probe_threads_counter: AtomicUsize,
+ mut reservation: MemoryReservation,
+ memory_pool: Arc<dyn MemoryPool>,
+ ) -> Self {
+ let evaluator = create_operand_evaluator(&spatial_predicate, options.clone());
+ let refiner = create_refiner(
+ options.spatial_library,
+ &spatial_predicate,
+ options.clone(),
+ 0,
+ GeoStatistics::empty(),
+ );
+ let refiner_reservation = reservation.split(0);
+ let refiner_reservation = ConcurrentReservation::try_new(0, refiner_reservation).unwrap();
+ let rtree = RTreeBuilder::<f32>::new(0).finish::<HilbertSort>();
+ Self {
+ schema,
+ evaluator,
+ refiner,
+ refiner_reservation,
+ rtree,
+ data_id_to_batch_pos: Vec::new(),
+ indexed_batches: Vec::new(),
+ geom_idx_vec: Vec::new(),
+ visited_left_side: None,
+ probe_threads_counter,
+ knn_components: KnnComponents::new(0, &[], memory_pool.clone()).unwrap(), // Empty index has no cache
+ reservation,
+ }
+ }
+
+ #[allow(clippy::too_many_arguments)]
+ pub fn new(
Review Comment:
Dropped this new() call and marked all fields as `pub(crate)` to be accessible in spatial_index_builder.rs.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2504001651
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -546,24 +543,21 @@ impl SpatialJoinExec {
let num_partitions = build_side.output_partitioning().partition_count();
let mut build_streams = Vec::with_capacity(num_partitions);
- let mut build_metrics = Vec::with_capacity(num_partitions);
for k in 0..num_partitions {
let stream = build_side.execute(k, Arc::clone(&context))?;
build_streams.push(stream);
- build_metrics.push(SpatialJoinBuildMetrics::new(k, &self.metrics));
}
let probe_thread_count = self.right.output_partitioning().partition_count();
Review Comment:
I also found that the properties of SpatialJoinExec may be incorrect for KNN join. For instance, [`maintains_input_order`](https://github.com/apache/sedona-db/blob/2939063fbac28dc06a03e1d18743fa327fbba7ed/rust/sedona-spatial-join/src/exec.rs#L215-L223) assumed that the right side is the probe side, which may not be the case for KNN join. We also need to revise the correctness of `compute_properties` of SpatialJoinExec as well.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2501695361
##########
rust/sedona-spatial-join/src/index/spatial_index_builder.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::BooleanBufferBuilder;
+use arrow_schema::SchemaRef;
+use datafusion_physical_plan::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder};
+use sedona_common::SpatialJoinOptions;
+use sedona_expr::statistics::GeoStatistics;
+
+use datafusion_common::{utils::proxy::VecAllocExt, Result};
+use datafusion_execution::memory_pool::{MemoryConsumer, MemoryPool, MemoryReservation};
+use datafusion_expr::JoinType;
+use futures::StreamExt;
+use geo_index::rtree::{sort::HilbertSort, RTree, RTreeBuilder};
+use parking_lot::Mutex;
+use std::sync::{atomic::AtomicUsize, Arc};
+
+use crate::{
+ collect::{BuildPartition, BuildSideBatch},
+ index::{knn_adapter::KnnComponents, spatial_index::SpatialIndex},
+ operand_evaluator::create_operand_evaluator,
+ refine::create_refiner,
+ spatial_predicate::SpatialPredicate,
+ utils::{
+ concurrent_reservation::ConcurrentReservation, join_utils::need_produce_result_in_final,
+ },
+};
+
+// Type aliases for better readability
+type SpatialRTree = RTree<f32>;
+type DataIdToBatchPos = Vec<(i32, i32)>;
+type RTreeBuildResult = (SpatialRTree, DataIdToBatchPos);
+
+/// Rough estimate for in-memory size of the rtree per rect in bytes
+const RTREE_MEMORY_ESTIMATE_PER_RECT: usize = 60;
+
+/// The prealloc size for the refiner reservation. This is used to reduce the frequency of growing
+/// the reservation when updating the refiner memory reservation.
+const REFINER_RESERVATION_PREALLOC_SIZE: usize = 10 * 1024 * 1024; // 10MB
+
+/// Builder for constructing a SpatialIndex from geometry batches.
+///
+/// This builder handles:
+/// 1. Accumulating geometry batches to be indexed
+/// 2. Building the spatial R-tree index
+/// 3. Setting up memory tracking and visited bitmaps
+/// 4. Configuring prepared geometries based on execution mode
+pub(crate) struct SpatialIndexBuilder {
+ schema: SchemaRef,
+ spatial_predicate: SpatialPredicate,
+ options: SpatialJoinOptions,
+ join_type: JoinType,
+ probe_threads_count: usize,
+ metrics: SpatialJoinBuildMetrics,
+
+ /// Batches to be indexed
+ indexed_batches: Vec<BuildSideBatch>,
+ /// Memory reservation for tracking the memory usage of the spatial index
+ reservation: MemoryReservation,
+
+ /// Statistics for indexed geometries
+ stats: GeoStatistics,
+
+ /// Memory pool for managing the memory usage of the spatial index
+ memory_pool: Arc<dyn MemoryPool>,
+}
+
+/// Metrics for the build phase of the spatial join.
+#[derive(Clone, Debug, Default)]
+pub(crate) struct SpatialJoinBuildMetrics {
+ /// Total time for collecting build-side of join
+ pub(crate) build_time: metrics::Time,
+ /// Memory used by the spatial-index in bytes
+ pub(crate) build_mem_used: metrics::Gauge,
+}
+
+impl SpatialJoinBuildMetrics {
+ pub fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
+ Self {
+ build_time: MetricBuilder::new(metrics).subset_time("build_time", partition),
+ build_mem_used: MetricBuilder::new(metrics).gauge("build_mem_used", partition),
+ }
+ }
+}
+
+impl SpatialIndexBuilder {
+ /// Create a new builder with the given configuration.
+ pub fn new(
Review Comment:
I decided to leave it as-is. Two fields of it need to be initialized with default values. I'd like to keep this new method to initialize the fields consistently.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2501695361
##########
rust/sedona-spatial-join/src/index/spatial_index_builder.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::BooleanBufferBuilder;
+use arrow_schema::SchemaRef;
+use datafusion_physical_plan::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder};
+use sedona_common::SpatialJoinOptions;
+use sedona_expr::statistics::GeoStatistics;
+
+use datafusion_common::{utils::proxy::VecAllocExt, Result};
+use datafusion_execution::memory_pool::{MemoryConsumer, MemoryPool, MemoryReservation};
+use datafusion_expr::JoinType;
+use futures::StreamExt;
+use geo_index::rtree::{sort::HilbertSort, RTree, RTreeBuilder};
+use parking_lot::Mutex;
+use std::sync::{atomic::AtomicUsize, Arc};
+
+use crate::{
+ collect::{BuildPartition, BuildSideBatch},
+ index::{knn_adapter::KnnComponents, spatial_index::SpatialIndex},
+ operand_evaluator::create_operand_evaluator,
+ refine::create_refiner,
+ spatial_predicate::SpatialPredicate,
+ utils::{
+ concurrent_reservation::ConcurrentReservation, join_utils::need_produce_result_in_final,
+ },
+};
+
+// Type aliases for better readability
+type SpatialRTree = RTree<f32>;
+type DataIdToBatchPos = Vec<(i32, i32)>;
+type RTreeBuildResult = (SpatialRTree, DataIdToBatchPos);
+
+/// Rough estimate for in-memory size of the rtree per rect in bytes
+const RTREE_MEMORY_ESTIMATE_PER_RECT: usize = 60;
+
+/// The prealloc size for the refiner reservation. This is used to reduce the frequency of growing
+/// the reservation when updating the refiner memory reservation.
+const REFINER_RESERVATION_PREALLOC_SIZE: usize = 10 * 1024 * 1024; // 10MB
+
+/// Builder for constructing a SpatialIndex from geometry batches.
+///
+/// This builder handles:
+/// 1. Accumulating geometry batches to be indexed
+/// 2. Building the spatial R-tree index
+/// 3. Setting up memory tracking and visited bitmaps
+/// 4. Configuring prepared geometries based on execution mode
+pub(crate) struct SpatialIndexBuilder {
+ schema: SchemaRef,
+ spatial_predicate: SpatialPredicate,
+ options: SpatialJoinOptions,
+ join_type: JoinType,
+ probe_threads_count: usize,
+ metrics: SpatialJoinBuildMetrics,
+
+ /// Batches to be indexed
+ indexed_batches: Vec<BuildSideBatch>,
+ /// Memory reservation for tracking the memory usage of the spatial index
+ reservation: MemoryReservation,
+
+ /// Statistics for indexed geometries
+ stats: GeoStatistics,
+
+ /// Memory pool for managing the memory usage of the spatial index
+ memory_pool: Arc<dyn MemoryPool>,
+}
+
+/// Metrics for the build phase of the spatial join.
+#[derive(Clone, Debug, Default)]
+pub(crate) struct SpatialJoinBuildMetrics {
+ /// Total time for collecting build-side of join
+ pub(crate) build_time: metrics::Time,
+ /// Memory used by the spatial-index in bytes
+ pub(crate) build_mem_used: metrics::Gauge,
+}
+
+impl SpatialJoinBuildMetrics {
+ pub fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
+ Self {
+ build_time: MetricBuilder::new(metrics).subset_time("build_time", partition),
+ build_mem_used: MetricBuilder::new(metrics).gauge("build_mem_used", partition),
+ }
+ }
+}
+
+impl SpatialIndexBuilder {
+ /// Create a new builder with the given configuration.
+ pub fn new(
Review Comment:
I decided to leave it as-is. Two fields of it need to be initialized with default values. I'd like to keep this new method to initialize the fields consistently. I've tried switching to builder pattern. However, applying the builder pattern to a builder also looks strange.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation merged PR #277:
URL: https://github.com/apache/sedona-db/pull/277
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2504253551
##########
python/sedonadb/python/sedonadb/dbapi.py:
##########
@@ -38,7 +38,7 @@ def connect(**kwargs: Mapping[str, Any]) -> "Connection":
>>> con = sedona.dbapi.connect()
>>> with con.cursor() as cur:
- ... cur.execute("SELECT 1 as one")
+ ... _ = cur.execute("SELECT 1 as one")
Review Comment:
Thanks!
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2502546396
##########
python/sedonadb/python/sedonadb/dbapi.py:
##########
@@ -38,7 +38,7 @@ def connect(**kwargs: Mapping[str, Any]) -> "Connection":
>>> con = sedona.dbapi.connect()
>>> with con.cursor() as cur:
- ... cur.execute("SELECT 1 as one")
+ ... _ = cur.execute("SELECT 1 as one")
Review Comment:
Fix a doctest failure:
```
=================================== FAILURES ===================================
_______________________ [doctest] sedonadb.dbapi.connect _______________________
031 DBAPI is experimental.
032
033 Args:
034 kwargs: Extra keyword arguments passed to
035 `adbc_driver_manager.dbapi.Connection()`.
036
037 Examples:
038
039 >>> con = sedona.dbapi.connect()
040 >>> with con.cursor() as cur:
Expected:
[(1,)]
Got:
<adbc_driver_manager.dbapi.Cursor object at 0x7fd4e20c1e80>
[(1,)]
```
This is caused by a recent `adbc_driver_manager.dbapi` update: https://github.com/apache/arrow-adbc/commit/7076bee4fc1f30a49668723bcccb7ea25bb19fc8#diff-aac0b59df381ba9a1f9232bd15d9f12610bb80beb238eb51901febba5fabc066R742
The `execute` method now returns Self to enable chaining. This will print an extra message in interactive mode. We suppress this message by assigning the return value of `execute` to `_`.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2503867372
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -546,24 +543,21 @@ impl SpatialJoinExec {
let num_partitions = build_side.output_partitioning().partition_count();
let mut build_streams = Vec::with_capacity(num_partitions);
- let mut build_metrics = Vec::with_capacity(num_partitions);
for k in 0..num_partitions {
let stream = build_side.execute(k, Arc::clone(&context))?;
build_streams.push(stream);
- build_metrics.push(SpatialJoinBuildMetrics::new(k, &self.metrics));
}
let probe_thread_count = self.right.output_partitioning().partition_count();
Review Comment:
```suggestion
let probe_thread_count = probe_plan.output_partitioning().partition_count();
```
Isn't this more correct ?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2503967394
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -546,24 +543,21 @@ impl SpatialJoinExec {
let num_partitions = build_side.output_partitioning().partition_count();
let mut build_streams = Vec::with_capacity(num_partitions);
- let mut build_metrics = Vec::with_capacity(num_partitions);
for k in 0..num_partitions {
let stream = build_side.execute(k, Arc::clone(&context))?;
build_streams.push(stream);
- build_metrics.push(SpatialJoinBuildMetrics::new(k, &self.metrics));
}
let probe_thread_count = self.right.output_partitioning().partition_count();
Review Comment:
Thank you for catching this.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2504257816
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -546,24 +543,21 @@ impl SpatialJoinExec {
let num_partitions = build_side.output_partitioning().partition_count();
let mut build_streams = Vec::with_capacity(num_partitions);
- let mut build_metrics = Vec::with_capacity(num_partitions);
for k in 0..num_partitions {
let stream = build_side.execute(k, Arc::clone(&context))?;
build_streams.push(stream);
- build_metrics.push(SpatialJoinBuildMetrics::new(k, &self.metrics));
}
let probe_thread_count = self.right.output_partitioning().partition_count();
Review Comment:
> We also need to revise the correctness of compute_properties of SpatialJoinExec as well.
Should we file an issue?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2504817389
##########
rust/sedona-spatial-join/src/exec.rs:
##########
@@ -546,24 +543,21 @@ impl SpatialJoinExec {
let num_partitions = build_side.output_partitioning().partition_count();
let mut build_streams = Vec::with_capacity(num_partitions);
- let mut build_metrics = Vec::with_capacity(num_partitions);
for k in 0..num_partitions {
let stream = build_side.execute(k, Arc::clone(&context))?;
build_streams.push(stream);
- build_metrics.push(SpatialJoinBuildMetrics::new(k, &self.metrics));
}
let probe_thread_count = self.right.output_partitioning().partition_count();
Review Comment:
I filed https://github.com/apache/sedona-db/issues/282 to track this issue.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: Refactor spatial join code for easier implementing new features [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #277:
URL: https://github.com/apache/sedona-db/pull/277#discussion_r2499751997
##########
rust/sedona-spatial-join/src/refine/geos.rs:
##########
@@ -321,6 +319,12 @@ impl GeosRefiner {
}
}
+fn estimate_prep_geom_in_mem_size(wkb: &Wkb<'_>) -> usize {
+ // TODO: This is a rough estimate of the memory usage of the prepared geometry and
+ // may not be accurate.
Review Comment:
Let's track this if it is not considered in this PR:
```suggestion
// TODO: This is a rough estimate of the memory usage of the prepared geometry and
// may not be accurate.
// https://github.com/apache/sedona-db/issues/xxx
```
##########
rust/sedona-spatial-join/src/index/spatial_index.rs:
##########
@@ -0,0 +1,1487 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::{
+ atomic::{AtomicUsize, Ordering},
+ Arc,
+};
+
+use arrow_array::RecordBatch;
+use arrow_schema::SchemaRef;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::{MemoryPool, MemoryReservation};
+use geo_index::rtree::distance::{DistanceMetric, GeometryAccessor};
+use geo_index::rtree::{sort::HilbertSort, RTree, RTreeBuilder, RTreeIndex};
+use geo_index::IndexableNum;
+use geo_types::{Point, Rect};
+use parking_lot::Mutex;
+use sedona_expr::statistics::GeoStatistics;
+use sedona_geo::to_geo::item_to_geometry;
+use sedona_geo_generic_alg::algorithm::Centroid;
+use wkb::reader::Wkb;
+
+use crate::{
+ collect::BuildSideBatch,
+ index::{
+ knn_adapter::{KnnComponents, SedonaKnnAdapter},
+ IndexQueryResult, QueryResultMetrics,
+ },
+ operand_evaluator::{create_operand_evaluator, OperandEvaluator},
+ refine::{create_refiner, IndexQueryResultRefiner},
+ spatial_predicate::SpatialPredicate,
+ utils::concurrent_reservation::ConcurrentReservation,
+};
+use arrow::array::BooleanBufferBuilder;
+use sedona_common::{option::SpatialJoinOptions, ExecutionMode};
+
+pub(crate) struct SpatialIndex {
+ schema: SchemaRef,
+
+ /// The spatial predicate evaluator for the spatial predicate.
+ evaluator: Arc<dyn OperandEvaluator>,
+
+ /// The refiner for refining the index query results.
+ refiner: Arc<dyn IndexQueryResultRefiner>,
+
+ /// Memory reservation for tracking the memory usage of the refiner
+ refiner_reservation: ConcurrentReservation,
+
+ /// R-tree index for the geometry batches. It takes MBRs as query windows and returns
+ /// data indexes. These data indexes should be translated using `data_id_to_batch_pos` to get
+ /// the original geometry batch index and row index, or translated using `prepared_geom_idx_vec`
+ /// to get the prepared geometries array index.
+ rtree: RTree<f32>,
+
+ /// Indexed batches containing evaluated geometry arrays. It contains the original record
+ /// batches and geometry arrays obtained by evaluating the geometry expression on the build side.
+ indexed_batches: Vec<BuildSideBatch>,
+ /// An array for translating rtree data index to geometry batch index and row index
+ data_id_to_batch_pos: Vec<(i32, i32)>,
+
+ /// An array for translating rtree data index to consecutive index. Each geometry may be indexed by
+ /// multiple boxes, so there could be multiple data indexes for the same geometry. A mapping for
+ /// squashing the index makes it easier for persisting per-geometry auxiliary data for evaluating
+ /// the spatial predicate. This is extensively used by the spatial predicate evaluators for storing
+ /// prepared geometries.
+ geom_idx_vec: Vec<usize>,
+
+ /// Shared bitmap builders for visited left indices, one per batch
+ visited_left_side: Option<Mutex<Vec<BooleanBufferBuilder>>>,
+
+ /// Counter of running probe-threads, potentially able to update `bitmap`.
+ /// Each time a probe thread finished probing the index, it will decrement the counter.
+ /// The last finished probe thread will produce the extra output batches for unmatched
+ /// build side when running left-outer joins. See also [`report_probe_completed`].
+ probe_threads_counter: AtomicUsize,
+
+ /// Shared KNN components (distance metrics and geometry cache) for efficient KNN queries
+ knn_components: KnnComponents,
+
+ /// Memory reservation for tracking the memory usage of the spatial index
+ /// Cleared on `SpatialIndex` drop
+ #[expect(dead_code)]
+ reservation: MemoryReservation,
+}
+
+impl SpatialIndex {
+ pub fn empty(
+ spatial_predicate: SpatialPredicate,
+ schema: SchemaRef,
+ options: SpatialJoinOptions,
+ probe_threads_counter: AtomicUsize,
+ mut reservation: MemoryReservation,
+ memory_pool: Arc<dyn MemoryPool>,
+ ) -> Self {
+ let evaluator = create_operand_evaluator(&spatial_predicate, options.clone());
+ let refiner = create_refiner(
+ options.spatial_library,
+ &spatial_predicate,
+ options.clone(),
+ 0,
+ GeoStatistics::empty(),
+ );
+ let refiner_reservation = reservation.split(0);
+ let refiner_reservation = ConcurrentReservation::try_new(0, refiner_reservation).unwrap();
+ let rtree = RTreeBuilder::<f32>::new(0).finish::<HilbertSort>();
+ Self {
+ schema,
+ evaluator,
+ refiner,
+ refiner_reservation,
+ rtree,
+ data_id_to_batch_pos: Vec::new(),
+ indexed_batches: Vec::new(),
+ geom_idx_vec: Vec::new(),
+ visited_left_side: None,
+ probe_threads_counter,
+ knn_components: KnnComponents::new(0, &[], memory_pool.clone()).unwrap(), // Empty index has no cache
+ reservation,
+ }
+ }
+
+ #[allow(clippy::too_many_arguments)]
+ pub fn new(
Review Comment:
It probably makes sense just to drop `new()` at this point (to force construction with named arguments)
##########
rust/sedona-spatial-join/src/index/spatial_index_builder.rs:
##########
@@ -0,0 +1,304 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::BooleanBufferBuilder;
+use arrow_schema::SchemaRef;
+use datafusion_physical_plan::metrics::{self, ExecutionPlanMetricsSet, MetricBuilder};
+use sedona_common::SpatialJoinOptions;
+use sedona_expr::statistics::GeoStatistics;
+
+use datafusion_common::{utils::proxy::VecAllocExt, Result};
+use datafusion_execution::memory_pool::{MemoryConsumer, MemoryPool, MemoryReservation};
+use datafusion_expr::JoinType;
+use futures::StreamExt;
+use geo_index::rtree::{sort::HilbertSort, RTree, RTreeBuilder};
+use parking_lot::Mutex;
+use std::sync::{atomic::AtomicUsize, Arc};
+
+use crate::{
+ collect::{BuildPartition, BuildSideBatch},
+ index::{knn_adapter::KnnComponents, spatial_index::SpatialIndex},
+ operand_evaluator::create_operand_evaluator,
+ refine::create_refiner,
+ spatial_predicate::SpatialPredicate,
+ utils::{
+ concurrent_reservation::ConcurrentReservation, join_utils::need_produce_result_in_final,
+ },
+};
+
+// Type aliases for better readability
+type SpatialRTree = RTree<f32>;
+type DataIdToBatchPos = Vec<(i32, i32)>;
+type RTreeBuildResult = (SpatialRTree, DataIdToBatchPos);
+
+/// Rough estimate for in-memory size of the rtree per rect in bytes
+const RTREE_MEMORY_ESTIMATE_PER_RECT: usize = 60;
+
+/// The prealloc size for the refiner reservation. This is used to reduce the frequency of growing
+/// the reservation when updating the refiner memory reservation.
+const REFINER_RESERVATION_PREALLOC_SIZE: usize = 10 * 1024 * 1024; // 10MB
+
+/// Builder for constructing a SpatialIndex from geometry batches.
+///
+/// This builder handles:
+/// 1. Accumulating geometry batches to be indexed
+/// 2. Building the spatial R-tree index
+/// 3. Setting up memory tracking and visited bitmaps
+/// 4. Configuring prepared geometries based on execution mode
+pub(crate) struct SpatialIndexBuilder {
+ schema: SchemaRef,
+ spatial_predicate: SpatialPredicate,
+ options: SpatialJoinOptions,
+ join_type: JoinType,
+ probe_threads_count: usize,
+ metrics: SpatialJoinBuildMetrics,
+
+ /// Batches to be indexed
+ indexed_batches: Vec<BuildSideBatch>,
+ /// Memory reservation for tracking the memory usage of the spatial index
+ reservation: MemoryReservation,
+
+ /// Statistics for indexed geometries
+ stats: GeoStatistics,
+
+ /// Memory pool for managing the memory usage of the spatial index
+ memory_pool: Arc<dyn MemoryPool>,
+}
+
+/// Metrics for the build phase of the spatial join.
+#[derive(Clone, Debug, Default)]
+pub(crate) struct SpatialJoinBuildMetrics {
+ /// Total time for collecting build-side of join
+ pub(crate) build_time: metrics::Time,
+ /// Memory used by the spatial-index in bytes
+ pub(crate) build_mem_used: metrics::Gauge,
+}
+
+impl SpatialJoinBuildMetrics {
+ pub fn new(partition: usize, metrics: &ExecutionPlanMetricsSet) -> Self {
+ Self {
+ build_time: MetricBuilder::new(metrics).subset_time("build_time", partition),
+ build_mem_used: MetricBuilder::new(metrics).gauge("build_mem_used", partition),
+ }
+ }
+}
+
+impl SpatialIndexBuilder {
+ /// Create a new builder with the given configuration.
+ pub fn new(
Review Comment:
This is also probably a difficult `new()` call to read
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org