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 2026/03/03 10:32:15 UTC
[PR] fix(spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Kontinuation opened a new pull request, #677:
URL: https://github.com/apache/sedona-db/pull/677
## Summary
- Introduce `ProbeShuffleExec`, a thin wrapper around `RepartitionExec` that is opaque to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes, preventing them from stripping the round-robin repartitioning that the spatial join planner inserts on the probe side.
- Move the `ProbeShuffleExec` presence assertion into `run_spatial_join_query` so it runs on every optimized spatial join integration test (~174 cases) rather than only in standalone tests.
## Problem
DataFusion's optimizer passes recognize `RepartitionExec` nodes and can strip or replace them. When the spatial join planner inserts a `RepartitionExec(RoundRobinBatch)` on the probe side to ensure proper parallelism, these optimizer passes could remove it, causing the probe side to lose its intended repartitioning.
## Solution
`ProbeShuffleExec` wraps a `RepartitionExec` internally but presents itself as a different `ExecutionPlan` type. Since the optimizer passes only look for `RepartitionExec` nodes by type, they leave `ProbeShuffleExec` untouched. The wrapper delegates all `ExecutionPlan` trait methods to the inner `RepartitionExec`.
--
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] fix(rust/sedona-spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on PR #677:
URL: https://github.com/apache/sedona-db/pull/677#issuecomment-3997094072
It applies to all the test cases, the original implementation didn't work and I have not added assertions to verify that RepartitionExec should be present on the probe side. This patch updated the common test method for running spatial join to verify that `ProbeShuffleExec` presents in every spatial join queries.
`ProbeShuffleExec` has significant impact on the the CPU utilization of SpatialBench Q10 and Q11. I see 10% performance improvement for non-spilled runs and 70%+ for spilled runs.
--
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] fix(rust/sedona-spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #677:
URL: https://github.com/apache/sedona-db/pull/677#discussion_r2878488004
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
+/// A round-robin repartitioning node that is invisible to DataFusion's
+/// physical optimizer passes.
+///
+/// See [module-level documentation](self) for motivation and design.
+#[derive(Debug)]
+pub struct ProbeShuffleExec {
+ inner_repartition: RepartitionExec,
+}
+
+impl ProbeShuffleExec {
+ /// Create a new [`ProbeShuffleExec`] that round-robin repartitions `input`
+ /// into the same number of output partitions as `input`. This will ensure
+ /// that the probe workload of a spatial join will be evenly distributed.
+ /// More importantly, shuffled probe side data will be less likely to
+ /// cause skew issues when out-of-core, spatial partitioned spatial join is enabled,
+ /// especially when the input probe data is sorted by their spatial locations.
+ pub fn try_new(input: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let num_partitions = input.output_partitioning().partition_count();
+ let inner_repartition = RepartitionExec::try_new(
+ Arc::clone(&input),
+ Partitioning::RoundRobinBatch(num_partitions),
+ )?;
+ Ok(Self { inner_repartition })
+ }
+
+ /// Try to wrap the given [`RepartitionExec`] `plan` with [`ProbeShuffleExec`].
+ pub fn try_wrap_repartition(plan: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let Some(repartition_exec) = plan.as_any().downcast_ref::<RepartitionExec>() else {
+ return plan_err!(
+ "ProbeShuffleExec can only wrap RepartitionExec, but got {}",
+ plan.name()
+ );
+ };
+ Ok(Self {
+ inner_repartition: repartition_exec.clone(),
+ })
+ }
+
+ /// Number of output partitions.
+ pub fn num_partitions(&self) -> usize {
+ self.inner_repartition
+ .properties()
+ .output_partitioning()
+ .partition_count()
+ }
+}
+
+impl DisplayAs for ProbeShuffleExec {
+ fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result {
+ match t {
+ DisplayFormatType::Default | DisplayFormatType::Verbose => {
+ write!(
+ f,
+ "ProbeShuffleExec: partitioning=RoundRobinBatch({})",
+ self.num_partitions()
+ )
+ }
+ DisplayFormatType::TreeRender => {
+ write!(f, "partitioning=RoundRobinBatch({})", self.num_partitions())
+ }
+ }
+ }
+}
+
+impl ExecutionPlan for ProbeShuffleExec {
+ fn name(&self) -> &str {
+ "ProbeShuffleExec"
+ }
+
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn properties(&self) -> &PlanProperties {
+ self.inner_repartition.properties()
+ }
+
+ fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>> {
+ vec![&self.inner_repartition.input()]
Review Comment:
Good idea. Updated.
--
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] fix(spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #677:
URL: https://github.com/apache/sedona-db/pull/677#discussion_r2878244388
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
+/// A round-robin repartitioning node that is invisible to DataFusion's
+/// physical optimizer passes.
+///
+/// See [module-level documentation](self) for motivation and design.
+#[derive(Debug)]
+pub struct ProbeShuffleExec {
+ inner_repartition: RepartitionExec,
+}
+
+impl ProbeShuffleExec {
+ /// Create a new [`ProbeShuffleExec`] that round-robin repartitions `input`
+ /// into the same number of output partitions as `input`. This will ensure
+ /// that the probe workload of a spatial join will be evenly distributed.
+ /// More importantly, shuffled probe side data will be less likely to
+ /// cause skew issues when out-of-core, spatial partitioned spatial join is enabled,
+ /// especially when the input probe data is sorted by their spatial locations.
+ pub fn try_new(input: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let num_partitions = input.output_partitioning().partition_count();
+ let inner_repartition = RepartitionExec::try_new(
+ Arc::clone(&input),
+ Partitioning::RoundRobinBatch(num_partitions),
+ )?;
+ Ok(Self { inner_repartition })
+ }
+
+ /// Try to wrap the given [`RepartitionExec`] `plan` with [`ProbeShuffleExec`].
+ pub fn try_wrap_repartition(plan: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let Some(repartition_exec) = plan.as_any().downcast_ref::<RepartitionExec>() else {
+ return plan_err!(
+ "ProbeShuffleExec can only wrap RepartitionExec, but got {}",
+ plan.name()
+ );
+ };
+ Ok(Self {
+ inner_repartition: repartition_exec.clone(),
+ })
+ }
+
+ /// Number of output partitions.
+ pub fn num_partitions(&self) -> usize {
+ self.inner_repartition
+ .properties()
+ .output_partitioning()
+ .partition_count()
+ }
+}
+
+impl DisplayAs for ProbeShuffleExec {
+ fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result {
+ match t {
+ DisplayFormatType::Default | DisplayFormatType::Verbose => {
+ write!(
+ f,
+ "ProbeShuffleExec: partitioning=RoundRobinBatch({})",
+ self.num_partitions()
+ )
+ }
+ DisplayFormatType::TreeRender => {
+ write!(f, "partitioning=RoundRobinBatch({})", self.num_partitions())
+ }
+ }
+ }
+}
+
+impl ExecutionPlan for ProbeShuffleExec {
+ fn name(&self) -> &str {
+ "ProbeShuffleExec"
+ }
+
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn properties(&self) -> &PlanProperties {
+ self.inner_repartition.properties()
+ }
+
+ fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>> {
+ vec![&self.inner_repartition.input()]
Review Comment:
`children()` currently returns `vec![&self.inner_repartition.input()]`. This is very likely incorrect for DataFusion execs: `input()` typically returns `&Arc<dyn ExecutionPlan>`, so taking an extra `&` produces `&&Arc<_>` and won’t type-check (or, if `input()` returns an `Arc`, this would create a reference to a temporary). Adjust this to return a `&Arc<dyn ExecutionPlan>` directly, consistent with other `ExecutionPlan::children()` impls in this crate (e.g., `SpatialJoinExec`).
```suggestion
vec![self.inner_repartition.input()]
```
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
Review Comment:
`ExecutionPlanProperties` is imported but not used. Dropping the unused import will keep the crate warning/Clippy output clean.
```suggestion
DisplayAs, DisplayFormatType, ExecutionPlan, Partitioning, PlanProperties,
};
```
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
+/// A round-robin repartitioning node that is invisible to DataFusion's
+/// physical optimizer passes.
+///
+/// See [module-level documentation](self) for motivation and design.
+#[derive(Debug)]
+pub struct ProbeShuffleExec {
+ inner_repartition: RepartitionExec,
+}
+
+impl ProbeShuffleExec {
+ /// Create a new [`ProbeShuffleExec`] that round-robin repartitions `input`
+ /// into the same number of output partitions as `input`. This will ensure
+ /// that the probe workload of a spatial join will be evenly distributed.
+ /// More importantly, shuffled probe side data will be less likely to
+ /// cause skew issues when out-of-core, spatial partitioned spatial join is enabled,
+ /// especially when the input probe data is sorted by their spatial locations.
+ pub fn try_new(input: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let num_partitions = input.output_partitioning().partition_count();
+ let inner_repartition = RepartitionExec::try_new(
+ Arc::clone(&input),
+ Partitioning::RoundRobinBatch(num_partitions),
+ )?;
+ Ok(Self { inner_repartition })
+ }
+
+ /// Try to wrap the given [`RepartitionExec`] `plan` with [`ProbeShuffleExec`].
+ pub fn try_wrap_repartition(plan: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let Some(repartition_exec) = plan.as_any().downcast_ref::<RepartitionExec>() else {
+ return plan_err!(
+ "ProbeShuffleExec can only wrap RepartitionExec, but got {}",
+ plan.name()
+ );
+ };
+ Ok(Self {
+ inner_repartition: repartition_exec.clone(),
+ })
+ }
+
+ /// Number of output partitions.
+ pub fn num_partitions(&self) -> usize {
+ self.inner_repartition
+ .properties()
+ .output_partitioning()
+ .partition_count()
+ }
+}
+
+impl DisplayAs for ProbeShuffleExec {
+ fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result {
+ match t {
+ DisplayFormatType::Default | DisplayFormatType::Verbose => {
+ write!(
+ f,
+ "ProbeShuffleExec: partitioning=RoundRobinBatch({})",
+ self.num_partitions()
+ )
+ }
+ DisplayFormatType::TreeRender => {
+ write!(f, "partitioning=RoundRobinBatch({})", self.num_partitions())
+ }
+ }
+ }
+}
+
+impl ExecutionPlan for ProbeShuffleExec {
+ fn name(&self) -> &str {
+ "ProbeShuffleExec"
+ }
+
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn properties(&self) -> &PlanProperties {
+ self.inner_repartition.properties()
+ }
+
+ fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>> {
+ vec![&self.inner_repartition.input()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ mut children: Vec<Arc<dyn ExecutionPlan>>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ if children.len() != 1 {
+ return internal_err!(
+ "ProbeShuffleExec expects exactly 1 child, got {}",
+ children.len()
+ );
+ }
+ let child = children.remove(0);
+ Ok(Arc::new(Self::try_new(child)?))
Review Comment:
`with_new_children()` rebuilds the wrapper via `Self::try_new(child)`, which recomputes the output partition count from the *new child’s* partitioning. If this `ProbeShuffleExec` was created via `try_wrap_repartition()` or via `repartitioned(target_partitions, ...)`, this will silently drop the existing target partition count and change the plan’s partitioning. Consider reconstructing using the existing `self.num_partitions()` (or storing the intended partition count in the struct) so `with_new_children()` preserves this node’s semantics.
```suggestion
let new_inner = self.inner_repartition.with_new_children(vec![child])?;
let new_plan = Self::try_wrap_repartition(new_inner)?;
Ok(Arc::new(new_plan))
```
--
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] fix(rust/sedona-spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #677:
URL: https://github.com/apache/sedona-db/pull/677#discussion_r2878494631
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
+/// A round-robin repartitioning node that is invisible to DataFusion's
+/// physical optimizer passes.
+///
+/// See [module-level documentation](self) for motivation and design.
+#[derive(Debug)]
+pub struct ProbeShuffleExec {
+ inner_repartition: RepartitionExec,
+}
+
+impl ProbeShuffleExec {
+ /// Create a new [`ProbeShuffleExec`] that round-robin repartitions `input`
+ /// into the same number of output partitions as `input`. This will ensure
+ /// that the probe workload of a spatial join will be evenly distributed.
+ /// More importantly, shuffled probe side data will be less likely to
+ /// cause skew issues when out-of-core, spatial partitioned spatial join is enabled,
+ /// especially when the input probe data is sorted by their spatial locations.
+ pub fn try_new(input: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let num_partitions = input.output_partitioning().partition_count();
+ let inner_repartition = RepartitionExec::try_new(
+ Arc::clone(&input),
+ Partitioning::RoundRobinBatch(num_partitions),
+ )?;
+ Ok(Self { inner_repartition })
+ }
+
+ /// Try to wrap the given [`RepartitionExec`] `plan` with [`ProbeShuffleExec`].
+ pub fn try_wrap_repartition(plan: Arc<dyn ExecutionPlan>) -> Result<Self> {
+ let Some(repartition_exec) = plan.as_any().downcast_ref::<RepartitionExec>() else {
+ return plan_err!(
+ "ProbeShuffleExec can only wrap RepartitionExec, but got {}",
+ plan.name()
+ );
+ };
+ Ok(Self {
+ inner_repartition: repartition_exec.clone(),
+ })
+ }
+
+ /// Number of output partitions.
+ pub fn num_partitions(&self) -> usize {
+ self.inner_repartition
+ .properties()
+ .output_partitioning()
+ .partition_count()
+ }
+}
+
+impl DisplayAs for ProbeShuffleExec {
+ fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result {
+ match t {
+ DisplayFormatType::Default | DisplayFormatType::Verbose => {
+ write!(
+ f,
+ "ProbeShuffleExec: partitioning=RoundRobinBatch({})",
+ self.num_partitions()
+ )
+ }
+ DisplayFormatType::TreeRender => {
+ write!(f, "partitioning=RoundRobinBatch({})", self.num_partitions())
+ }
+ }
+ }
+}
+
+impl ExecutionPlan for ProbeShuffleExec {
+ fn name(&self) -> &str {
+ "ProbeShuffleExec"
+ }
+
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn properties(&self) -> &PlanProperties {
+ self.inner_repartition.properties()
+ }
+
+ fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>> {
+ vec![&self.inner_repartition.input()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ mut children: Vec<Arc<dyn ExecutionPlan>>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ if children.len() != 1 {
+ return internal_err!(
+ "ProbeShuffleExec expects exactly 1 child, got {}",
+ children.len()
+ );
+ }
+ let child = children.remove(0);
+ Ok(Arc::new(Self::try_new(child)?))
Review Comment:
We'd better not do this. We want to make sure that the initial number of partitions be consistent with the child.
--
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] fix(rust/sedona-spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #677:
URL: https://github.com/apache/sedona-db/pull/677
--
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] fix(rust/sedona-spatial-join): wrap probe-side repartition in ProbeShuffleExec to prevent optimizer stripping [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #677:
URL: https://github.com/apache/sedona-db/pull/677#discussion_r2878485149
##########
rust/sedona-spatial-join/src/planner/probe_shuffle_exec.rs:
##########
@@ -0,0 +1,225 @@
+// 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.
+
+//! [`ProbeShuffleExec`] — a round-robin repartitioning wrapper that is invisible
+//! to DataFusion's `EnforceDistribution` / `EnforceSorting` optimizer passes.
+//!
+//! Those passes unconditionally strip every [`RepartitionExec`] before
+//! re-evaluating distribution requirements. Because `SpatialJoinExec` reports
+//! `UnspecifiedDistribution` for its inputs, a bare `RepartitionExec` that was
+//! inserted by the extension planner is removed and never re-added.
+//!
+//! `ProbeShuffleExec` wraps a hidden, internal `RepartitionExec` so that:
+//! * **Optimizer passes** see an opaque node (not a `RepartitionExec`) and leave
+//! it alone.
+//! * **`children()` / `with_new_children()`** expose the *original* input so
+//! the rest of the optimizer tree can still be rewritten normally.
+//! * **`execute()`** delegates to the internal `RepartitionExec` which performs
+//! the actual round-robin shuffle.
+
+use std::any::Any;
+use std::fmt;
+use std::sync::Arc;
+
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{internal_err, plan_err, Result, Statistics};
+use datafusion_execution::{SendableRecordBatchStream, TaskContext};
+use datafusion_physical_expr::PhysicalExpr;
+use datafusion_physical_plan::execution_plan::CardinalityEffect;
+use datafusion_physical_plan::filter_pushdown::{
+ ChildPushdownResult, FilterDescription, FilterPushdownPhase, FilterPushdownPropagation,
+};
+use datafusion_physical_plan::metrics::MetricsSet;
+use datafusion_physical_plan::projection::ProjectionExec;
+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::{
+ DisplayAs, DisplayFormatType, ExecutionPlan, ExecutionPlanProperties, Partitioning,
+ PlanProperties,
+};
+
Review Comment:
This is incorrect. Calling `output_partitioning` method needs to import this trait.
--
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