feat: parallel BWAG for the range-window shape (h2o Q8) - #2223
Conversation
a5db932 to
42a21ef
Compare
|
@andygrove @phillipleblanc we now have our first real (EKS) benchmark results for halo-based parallel windows. h2o is kind of a worst case, because as the data keeps getting bigger, the value range stays the same, so the cost of duplicated halos starts to dominate: but even if we cap it at 8 or 32 cores, 5-15x wins seem worthwhile? (not to mention OOM reduction). I promised "this time" I was just going to deliver a PR with e2e abilities, but TBH it's grown too big again. Given that it now proves it works, you guys cool with me splitting off lots of little supporting PRs from this? |
e9a6cfc to
ecff221
Compare
ecff221 to
a0e08c8
Compare
…2253) Wraps DataFusion's `BoundedWindowAggExec` and overrides its `SinglePartition` distribution requirement to `Unspecified`. Hides BWAG from tree walkers by returning only the wrapper's input from `children()`, so `EnforceDistribution` can't reinsert an SPM(K→1) beneath it. Safe iff the input is already range-repartitioned + halo covers frame boundaries (see module doc). This is a Ballista-side placeholder for the upstream draft at apache/datafusion#23026 ("Parallel bounded RANGE-frame window functions without PARTITION BY"). Once that lands and Ballista bumps its DF pin past it, this wrapper collapses and callers target DF's BWAG directly. - `ballista_core::execution_plans::partitioned_bounded_window_agg`: the new operator. `InputOrderMode` and `can_repartition` are hardcoded (`Sorted` / `false`) — the only planned caller is a no-PARTITION-BY + single-Column-ORDER-BY range-window rule. - `BallistaPhysicalPlanNode::PartitionedBoundedWindowAgg`: proto message carrying only `window_expr` — the rest is implicit from the caller's shape gates. Round-trip goes through DF's `serialize_physical_window_expr` / `parse_physical_window_expr`. - Unit test `per_partition_execute_running_sum_no_cross_partition_leak` proves BWAG actually aggregates (partition 0's running sums are [1, 3, 6], not [1, 2, 3]) and that the K→K partitioning doesn't leak across partition boundaries (partition 1's first sum is 100, not 106). No in-tree caller yet — this is prep for the parallel-window rule extracted from #2223. Landing it separately keeps that PR's diff focused on the scheduler rule. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9004b04 to
6be8120
Compare
…er (purely additive) **Purely additive.** No in-tree callers, no behavior change. The type is defined, serialized, and round-trip tested; wiring lands in follow-ups. Extracted from apache#2223 (a working end-to-end parallel-window branch) as the first slice of the epic to land parallel windows incrementally, one reviewable piece at a time. Sibling to `ShuffleReaderExec`: keeps each upstream source alive as its own stream and feeds all N into a `StreamingMerge` keyed on the child's declared output ordering. The regular reader concatenates in arrival order, which breaks the monotonicity RANGE-frame window operators (and sort-merge-join build sides) require. - `range_shuffle_reader.rs` — operator impl + tests - proto message + oneof slot 12 - serde encode/decode + roundtrip test - three shuffle_reader helpers promoted to `pub(crate)` so RSR can reuse them: `local_remote_read_split`, `fetch_partition_local`, `fetch_partition_remote`
…er (purely additive) (#2255) * feat(core): RangeShuffleReaderExec — ordering-preserving shuffle reader (purely additive) **Purely additive.** No in-tree callers, no behavior change. The type is defined, serialized, and round-trip tested; wiring lands in follow-ups. Extracted from #2223 (a working end-to-end parallel-window branch) as the first slice of the epic to land parallel windows incrementally, one reviewable piece at a time. Sibling to `ShuffleReaderExec`: keeps each upstream source alive as its own stream and feeds all N into a `StreamingMerge` keyed on the child's declared output ordering. The regular reader concatenates in arrival order, which breaks the monotonicity RANGE-frame window operators (and sort-merge-join build sides) require. - `range_shuffle_reader.rs` — operator impl + tests - proto message + oneof slot 12 - serde encode/decode + roundtrip test - three shuffle_reader helpers promoted to `pub(crate)` so RSR can reuse them: `local_remote_read_split`, `fetch_partition_local`, `fetch_partition_remote` * docs(executor): TODO for RangeShuffleReaderExec late-bind Marks the follow-up: create_query_stage_exec only downcasts ShuffleReaderExec, so a task carrying a RangeShuffleReaderExec would miss work_dir + client_pool the moment a planner rule plants one. No functional change today — no code path emits the operator yet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dc0f80a to
0f9c18b
Compare
f7f59c5 to
52d1c34
Compare
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 apache#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
52d1c34 to
9971682
Compare
The h2o job was still running Q8 through the baseline serial pipeline — `ballista.planner.parallel_window.enabled` defaults to `false`, so the whole rule was inert here. Flipping it on lets `--verify` diff the parallel path against the local DataFusion oracle in CI, which is what we actually want before undrafting apache#2223. Other queries in the suite either don't match the rule's shape gates (no PARTITION BY + single-column Float64 ORDER BY + finite RANGE frame) and pass through untouched, or they do match and now get extra coverage.
Missing rows in Q8 window sums — `cut_partitions` routed files by raw cut ranges, but the downstream `RangeFilterExec` widens each partition by `[halo_lo, halo_hi]`. A producer file whose sketch fell entirely on one side of a cut but within halo width of it was never delivered to the neighbouring partition, so RANGE-frame window sums lost those halo rows. - `cut_partitions` gains `(halo_lo, halo_hi)` f64 args; the b_lo/b_hi partition_points shift the sketch by the halos so bucket k's effective range becomes `[cuts[k-1] - halo_lo, cuts[k] + halo_hi)`. - `aqe/mod.rs::update_stage_progress` walks the full plan for the `RangeFilterExec` sitting directly on the boundary `ExchangeExec` (matched by `routing_expr` eq) and reads its halos. Absent RFE is an error — range-repartition writers produce straddler duplicates that only the reader-side filter can trim, so the shape is required. - Non-Float64 halo scalars also error (shape violation). Verified: h2o Q8 @ 1e7 with `parallel_window.enabled=true` now diffs to the DataFusion oracle at OK. WIP: reviewer TODOs left inline for follow-up: - multi-hit handling in `repartition_routing_expr` - test coverage for halo_hi and K=3 partitions - whether the walker should scope to the downstream stage - multi-legged plan behaviour - hard-error on routing_expr mismatch Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ng spine The previous body descended into every child looking for the first (U/O)RRE, so a range-repartition op sitting below a join or union leg would be misattributed as driving this stage's output partitioning. In practice today only ParallelWindowRule plants RREs (single-legged shape), so the misattribution was inert — but the shape was ambiguous and needed to become explicit before further rules land. Descend only through nodes on the [`preserves_partitioning`] whitelist (Filter, Projection, Sort-with-preserve-partitioning, RSE, ShuffleWriter, Window, Buffer). Any partition-non-preserving barrier (join, union, hash-agg, unknown op) stops descent and returns `Ok(None)` — the RRE below such a barrier isn't visible in this stage's output partitioning anyway. Multi-child descent-through-node is now an internal_err since every whitelist entry is single-child by construction; the guard catches whitelist bugs early. No behaviour change on the current call sites; every existing `repartition_routing_expr_*` test still passes and h2o Q8 @ 1e7 still verifies OK against the DataFusion oracle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The K=2 halo test only proved `halo_lo` widens downward on a 2-partition layout — nothing verified `halo_hi` or that the halo band stays local to adjacent partitions. Replace with a K=5 layout using asymmetric halos (`halo_lo=1`, `halo_hi=2`) so a single fixture proves: - `halo_lo` widens each partition downward (200 → P2 via halo_lo). - `halo_hi` widens each partition upward (400 → P2 via halo_hi). - The middle partition sees siblings from BOTH halo bands simultaneously. - Halo does NOT bleed across two cut hops — P0 doesn't get 200's file, P4 doesn't get 400's file. Existing `disjoint` and `straddling` tests still cover the no-halo path (halo=0,0), so the raw-cut semantics stay separately verified. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…errors on shape violations Two concerns the previous walker punted on: - `PhysicalExpr::eq(routing_expr)` isn't unique across stages — two range-repartition boundaries could share the same `Column(name, idx)` shape after independent projections, and the top-down walk would return the first match rather than THIS boundary's RFE. - A `RangeFilterExec` with `children().len() != 1` is a shape violation, but the walker silently `Continue`d past it. Match by `ExchangeExec::stage_id() == producer_stage_id` — the caller already has the completing stage's id and stage_id is uniquely assigned per boundary, so no cross-stage collision. Any non-single- child RFE hard-errors via `internal_err!`. The manual `halo_err` sentinel is gone — `apply` propagates `Err` from the closure directly. Also fixes the `[`preserves_partitioning`]` intra-doc link that CI caught with `-D warnings` — the function lives in `super::` from the runtime_stats scope. Verified: h2o Q8 @ 1e7 with `parallel_window.enabled=true` still verifies OK against the DataFusion oracle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`resolve_range_filter_cuts` used `PhysicalExpr::eq` on routing_expr to
pair each pending `RangeFilterExec` with its boundary `ExchangeExec`.
That's ambiguous in multi-legged shapes (a future SMJ with
range-repartition on both sides could have both boundaries route on
`Column("v", 0)` after independent projections, silently wiring the
wrong cuts to the wrong RFE). Same latent bug shape as the halo
walker just fixed via `stage_id` disambiguation.
Cure: descend the RFE's own single-child spine to the first
descendant `ExchangeExec`. Descent is unique by construction — every
RFE has exactly one child — so multi-legged shapes pair each RFE with
its own leg's boundary. Then verify `rf.routing_expr().eq(&routing.
routing_expr)` as a plant-time sanity check; disagreement means the
rule wired the wrong RFE to the boundary.
Shape violations (multi-child RFE, spine fork before hitting an
exchange, unresolved descendant routing, routing_expr mismatch) all
hard-error via `internal_err!` — plant-time or stage-progress bugs
that should never be silently absorbed.
No behaviour change on today's single-legged Q8 shape; h2o Q8 @ 1e7
with `parallel_window.enabled=true` still verifies OK against the
DataFusion oracle.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The doc was a running notebook between me and Claude — the "rope-bridge principle" formula, the end-state sketch, the ticked/unticked plan — not public developer reference material. Moving it to `.local/` (gitignored) keeps the notebook accessible locally without shipping it as `docs/developer/`. Also drops three memory-slug refs (`[[parallel-range-window]]`, `[[project-prefix-scan-two-pass-rejected]]`, `[[kll-sketch]]`) from `parallel_window.rs`'s module doc — those were private auto-memory links, they don't render as intra-doc references and readers can't follow them. Replaced with plain-English equivalents that stand on their own. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o one fn
`as_candidate` and `rewrite_bwag` were split for no real reason: the former
was a loose shape check that got thrown away and re-asserted by the latter
(BWAG re-downcast with an "internal error: caller passed non-BWAG" path
that could never fire). The split also blurred failure signaling — the
non-Float64 order key and non-numeric halo bound checks came back as
`Err("shape matched but skipped")` when they're really shape gates.
Merged into `maybe_rewrite_bwag(node, K) -> Result<Option<...>>`:
- `Ok(None)` = shape gate missed (not a BWAG, PARTITION BY, ROWS frame,
UNBOUNDED bound, non-Float64 order key, non-numeric halo scalar, or
already-rewritten subtree). Silent — no log spam on the hot path.
- `Ok(Some(_))` = rewrite applied.
- `Err(_)` = actual invariant violation (BWAG/SPM/SortExec with =/= 1
child, schema lookup on ORDER BY expr, constructor try_new).
Drops the `WindowCandidate` shuttle struct and the dead BWAG re-downcast.
`as_finite` -> `is_finite` (bool now that the passthrough-of-reference
buys nothing); `halo_from_bound` -> `Option<f64>` matching the new
"non-numeric = shape gate" model, with its test updated in kind.
Verified: h2o Q8 @ 1e7 still produces the expected RFE_narrow -> PBWAG ->
RFE_wide(halo=[3,0]) -> ORRE(->8 parts) plan and `--verify` passes vs the
DataFusion oracle on 10M rows.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@phillipleblanc reviews are always appreciated if you have time. I finally got the diff down on this one to a reviewable size by merging all its dependencies. I think prefix scanning or extending this to cover |
|
thanks @avantgardnerio , will need some time to review this |
|
This makes sense as an improvement - I have one question about a potential wrong-answer issue:
I believe this means the query can succeed while returning incorrect results. We could initially restrict this optimization to ascending order, which will prevent the issue, but the correct fix would be to reverse the halo directions for descending order. |
SQL RANGE frame semantics invert with sort direction — `k PRECEDING` under DESC refers to *larger* values — so the current halo (widens the lower side of each bucket) misses frame ancestors when the query is DESC. Rewrite silently returned wrong sums. Gate DESC to the serial path with a TODO covering both pieces DESC support needs: halo direction swap in the rule + a mirrored DESC branch in `RangeFilterExec::sorted_on_key`'s fast path. Reported by @phillipleblanc on apache#2223. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
thanks @phillipleblanc ! That was indeed a bug. I've added the ASC restriction for now, a test, and a TODO. I think it would be better to tackle it in a follow up PR to keep this one from growing any bigger. |
andygrove
left a comment
There was a problem hiding this comment.
This is epic! Thanks @avantgardnerio and thanks @phillipleblanc for the review
Summary
Turns h2o's window-Q8 shape (
sum(v2) OVER (ORDER BY v2 RANGE BETWEEN 3 PRECEDING AND CURRENT ROW)) from a serial pipeline into a distributed range-shuffle.The stack layers cleanly on top of the parallel-window primitives that have already landed (#2038, #2169, #2175, #2180, #2195, #2196):
ParallelWindowRulematchesBoundedWindowAggExecin the "no PARTITION BY + single Column ORDER BY on Float64 + finite RANGE frame" shape and rewrites as a completely parallel pipeline.is_stage_boundaryinDistributedExchangeRuleteaches the SPM branch that aRangeFilterExecsitting directly on a resolvedExchangeExeccounts as part of the boundary: we chose not to fold range-filtering intoShuffleReader/ExchangeExec, so the filter is conceptually part of the boundary shape by design.PartitionedBoundedWindowAggExecis a Ballista-specific wrapper for DataFusion'sBoundedWindowAggExec. It hides BWAG from tree walkers (children()returns only the wrapper's input) and declaresDistribution::UnspecifiedDistribution, soEnforceDistributiondoesn't insert anSPM(K→1)beneath.execute(i)delegates straight toBWAG::execute(i), which already processes each partition independently: DataFusion's BWAG algorithm has no cross-partition state. Safe because the rule's shape gates guarantee range-repartition upstream + halo covers frame boundaries.Effectively, this is
apache/datafusion#23026(parallel-BWAG) implemented as a Ballista-side wrapper: one operator, no DF-internals fork.Plan shape
MPTs come from the existing
ballista.scheduler.max_partitions_per_taskknob; on a 2×4-vcore cluster withmax=4we get 2 tasks per stage, one per exec.Results
Measured on EKS (2 pods, each on its own
r6i.24xlargenode, 8 vCPU × 64 GiB per pod, gp3 shuffle work-dir, h2o parquet on S3 ineu-west-1). K=8, MPT=4 → 2 tasks/pod/stage. Sweep runs the full 17-query h2owindow.sqlsuite at 1e8 rows (100M), once withparallel_window.enabled=false, once with=true.Q8 headline (the shape this PR targets)
parallel_window.enabled=false) : SPM collapse → 1-partition BWAGparallel_window.enabled=true) : K=8, 4 partitions/podPer-stage breakdown of the parallel run (from scheduler
/api/job/{id}/stages):ShuffleWriter → RuntimeStats → ORRE → RuntimeStats → SortExec → DataSource(s3)ShuffleWriter → Projection → RangeFilter(narrow) → PartitionedBWAG → RangeFilter(wide) → RangeShuffleReaderStage 0 is dominated by parquet scan + sort; ORRE scatter is cheap (~6 ns/row across 100M rows). Stage 1 is BWAG-dominated as expected; each pod runs 4 output partitions concurrently over its 8 physical vCPUs.
Full 17-query null-test sweep
The rule is deliberately narrow (Q8's exact shape). The other 16 queries are the null-test: same on/off wall time confirms no regression on shapes the rule doesn't target.
sum() OVER (), could range partition / prefix scanfirst_value/row_number ORDER BY id3(unbounded frame)PARTITION BY id1/id2/id3already hash-parallelPARTITION BY id2ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING(index-based)ROWS BETWEEN 100 PRECEDING AND CURRENT ROWROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROWPARTITION BY id2 ORDER BY id3 ROWS ...PARTITION BY id2ROW_NUMBER PARTITION BY id2top-NWhy Q8 is the only mover: it's the only query in the suite where the ordered window can't be parallelized by any existing mechanism.
PARTITION BYgives a natural K-space;EnforceDistribution → Hash(key) → per-partition BWAGalready runs in parallel across K executors. Nothing for this rule to add.row_number,first_valuedefault). Needs a prefix-scan design (cross-partition running state).PARTITION BY(so no natural K-space) nor a cheap-per-row window op (RANGE frame does value-band search per row, ~2.9 μs/row vs. ~380 ns/row for Q7's ROWS UNBOUNDED cumulative sum). That intersection - "must be single-partition AND single-partition is genuinely expensive" - is the niche this rule fills.Workload-shape caveat: the 4.78× win assumes BWAG's per-row cost is comparable to what's around it. Heavier per-row work (a cryptographic hash, an expensive UDF, a
regexp_replacechain) would scale even better, closer to the 8× vcore count. Lighter than a running sum is hard to construct, so 4.78× is roughly the floor for the shape this PR handles.Correctness:
.github/workflows/h2o.ymlruns each h2o query against a single-process DataFusion oracle under--verifyand diffs the row sets. Q8's parallel-path aggregatesummatches baseline to within Float64 associativity noise (relative delta ~5×10⁻¹⁴, under the √N × ε floor for 100M-row parallel summation).Follow-ups (not this PR)