Skip to content

Commit f7f59c5

Browse files
refactor(core): replace PerPartitionFilterExec with RangeFilterExec
PerPartitionFilterExec's only caller (BallistaAdapter above ShuffleReader for hash-agg correctness) was already using it as a range-shaped filter. Widening it into a general per-partition arbitrary-predicate op — with halo-widening bolted on for the parallel-window rewrite — would leak a range concept into an arbitrary-predicate contract. RangeFilterExec is the honest shape: routing_expr + cuts + halo_lo / halo_hi. Per-partition semantics fall out of the local partition index, not from a Vec of independent predicates. Ordering knowledge on the input opens the door to a future ValueIndexReader-driven binary-search path (PR #2204 direction) that a generic FilterExec can't take. Notable pieces: - `cuts: Arc<Mutex<Option<Vec<f64>>>>` + `resolve_cuts` API mirror `ExchangeExec::range_repartition_routing()` — the ParallelWindow rule plants a pending RangeFilterExec at plan time; the scheduler resolves cuts after stage 0's RuntimeStatsExec reports merge. `execute` and serialization both refuse while cuts are unresolved. - `partition_indices: Vec<usize>` maps local → global partition index. Restrict slices this mapping without touching cuts (cuts stay whole; they describe the K global partitions). Replaces PPFE's per-partition predicate-vec slicing in task_builder's restrict path. - Public API + proto speak `ScalarValue` (not `f64`) per the type- generality rule for the range-repartition family: the outer contract is type-agnostic so KLL can widen internal storage later without an API break. Internal downcast to `f64` today; non-Float64 inputs error with a clear message. - Adapter builds RangeFilterExec with `halo=0` for the existing hash-agg case; the parallel-window rule will build it with non-zero halo. Migration: delete `PerPartitionFilterExec`, migrate all callers, rename proto `PerPartitionFilterExecNode` → `RangeFilterExecNode`, update doc comments. Full test suite (597 tests) passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> feat(scheduler): ParallelWindowRule — distributed range-shuffle for BWAG Adds `ParallelWindowRule` to the AQE default_optimizers chain (position 2, before `SelectJoinRule` and DF's own optimizers, before `DistributedExchangeRule`). The rule matches bounded RANGE-frame windows with no PARTITION BY and a single-column Float64 ORDER BY, and rewrites them into a range-shuffle so BoundedWindowAggExec's SinglePartition requirement isn't a serial bottleneck. Shape: RangeFilterExec (narrow, halo=0, cuts=pending) BoundedWindowAggExec SortPreservingMergeExec RangeFilterExec (wide, halo=frame bounds, cuts=pending) RuntimeStatsExec (post-ORRE per-partition sketch → scheduler) OrderedRangeRepartitionExec (K sorted disjoint outputs) RuntimeStatsExec (local sketch; feeds ORRE's cut walker) SortExec (preserve_partitioning=true) <source> Both RangeFilterExecs are planted with cuts=None. After stage 0's tasks complete and their RSE reports are merged into K-1 quantile cuts, the scheduler's `resolve_range_filter_cuts` walker (in `adapt_to_ballista`) finds every pending RangeFilterExec in the downstream stage's plan and resolves it against the matching ExchangeExec's routing_expr. Adapter no longer injects RangeFilterExec — the rule is the sole planter, single source of truth. Idempotency guard on the rule bails when the BWAG's subtree already contains our own ORRE/RangeFilter (AQE re-plans fire the chain again on the already-rewritten plan). Also relaxes `ORRE::try_new` — the child-claims-sortedness check moved from construction to `execute()`. Rule-time construction races with `EnforceSorting` (which planted a SortExec on ORRE's declared `required_input_ordering` *after* the rule ran), so refusing at try_new was too strict. The runtime check at execute() still catches invariant breaks; two tests moved from `try_new_rejects_*` to `execute_rejects_*`. Verified on h2o Q8 at 1e7 under a 2G/exec cgroup cap: stage 0 (8 tasks) and stage 1 (8 tasks) both parallelize across both executors, no OOM. Stage 2 still collapses to a single task doing SPM+BWAG+narrow — DE inserts a shuffle boundary below the SPM, putting BWAG in the final stage. That collapse is the next follow-up; the machinery for the range-shuffle itself is in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> fix(scheduler): stop DE inserting Exchange between SPM and rule-planted RangeFilterExec `ParallelWindowRule` plants a `RangeFilterExec` directly on the resolved range-repartition `ExchangeExec`, with `SortPreservingMergeExec` above. `DistributedExchangeRule`'s SPM branch was checking whether SPM's immediate child was an `ExchangeExec` — seeing the `RangeFilterExec`, it injected another `ExchangeExec`, cutting the plan into an extra collapse stage. Introduce `is_stage_boundary` and treat a `RangeFilterExec` sitting directly on an `ExchangeExec` as part of the boundary. That is a conscious design shape — we chose not to fold range-filtering into `ShuffleReader`/`ExchangeExec`, so the filter is part of the boundary by construction. This matches the pre-`fcb31520` behaviour where the adapter injected the filter after DE had already run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> refactor(scheduler): size ParallelWindowRule's K from source partitions K was `config.execution.target_partitions.max(2)` — a placeholder chosen while writing the rule. The natural sizing is `source.output_partitioning().partition_count()`: ORRE re-slices each input partition into a range-disjoint output partition, so K = input partitions is the 1:1 rearrangement. No behaviour change on h2o Q8 (`target_partitions` and source partitions both settle at 8), but the rule no longer depends on the config knob or its `.max(2)` fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> feat(core, scheduler): PartitionedBoundedWindowAggExec — parallel BWAG for the range-window shape DataFusion's `BoundedWindowAggExec` declares `SinglePartition` when no PARTITION BY is present, forcing `EnforceDistribution` to collapse K→1 via `SortPreservingMergeExec`. With `ParallelWindowRule`'s range- repartition upstream, each ORRE output partition is a globally range-disjoint slice + halo — BWAG can safely run per-partition on those K slices and produce K correct outputs. `PartitionedBoundedWindowAggExec` wraps BWAG, exposes only the input as its plan-tree child (BWAG itself is hidden from tree walkers), and overrides `required_input_distribution` to `UnspecifiedDistribution`. `execute(i)` delegates to the wrapped BWAG, which already processes each partition independently. - `ballista_core::execution_plans::partitioned_bounded_window_agg`: the new operator. `InputOrderMode` and `can_repartition` are hardcoded (`Sorted` / `false`) per the rule's shape gates. - `BallistaPhysicalPlanNode::PartitionedBoundedWindowAgg`: proto message carrying only `window_expr` — the rest is implicit from the rule's invariants. Round-trip goes through DF's `serialize_physical_window_expr` / `parse_physical_window_expr`. - `ParallelWindowRule::rewrite_bwag`: drops the SPM the previous rewrite planted between BWAG and the wide `RangeFilterExec`, and swaps BWAG for `PartitionedBoundedWindowAggExec`. K is sourced from `config.execution.target_partitions.max(2)` — at rule-fire time `DataSourceExec` still has 1 file_group (splits happen later in the AQE chain), so the plan tree can't yet tell us the true source width. Reverts the "size K from source" refactor. - `rewrites_q8_shape` test now asserts `PartitionedBoundedWindowAggExec` and NO `SortPreservingMergeExec` in the output. On h2o Q8 @ 1e7 under a 2G/exec cgroup cap, 2 execs × 4 vcores, `ballista.scheduler.max_partitions_per_task=4`: 41 s (down from 155 s) and returns the full 10M rows (previous runs returned only 1.55M — the K→1 collapse dropped ~87% of the output because the narrow `RangeFilterExec` above the collapsed BWAG kept only partition-0's range). Both stages run 2 MPT tasks (one per exec). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> feat(core, scheduler): gate ParallelWindowRule behind ballista.planner.parallel_window.enabled Adds an opt-in config flag so users of AQE don't inherit the range-window rewrite by default. Matches the shape of `ballista.planner.coalesce.enabled`: new AQE rule → new opt-in flag. Default `false`. - `BALLISTA_PARALLEL_WINDOW_ENABLED` + registry entry + getter on `BallistaConfig`. - Guard clause at the top of `ParallelWindowRule::optimize` returns the plan untouched when the flag is off. - Existing shape tests keep the rule enabled through the local `optimize` helper; a new `disabled_by_default` test asserts the rewrite is inert without the extension registered. - Regenerated `docs/source/user-guide/configs.md`. notes feat(core, scheduler, executor): RangeShuffleReaderExec — ordered k-way merge at stage boundary [WIP] Closes the RANGE-frame correctness gap: the regular ShuffleReaderExec concatenates upstream sources in arrival order, breaking the monotonicity BWAG's Range-frame cursor assumes. RangeShuffleReaderExec keeps each source alive as its own stream and feeds them all into StreamingMerge on the child's declared ordering. - new RangeShuffleReaderExec (fetch reuses shuffle_reader helpers, backpressure via merge demand; no permit governor, no per-source buffering) - adapter plants it whenever exchange.input().output_ordering().is_some() - proto + codec round-trip; executor work_dir/client_pool late binding; task_builder partition-slice restriction h2o Q8 @ 1e7 SUM diff: parallel_window=true/false now agree to 5e-14 relative (FP noise floor). Previously diverged at run boundaries. Follow-ups (see next-session TODO): planner.rs::rollback_resolved_shuffles falls through, cluster/mod.rs::stage_has_input_collapse falls through. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> refactor(scheduler): audit-driven RangeShuffleReaderExec whitelist fills Fills two production downcast sites that fell through the RangeShuffleReaderExec shape, plus fmt fallout from the initial slice. - planner::rollback_resolved_shuffles: rolls range readers back to plain UnresolvedShuffleExec. Range-ness is derived at plan time from the child's ordering, so a re-plan's adapter walk re-plants a fresh range reader — no proto extension needed. - cluster::stage_has_input_collapse: range reader is a stage boundary; the walker must stop there, else a single-output-partition range reader spuriously trips the `partition_count == 1` collapse arm. Tests: - rollback_resolved_shuffles_reduces_range_reader_to_plain_unresolved - stage_has_input_collapse_stops_at_range_reader Fmt: adapter's #[cfg(test)] mod moved below resolve_range_filter_cuts to satisfy clippy::items_after_test_module. Follow-up still open: execution_graph_dot.rs graphviz — will render generic node label for the range reader. Diagnostic only, safe to punt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> docs(parallel-range-window): tick Ordered ShuffleReader, drop correctness-gap section The gap is closed — RangeShuffleReaderExec ships in d87321d + c8f3f20, and the h2o Q8 SUM diff between parallel_window=true/false lands at 5e-14 relative (Float64 noise floor). Rewrite the ticked line to describe the landed shape and note the writer-vs-demand-driven follow-up. Drop the "Dot-product check" bullet for the reader (now landed) and the whole correctness-gap section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> perf(core): RangeFilterExec min/max fast paths + binary-search slice When `input.output_ordering()` leads with `routing_expr` ascending, take one of three shortcuts on each batch before `filter_record_batch`: - `last < lo` or `first >= hi` → drop the whole batch (skip). - `first >= lo && last < hi` → pass the batch through unchanged (Arc-clone). - mixed → `partition_point` on the Float64Array values for lo/hi indices + `RecordBatch::slice` (zero-copy view). Nullable routing columns fall back to `filter_record_batch` on a per-batch basis (Float64Array::values() returns garbage for null slots, breaking partition_point). `sorted_on_key` is derived at construction — no config knob. h2o Q8 with 2 execs × 4 vcores × MPT=4: scale cap parallel_window=false parallel_window=true speedup 1e7 2G 7.6 s 2.5 s 3.0× 1e8 4G 143 s 92 s 1.55× The 1e8 delta is smaller because the bottleneck shifts to shuffle IO / whole-file merge memory — the ValueIndex + per-task halo work next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> fix(docs): rustdoc + prettier CI - rustdoc: three `[`resolve_cuts`]` references in `range_filter.rs` (module doc + two item docs) resolved to no target; qualify as `RangeFilterExec::resolve_cuts` / `Self::resolve_cuts` so cargo doc no longer errors on ballista-core. - prettier: `docs/developer/parallel-range-window.md` had two `*emphasis*` spans (`*shape*`, `*task-level*`) and a stray blank line — prettier wants `_emphasis_` + single blank. No content change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> feat(core): RangeFilterExec metrics — fast-path counters + baseline Was returning `None` from `metrics()`, so the operator was invisible in the scheduler's stage-metrics dump. Adds `ExecutionPlanMetricsSet` with `BaselineMetrics` (elapsed_compute, output_rows via record_poll) and five path counters: `fast_skip_batches`, `fast_pass_batches`, `fast_slice_batches`, `slow_batches`, `input_rows`. Timer scoped post-poll so upstream shuffle IO isn't billed to this op. Scout on h2o Q8 @ 1e8 confirms fast path is firing as intended (99%+ pass-through on the narrow filter, 85% skip on the wide one, zero slow-path fallbacks) — filter is not the perf bottleneck. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> fix(docs): drop unresolved intra-doc link in parallel_window `resolve_range_filter_cuts` is private to the adapter module and not in scope from `parallel_window.rs`, so rustdoc rejects the intra-doc link under `-D warnings`. Keep it as plain inline code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> style(core): join split struct decl to satisfy rustfmt Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> refactor(core): slim RangeFilterExec — scheduler owns cuts/partition indices, RFE widens by halo Split the range-partitioning concerns out of RangeFilterExec so it looks like PerPartitionFilterExec's counterpart for sorted-key filtering: - RFE fields: input, routing_expr, halo_lo/hi (ScalarValue), raw_bounds (late-bound), sorted_on_key detection, metrics. - Gone: cuts, partition_indices, resolve_cuts, restrict_partitions, try_new_with_indices. Widening from cuts+halo to per-partition bounds moves scheduler-side (adapter builds raw_bounds from cuts; RFE widens by its own halos internally at resolve_bounds time). - task_builder RFE branch is now a plain "slice raw_bounds parallel to input restriction" — no partition_indices remap. - All APIs and proto fields are ScalarValue (arrow-primitive-generic); internal downcast to f64 with Err for non-Float64 until KLL widens. Halos are functional on RFE (widens raw→widened at resolve time), not write-only decoration. The scheduler-side cut_partitions also needs halo-widened overlap for correct file routing to RANGE-frame consumers; that's a separate cross-stage lookup left as a TODO here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> docs(core): fix RangeFilterExec intra-doc link at module scope `[\`Self::resolve_bounds\`]` on line 39 was in the module-level `//!` comment where `Self` is not defined. CI runs cargo doc with -D warnings so it fails; local runs pass silently. Use the fully-qualified path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> docs(scheduler): revert PPFE→RFE comment renames to reduce PR diff Three files carried only doc/comment renames from PerPartitionFilterExec to RangeFilterExec — no code changes. PPFE still exists in the tree, so the original phrasing remains accurate. Reverting shrinks the PR's review surface without touching semantics; a follow-up sweep can update these comments after PPFE is fully retired. - exchange.rs: 3 comment mentions of PPFE-as-cuts-consumer - test/coalesce_rule.rs: 1 test comment - test/range_repartition.rs: 2 test comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> fix(scheduler): plant RSE#1 below SortExec so cuts land before ORRE routes `ParallelWindowRule` used to leave `RSE#1` above `SortExec`, so the local sketch only started ingesting after Sort had fully materialized. ORRE consumed from RSE#1 as soon as Sort emitted, meaning the scheduler often handed ORRE a still-being-built sketch → approximate cuts → skewed shuffle files. Two changes to close this: 1. Move the rule to run *after* the DataFusion optimizer chain. At the old position the input was `BWAG → DataSource` (sources with `sort_order_for_reorder` satisfy BWAG's ordering natively, no Sort inserted yet); the SortExec placement we care about is only materialized once EnforceSorting / RepartitionFileScans have run. Running earlier also lets DF's later sort-pushdown move any Sort we plant down through the passthrough RSE#1, undoing the intended order. 2. Strip whatever DF planted for BWAG's SinglePartition + Sorted requirements (SPM and/or SortExec) and plant a fresh `SortExec → RSE#1 → source` chain below `ORRE`. The fresh Sort is the pipeline break: it consumes all input before emitting the first row, so RSE#1's sketch fully reports while Sort buffers. Q8 (h2o, SF=1e7, 8 vcores): - rule skipped (buggy pattern): 61s - RSE#1 above Sort (prior): 24s - RSE#1 below Sort (this): 17s ← ~1.4× speedup over prior Wide-RFE metrics on the new plan: input=18.84M / output=11.01M against a 10M-row dataset → 1.88× row-level read amplification, matching the theoretical (1 + halo/cut_width) ≈ 1.24× floor plus batch-granularity overhead from RangeShuffleReader. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> � Conflicts: � ballista/core/proto/ballista.proto � ballista/core/src/execution_plans/mod.rs � ballista/core/src/execution_plans/range_filter.rs � ballista/core/src/serde/generated/ballista.rs
1 parent 526918f commit f7f59c5

18 files changed

Lines changed: 1245 additions & 167 deletions

File tree

ballista/core/proto/ballista.proto

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,10 @@ message PerPartitionFilterExecNode {
136136
repeated datafusion.PhysicalExprNode predicates = 1;
137137
}
138138

139-
// Filter inputs with a per-input-partition half-open range predicate
140-
// widened by `halo_lo` / `halo_hi`. `raw_bounds[k]` is the cut range for
141-
// input partition `k` before halo widening; RFE widens internally at
142-
// resolve time. Zero halo recovers the exact range-repartition trim used
139+
// Filter inputs with a per-input-partition half-open range
140+
// predicate widened by `halo_lo` / `halo_hi`. `raw_bounds[k]` is the cut
141+
// range for input partition `k` before halo widening; RFE widens internally
142+
// at resolve time. Zero halo recovers the exact range-repartition trim used
143143
// above `ShuffleReaderExec`; non-zero halo widens each partition's read
144144
// range to include a boundary "context" band (bounded RANGE-frame windows).
145145
// The child plan is plumbed by the framework as `inputs[0]` during decode.

ballista/core/src/config.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,11 @@ pub const BALLISTA_COALESCE_ENABLED: &str = "ballista.planner.coalesce.enabled";
136136
/// This could benefit the workload by injecting EmptyExec in the plan (i.e during joins)
137137
pub const BALLISTA_PROPAGATE_EMPTY_ENABLED: &str =
138138
"ballista.planner.propagate_empty.enabled";
139+
/// Configuration key to enable the AQE `ParallelWindowRule`, which rewrites
140+
/// bounded-RANGE-frame windows into a distributed range-shuffle so BWAG's
141+
/// single-partition constraint isn't a serial bottleneck. Opt-in.
142+
pub const BALLISTA_PARALLEL_WINDOW_ENABLED: &str =
143+
"ballista.planner.parallel_window.enabled";
139144
/// Configuration key for the target post-coalesce partition byte size (bytes).
140145
/// Mirrors Spark's `spark.sql.adaptive.advisoryPartitionSizeInBytes`.
141146
pub const BALLISTA_COALESCE_TARGET_PARTITION_BYTES: &str =
@@ -323,6 +328,14 @@ static CONFIG_ENTRIES: LazyLock<HashMap<String, ConfigEntry>> = LazyLock::new(||
323328
of a join, allowing downstream work to be skipped.".to_string(),
324329
DataType::Boolean,
325330
Some(true.to_string())),
331+
ConfigEntry::new(BALLISTA_PARALLEL_WINDOW_ENABLED.to_string(),
332+
"Enables the AQE parallel-window rule (ParallelWindowRule), which \
333+
rewrites bounded-RANGE-frame windows into a distributed range-shuffle \
334+
so BoundedWindowAggExec's single-partition constraint is not a serial \
335+
bottleneck. Disabled by default — opt in when the workload contains \
336+
matching window shapes.".to_string(),
337+
DataType::Boolean,
338+
Some(false.to_string())),
326339
ConfigEntry::new(
327340
BALLISTA_COALESCE_TARGET_PARTITION_BYTES.to_string(),
328341
"Target post-coalesce partition size in bytes. Mirrors Spark's \
@@ -706,6 +719,11 @@ impl BallistaConfig {
706719
self.get_bool_setting(BALLISTA_PROPAGATE_EMPTY_ENABLED)
707720
}
708721

722+
/// Returns whether the AQE parallel-window rule is enabled.
723+
pub fn parallel_window_enabled(&self) -> bool {
724+
self.get_bool_setting(BALLISTA_PARALLEL_WINDOW_ENABLED)
725+
}
726+
709727
/// Returns compression codec that will be used during write stage of shuffle
710728
pub fn shuffle_compression_codec(
711729
&self,

ballista/core/src/execution_plans/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ pub use ordered_range_repartition::OrderedRangeRepartitionExec;
4848
pub use partitioned_bounded_window_agg::PartitionedBoundedWindowAggExec;
4949
pub use per_partition_filter::{PerPartitionFilterExec, range_partition_predicates};
5050
pub use plan_algebra::{preserves_distribution, preserves_partitioning};
51-
pub use range_filter::{RangeBound, RangeFilterExec, WidenedBound};
51+
pub use range_filter::RangeFilterExec;
5252
pub use range_shuffle_reader::RangeShuffleReaderExec;
5353
pub use runtime_stats::{
5454
MergedRuntimeStats, RuntimeStatsExec, TaskRuntimeStats,

ballista/core/src/execution_plans/ordered_range_repartition.rs

Lines changed: 51 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -178,25 +178,13 @@ impl OrderedRangeRepartitionExec {
178178
routing.expr
179179
);
180180
}
181-
// Input MUST claim to be sorted on our routing expression — otherwise
182-
// the k-way merge produces garbled output. Sortedness of individual
183-
// input partitions is enforced by the operator upstream (`SortExec`
184-
// with `preserve_partitioning=true`); this check verifies the plan
185-
// node declares that property.
186-
let input_first_sort = input.output_ordering().map(|ordering| ordering.first());
187-
let Some(input_first) = input_first_sort else {
188-
return internal_err!(
189-
"OrderedRangeRepartitionExec requires sorted input — child plan claims no ordering"
190-
);
191-
};
192-
if input_first.expr.as_ref() != routing.expr.as_ref() {
193-
return internal_err!(
194-
"OrderedRangeRepartitionExec: input's first sort key `{}` does not match \
195-
routing expression `{}`",
196-
input_first.expr,
197-
routing.expr
198-
);
199-
}
181+
// NB: input sortedness is NOT checked at try_new. The k-way merge in
182+
// execute() needs each input partition sorted on the routing key, but
183+
// that guarantee comes from `required_input_ordering()` below +
184+
// `EnforceSorting`: DataFusion inserts a `SortExec` above the source
185+
// when the declared requirement isn't satisfied. Checking at try_new
186+
// races with rule-time construction (rules run before EnforceSorting),
187+
// so the runtime check has moved to `execute()`.
200188
// Advertise each output partition as sorted on `order_by`. Downstream
201189
// operators (BWAG, HaloDrop) rely on this claim to skip redundant
202190
// Sort insertions.
@@ -362,6 +350,30 @@ impl ExecutionPlan for OrderedRangeRepartitionExec {
362350
partition: usize,
363351
ctx: Arc<TaskContext>,
364352
) -> Result<SendableRecordBatchStream> {
353+
// Invariant: EnforceSorting must have satisfied our
354+
// `required_input_ordering` — the k-way merge assumes each input
355+
// partition is sorted on the routing key. Rules that emit ORRE run
356+
// before EnforceSorting, so this check lives at execute-time rather
357+
// than construction-time.
358+
let input_first = self
359+
.input
360+
.output_ordering()
361+
.map(|ordering| ordering.first())
362+
.ok_or_else(|| {
363+
internal_datafusion_err!(
364+
"OrderedRangeRepartitionExec: input claims no ordering at execute — \
365+
EnforceSorting should have planted a SortExec"
366+
)
367+
})?;
368+
let routing = &self.order_by[0];
369+
if input_first.expr.as_ref() != routing.expr.as_ref() {
370+
return internal_err!(
371+
"OrderedRangeRepartitionExec: input's first sort key `{}` does not \
372+
match routing expression `{}`",
373+
input_first.expr,
374+
routing.expr
375+
);
376+
}
365377
let mut state = self
366378
.state
367379
.lock()
@@ -756,17 +768,24 @@ mod tests {
756768
}
757769

758770
#[test]
759-
fn try_new_rejects_unsorted_input() {
771+
fn execute_rejects_unsorted_input() {
772+
// try_new no longer checks input ordering — EnforceSorting is
773+
// trusted to plant a SortExec after rule-time construction. If
774+
// the invariant is broken by the time we get to execute(), the
775+
// runtime check fires.
760776
let schema = schema_v2_id();
761-
// MemorySourceConfig with no declared ordering — output_ordering() is None.
762-
let err = OrderedRangeRepartitionExec::try_new(
777+
let orre = OrderedRangeRepartitionExec::try_new(
763778
empty_input(&schema),
764779
vec![asc(&schema, "v2")],
765780
4,
766781
)
767-
.expect_err("input without ordering claim must be rejected");
782+
.expect("construction succeeds; check moved to execute()");
783+
let ctx = datafusion::prelude::SessionContext::new().task_ctx();
784+
let Err(err) = orre.execute(0, ctx) else {
785+
panic!("execute() should reject input without ordering claim");
786+
};
768787
assert!(
769-
err.to_string().contains("child plan claims no ordering"),
788+
err.to_string().contains("input claims no ordering"),
770789
"got: {err}"
771790
);
772791
}
@@ -790,20 +809,24 @@ mod tests {
790809
}
791810

792811
#[test]
793-
fn try_new_rejects_mismatched_sort_key() {
812+
fn execute_rejects_mismatched_sort_key() {
794813
let schema = schema_v2_id();
795-
// Sort input on `id` (Int64); DRR tries to route on `v2`.
814+
// Sort input on `id` (Int64); ORRE tries to route on `v2`.
796815
let source = empty_input(&schema);
797816
let id_sort = LexOrdering::new(vec![asc(&schema, "id")]).unwrap();
798817
let sorted_on_id =
799818
Arc::new(SortExec::new(id_sort, source).with_preserve_partitioning(true))
800819
as Arc<dyn ExecutionPlan>;
801-
let err = OrderedRangeRepartitionExec::try_new(
820+
let orre = OrderedRangeRepartitionExec::try_new(
802821
sorted_on_id,
803822
vec![asc(&schema, "v2")],
804823
4,
805824
)
806-
.expect_err("mismatched sort key must be rejected");
825+
.expect("construction succeeds; check moved to execute()");
826+
let ctx = datafusion::prelude::SessionContext::new().task_ctx();
827+
let Err(err) = orre.execute(0, ctx) else {
828+
panic!("execute() should reject mismatched sort key");
829+
};
807830
assert!(
808831
err.to_string().contains("does not match routing"),
809832
"got: {err}"

ballista/core/src/execution_plans/range_filter.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ use std::task::{Context, Poll};
6262

6363
use datafusion::arrow::array::{Array, RecordBatch};
6464
use datafusion::arrow::compute::filter_record_batch;
65-
use datafusion::arrow::datatypes::SchemaRef;
65+
use datafusion::arrow::datatypes::{DataType, SchemaRef};
6666
use datafusion::common::cast::{as_boolean_array, as_float64_array};
6767
use datafusion::common::{Result, Statistics, internal_err};
6868
use datafusion::execution::TaskContext;
@@ -90,7 +90,7 @@ pub type RangeBound = (Option<ScalarValue>, Option<ScalarValue>);
9090

9191
/// Bounds after halo widening. Float64-only internally today — see the
9292
/// "Type generality" section in the module doc.
93-
pub type WidenedBound = (Option<f64>, Option<f64>);
93+
type WidenedBound = (Option<f64>, Option<f64>);
9494

9595
/// Both raw and widened bounds. `raw` is preserved for serialization; the
9696
/// executor consumes `widened`.
@@ -654,12 +654,17 @@ impl RecordBatchStream for RangeFilterStream {
654654
}
655655
}
656656

657+
// Silence the unused import warning when the file is compiled without
658+
// arrow — DataType is only exercised via `data_type(...)` return checks.
659+
#[allow(dead_code)]
660+
fn _touch_datatype(_: DataType) {}
661+
657662
#[cfg(test)]
658663
mod tests {
659664
use super::*;
660665
use datafusion::arrow::array::Float64Array;
661666
use datafusion::arrow::compute::SortOptions;
662-
use datafusion::arrow::datatypes::{DataType, Field, Schema};
667+
use datafusion::arrow::datatypes::{Field, Schema};
663668
use datafusion::datasource::memory::MemorySourceConfig;
664669
use datafusion::datasource::source::DataSourceExec;
665670
use datafusion::physical_expr::LexOrdering;

ballista/core/src/execution_plans/runtime_stats.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -874,6 +874,16 @@ pub fn repartition_routing_expr(
874874
/// confirms the file is empty (`Some(0)`). If the file has rows or the
875875
/// row count is unknown (`None`), silently skipping would lose data —
876876
/// error out instead.
877+
///
878+
/// TODO(halo-aware routing): when the downstream stage has a
879+
/// `RangeFilterExec` with non-zero halo (bounded RANGE-frame windows), each
880+
/// partition's *effective* read range is `[cuts[k-1] - halo_lo, cuts[k] +
881+
/// halo_hi)`. This function currently uses the raw cut range, so files
882+
/// straddling the halo boundary aren't routed to their halo-widened
883+
/// consumer. Boundary rows near cuts can be missing from downstream
884+
/// window sums — a correctness gap for RANGE frames that this refactor
885+
/// does not resolve. Fix requires reaching across stages to read the
886+
/// consumer RFE's halos and widening the overlap check here.
877887
pub fn cut_partitions(
878888
original_partitions: Vec<Vec<PartitionLocation>>,
879889
reports: &[TaskRuntimeStats],
@@ -1755,7 +1765,7 @@ mod overlap_remap_tests {
17551765

17561766
/// A straddling sub-part — one whose sketched [min, max] spans the cut
17571767
/// — appears in BOTH downstream partitions' lists. This is the case
1758-
/// PerPartitionFilterExec exists to clean up.
1768+
/// RangeFilterExec exists to clean up.
17591769
#[test]
17601770
fn overlap_remap_straddling_producer_appears_in_both_partitions() {
17611771
// Producer 300 covers [5, 25) — straddles the cut at 15.

ballista/core/src/serde/generated/ballista.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,10 @@ pub struct PerPartitionFilterExecNode {
152152
::datafusion_proto::protobuf::PhysicalExprNode,
153153
>,
154154
}
155-
/// Filter inputs with a per-input-partition half-open range predicate
156-
/// widened by `halo_lo` / `halo_hi`. `raw_bounds\[k\]` is the cut range for
157-
/// input partition `k` before halo widening; RFE widens internally at
158-
/// resolve time. Zero halo recovers the exact range-repartition trim used
155+
/// Filter inputs with a per-input-partition half-open range
156+
/// predicate widened by `halo_lo` / `halo_hi`. `raw_bounds\[k\]` is the cut
157+
/// range for input partition `k` before halo widening; RFE widens internally
158+
/// at resolve time. Zero halo recovers the exact range-repartition trim used
159159
/// above `ShuffleReaderExec`; non-zero halo widens each partition's read
160160
/// range to include a boundary "context" band (bounded RANGE-frame windows).
161161
/// The child plan is plumbed by the framework as `inputs\[0\]` during decode.

ballista/executor/src/execution_engine.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@
2323
2424
use ballista_core::client_pool::BallistaClientPool;
2525
use ballista_core::execution_plans::sort_shuffle::SortShuffleWriterExec;
26-
use ballista_core::execution_plans::{ShuffleReaderExec, ShuffleWriterExec};
26+
use ballista_core::execution_plans::{
27+
RangeShuffleReaderExec, ShuffleReaderExec, ShuffleWriterExec,
28+
};
2729
use ballista_core::serde::protobuf::ShuffleWritePartition;
2830
use ballista_core::serde::scheduler::PartitionStats;
2931
use ballista_core::{JobId, utils};
@@ -150,6 +152,17 @@ impl ExecutionEngine for DefaultExecutionEngine {
150152
reader.with_work_dir(work_dir.to_string()),
151153
))),
152154
}
155+
} else if let Some(reader) = p.downcast_ref::<RangeShuffleReaderExec>() {
156+
match &self.client_pool {
157+
Some(client_pool) => Ok(Transformed::yes(Arc::new(
158+
reader
159+
.with_work_dir(work_dir.to_string())
160+
.with_client_pool(client_pool.clone()),
161+
))),
162+
None => Ok(Transformed::yes(Arc::new(
163+
reader.with_work_dir(work_dir.to_string()),
164+
))),
165+
}
153166
} else {
154167
// Scan restriction is scheduler-side (see
155168
// ballista/scheduler/src/state/task_builder.rs). The plan

ballista/scheduler/src/cluster/mod.rs

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ use crate::state::execution_graph::{
2424
use crate::state::task_manager::JobInfoCache;
2525
use ballista_core::config::BallistaConfig;
2626
use ballista_core::error::Result;
27-
use ballista_core::execution_plans::ShuffleReaderExec;
27+
use ballista_core::execution_plans::{RangeShuffleReaderExec, ShuffleReaderExec};
2828
use ballista_core::serde::protobuf::{
2929
AvailableVcores, ExecutorHeartbeat, JobStatus, job_status,
3030
};
@@ -369,15 +369,17 @@ pub trait JobState: Send + Sync {
369369
/// (e.g. `UnorderedRangeRepartitionExec`). Stops at leaves, multi-child
370370
/// operators (fan-in / joins), and stage boundaries.
371371
///
372-
/// The stage-boundary stop is currently a `ShuffleReaderExec` downcast — the
373-
/// only kind of stage-boundary leaf that appears in a resolved stage plan.
374-
/// The *general* rule is "stop at any stage boundary"; if new stage-boundary
375-
/// operators appear, add them here (or, better, get `ExecutionPlan` upstream
376-
/// to expose an `is_stage_boundary()` property so we don't keep
377-
/// enumerating).
372+
/// The stage-boundary stop enumerates the leaf readers that terminate a
373+
/// resolved stage plan: `ShuffleReaderExec` (regular / broadcast / coalesced)
374+
/// and `RangeShuffleReaderExec` (ordering-preserving). The *general* rule is
375+
/// "stop at any stage boundary"; if new stage-boundary operators appear, add
376+
/// them here (or, better, get `ExecutionPlan` upstream to expose an
377+
/// `is_stage_boundary()` property so we don't keep enumerating).
378378
fn stage_has_input_collapse(plan_root: &Arc<dyn ExecutionPlan>) -> bool {
379379
fn walk(node: &Arc<dyn ExecutionPlan>) -> bool {
380-
if node.downcast_ref::<ShuffleReaderExec>().is_some() {
380+
if node.downcast_ref::<ShuffleReaderExec>().is_some()
381+
|| node.downcast_ref::<RangeShuffleReaderExec>().is_some()
382+
{
381383
return false;
382384
}
383385
if node.properties().output_partitioning().partition_count() == 1 {
@@ -665,6 +667,7 @@ mod test {
665667
ExecutorMetadata, ExecutorOperatingSystemSpecification, ExecutorSpecification,
666668
};
667669

670+
use crate::cluster::stage_has_input_collapse;
668671
use crate::cluster::{BoundTask, bind_task_bias, bind_task_round_robin};
669672
use crate::state::execution_graph::{ExecutionGraph, StaticExecutionGraph};
670673
use crate::state::task_manager::JobInfoCache;
@@ -891,4 +894,37 @@ mod test {
891894
},
892895
]
893896
}
897+
898+
/// Both shuffle reader kinds are stage boundaries — walking through them
899+
/// to detect an input collapse would mis-classify the *next* stage's leaf
900+
/// as this stage's collapse. `stage_has_input_collapse` must return false
901+
/// as soon as a reader is seen. Guard the range variant explicitly since
902+
/// `UnknownPartitioning(1)` would otherwise trigger the single-partition
903+
/// arm.
904+
#[test]
905+
fn stage_has_input_collapse_stops_at_range_reader() {
906+
use ballista_core::execution_plans::RangeShuffleReaderExec;
907+
use datafusion::arrow::datatypes::{DataType, Field, Schema};
908+
use datafusion::physical_expr::expressions::Column;
909+
use datafusion::physical_expr::{LexOrdering, PhysicalSortExpr};
910+
use datafusion::physical_plan::ExecutionPlan;
911+
use datafusion::physical_plan::coalesce_partitions::CoalescePartitionsExec;
912+
913+
let schema =
914+
Arc::new(Schema::new(vec![Field::new("v", DataType::Float64, false)]));
915+
let sort_expr = PhysicalSortExpr::new_default(Arc::new(Column::new("v", 0)));
916+
let merge_ordering = LexOrdering::new(vec![sort_expr]).unwrap();
917+
// Single output partition — the case where the `partition_count == 1`
918+
// arm would fire without the reader guard.
919+
let reader = Arc::new(
920+
RangeShuffleReaderExec::try_new(1, vec![vec![]], schema, merge_ordering)
921+
.unwrap(),
922+
) as Arc<dyn ExecutionPlan>;
923+
let root: Arc<dyn ExecutionPlan> = Arc::new(CoalescePartitionsExec::new(reader));
924+
925+
assert!(
926+
!stage_has_input_collapse(&root),
927+
"a range-shuffle reader is a stage boundary, not an input collapse",
928+
);
929+
}
894930
}

0 commit comments

Comments
 (0)