Skip to content

feat(core): add OrderedRangeRepartitionExec — N sorted overlapping → K sorted disjoint - #2169

Merged
avantgardnerio merged 3 commits into
apache:mainfrom
avantgardnerio:brent/ordered-range-repartition-exec
Jul 25, 2026
Merged

feat(core): add OrderedRangeRepartitionExec — N sorted overlapping → K sorted disjoint#2169
avantgardnerio merged 3 commits into
apache:mainfrom
avantgardnerio:brent/ordered-range-repartition-exec

Conversation

@avantgardnerio

@avantgardnerio avantgardnerio commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds OrderedRangeRepartitionExec — the sorted sibling of UnorderedRangeRepartitionExec (#2123). Takes N locally-sorted input partitions whose value ranges may overlap; produces K range-disjoint output partitions where each output is fully sorted on the routing expression. Concatenating the K outputs in order yields a globally-sorted stream.

Reuses range_repartition_common.rs verbatim (landed with #2123): same discovery mechanism — walk child subtree, find sibling RuntimeStatsExec, snapshot T-Digest, compute K − 1 quantile cuts. Same fallback semantics: any discovery failure → single-bucket routing where every row lands in output 0.

Execution model — N × K channels + K k-way merges

  • N scatter tasks. Each reads one sorted input, calls split_batch_by_range on every batch, forwards sub-batches to K per-input senders (one per output partition).
  • K output-partition streams. Each merges N sub-streams via DataFusion's StreamingMergeBuilder — the SortPreservingMerge internals, no custom heap machinery here.
  • Backpressure. Bounded mpsc::channel(2) per input × output, symmetric to the Unordered variant. Total buffered at rest ≤ N × K × 2 batches.

Constructor requires input.output_ordering() to lead with the routing expression — otherwise the merger would produce garbled output. PlanProperties::eq_properties advertises each output partition as sorted on order_by, so downstream operators (BWAG, HaloDrop) can rely on that claim without inserting a redundant SortExec.

Scope

Just the operator + serde plumbing. Nothing inserts OrderedRangeRepartitionExec into a plan yet — no rewrite rule, no scheduler hook. Those land with the parallel-window detection rule alongside the Unordered sibling.

Included:

  • ordered_range_repartition.rs — the operator + 8 unit tests.
  • Proto: OrderedRangeRepartitionExecNode { repeated PhysicalSortExprNode order_by = 1; uint32 output_partitions = 2; } on BallistaPhysicalPlanNode.oneof (tag 9; tag 8 is Unordered).
  • Codec encode/decode registration on BallistaPhysicalExtensionCodec.
  • One serde roundtrip test.
  • No new external deps.
  • No changes to range_repartition_common.rs — reused verbatim.

Drift from source branch

  • RuntimeStatsExec now rejects nullable routing expressions at construction (T-Digest has no NULL slot; tightened during feat(core): add UnorderedRangeRepartitionExec — value-range router over unordered inputs #2123's upstreaming). The original test suite declared v2 as nullable and typed the batch builder as Vec<Option<f64>> but never actually populated a None. Tightened schema + helper types to non-nullable f64; will loosen again when we swap T-Digest for KLL and NULL becomes a first-class routing key.

Tests (9 new)

8 operator unit tests covering: constructor validation (empty order_by, unsorted input, mismatched sort key), end-to-end row conservation across two overlapping inputs, per-output sortedness, range-disjointness of outputs, no-stats fallback, and the double-execute guard.

1 serde roundtrip test (single-key ORDER BY through the SortExec-wrap that satisfies the ordering claim).

Test plan

  • cargo test -p ballista-core — 175/175 pass (25 in the range-repartition modules).
  • cargo clippy --all-targets clean across the workspace.
  • cargo fmt --all clean.

Next in the stack

The rewrite rule (ParallelWindowDetectRule) that inserts either operator above BoundedWindowAggExec based on whether the wrapping plan needs ordering preserved.

🤖 Generated with Claude Code

avantgardnerio and others added 3 commits July 24, 2026 08:58
…K sorted disjoint

Sibling of `UnorderedRangeRepartitionExec` (apache#2123). Same discovery mechanism
(walk child subtree for matching `RuntimeStatsExec`, snapshot T-Digest,
compute quantile cuts via `range_repartition_common::discover_cuts`);
different execution model.

Semantics: takes N locally-sorted input partitions whose value ranges may
overlap; produces K range-disjoint output partitions where each output is
fully sorted on the routing expression. Concatenating the K outputs in order
yields a globally-sorted stream — exactly what Stage 2 of the parallel-window
path needs to feed into Stage 3's BWAG.

Execution model — N × K channels + K k-way merges:

  * N scatter tasks. Each reads one sorted input, calls `split_batch_by_range`
    on every batch, and forwards sub-batches to K per-input senders (one per
    output partition).
  * K output-partition streams. Each merges N sub-streams via DataFusion's
    `StreamingMergeBuilder` — SPM internals, no custom heap machinery here.
  * Channels: bounded `mpsc::channel(2)` per input × output for symmetric
    backpressure with the Unordered variant. Total buffered at rest
    ≤ N × K × 2 batches.

Constructor requires `input.output_ordering()` to lead with the routing
expression — otherwise the merger would produce garbled output.
`PlanProperties::eq_properties` advertises each output partition as sorted on
`order_by`, so downstream operators (BWAG, HaloDrop) can rely on that claim
without an intervening `SortExec`.

Proto + serde follow the Unordered pattern (`uint32 output_partitions` only;
boundaries are runtime-discovered, stateless w.r.t. serde). Eight end-to-end
tests: conservation across two overlapping inputs, each output sorted,
outputs range-disjoint, no-stats fallback, constructor validation (empty
order_by, unsorted input, mismatched sort key), and the double-execute guard.
One serde roundtrip test on top.

Drift from source branch (`brent/h2o-window-benchmarks`):

  * `RuntimeStatsExec` now rejects nullable routing expressions at
    construction (T-Digest has no NULL slot; tightened during apache#2123's
    upstreaming). Original tests declared `v2` as nullable and typed the
    batch builder as `Vec<Option<f64>>` but never populated a `None`.
    Tightened schema + helper types to non-nullable `f64`; will loosen again
    when we swap T-Digest for KLL and NULL becomes a first-class routing key.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the audit Phillip asked for on apache#2123 (commit a281667 on the
unordered sibling). Preemptive polish before opening the PR for review.

- Implement all 6 algebraic ExecutionPlan methods explicitly (required_input_
  distribution, required_input_ordering, maintains_input_order, benefits_from_
  input_partitioning, partition_statistics, cardinality_effect), each with a
  docstring justifying the value. Two differ materially from the unordered
  sibling:

    * required_input_ordering returns Some(order_by): the child MUST be
      sorted on the routing expression — the per-output StreamingMerge
      assumes each of its N sub-streams is locally sorted. Declaring the
      requirement lets the optimizer see it and skip a redundant SortExec
      above us.

    * maintains_input_order returns true: each output is a k-way merge
      across N sorted sub-streams filtered to a value range, so the
      relative order within each output faithfully preserves the child's
      sort. That's the whole point (contrast Unordered, which returns
      false because scattering breaks order).

- Reject nullable routing expressions in try_new. Defense-in-depth against
  the no-stats-pair fallback path where discovery would silently fall back
  to single-bucket. TODO marker points at the KLL swap that lifts the
  restriction. The sibling RuntimeStatsExec already enforces the same
  invariant at plan-assembly time.

- Add try_new_rejects_nullable_routing_key covering the new invariant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ports apache#2123's review-fix commits (6b300df + 422ee6a) to the ordered
sibling.

1. `scatter_input_partition` was spawned as a bare `tokio::spawn` whose
   `JoinHandle` was discarded. A panic inside the body silently dropped
   the sender, and downstream consumers saw a clean EOF while data was
   lost. Wrap it in `SpawnedTask::spawn(guarded_scatter(...))` (helper
   already shared via `range_repartition_common` — added for apache#2123 and
   consumed here verbatim). Hold the wait-tasks in
   `DispatchState._drop_helper` so dropping the exec aborts all N
   scatter tasks. Refactored the scatter body to return `Result<()>`
   instead of internally calling `broadcast_error` — guarded_scatter
   handles the broadcast on error/panic.

2. `PlanProperties::new` defaults to `Lazy` / `NonCooperative`; ORRE
   eagerly drives its inputs from the moment `execute()` is called and
   yields at per-output channel sends, so set `EvaluationType::Eager` +
   `SchedulingType::Cooperative` — same combination DataFusion's
   `RepartitionExec` uses.

3. New `scatter_task_panic_surfaces_as_error` test asserts every output
   partition surfaces the panic as `Err(..)` with the original message
   preserved. `OrderedRangeRepartitionExec::try_new` rejects inputs
   without a declared ordering, so extend `PanickingSourceExec` with a
   `with_ordering(order_by)` constructor that lets the panic-source
   satisfy the ordering-claim precondition.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@avantgardnerio
avantgardnerio marked this pull request as ready for review July 24, 2026 16:04
@avantgardnerio

Copy link
Copy Markdown
Contributor Author

@phillipleblanc PTAL when you have a moment 🙌

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @avantgardnerio. This is purely additive and doesn't break any APIs, so LGTM. I did an AI review and nothing was flagged.

/// aborts its inner tokio task on drop, so holding these here ties the
/// background work's lifetime to this exec's — dropping the exec
/// cancels all scatter work.
_drop_helper: Vec<SpawnedTask<()>>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the underscore? just curious

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just to signify that no one reads this. It's just a struct member purely so it can get held until the struct is dropped.

@avantgardnerio
avantgardnerio merged commit 5e6201e into apache:main Jul 25, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants